On 11 May 2017, at 13:05, Max Leske <maxleske@gmail.com> wrote:

On 11 May 2017, at 11:49, Sven Van Caekenberghe <sven@stfx.eu> wrote:

Hi Max,

First thank you for the feedback, the discussion so far and your patches. I studied them carefully.

Still, I decided to take another road, implementation wise.

You see, the reason a global setting does not make sense is that there can (and are) multiple Zn clients and servers active in the same image, and each should be independent, not be influenced by configuration changes in others. This is why the use of dynamic variables fits so well.

I refactored the current situation a bit and added the necessary high level hooks. I moved the default to each dynamic variable class itself, removed it from ZnConstants, and added options to both ZnClient and ZnServer.

Please test (code committed in bleedingEdge) and let me know if it works for you (especially then #defaultEncoder part). Maybe we have to iterate more over this to fully support your use case.

I will. Thanks Sven!

Regards,

Sven

PS: One resource limit not yet moved under the new scheme is the maximum line length (currently set to 4096).

On 6 May 2017, at 15:30, Max Leske <maxleske@gmail.com> wrote:

On 5 May 2017, at 17:11, Sven Van Caekenberghe <sven@stfx.eu> wrote:

Max,

On 5 May 2017, at 16:59, Max Leske <maxleske@gmail.com> wrote:

Hi,

I'm performing a legal request that has more than 4000 parameters. This causes the Zinc server to return 400: Bad Request because ZnMultiValueDictionary is limited to 256 entries by default.

The dictionary has the option to remove the limit or to adjust it with a dynamic variable. Unfortunately, I don't see any way to properly configure this without monkey patching Zinc. Ideally, I'd like to remove the limit (which can't be done through the dynamic variable by the way because when the dynamic variable answers nil, the default will be set to 256).

The first thing that comes to mind is to move this setting to ZnConstants, but then I don't see any way to configure ZnConstants either (ZnConstants is referenced directly by its users). Maybe ZnConstants could be changed to hold a concrete constants class (itself by default).

In any case, I think this setting should be configurable and the configuration should be possible through one single entry point, together with options like #codec.

Thoughts?

Cheers,
Max

You are the first one to complain about this limit. 

This one and other resource limits exist to protect the client/server against abuse and attacks.

I'm aware of that but have not other way to do this right now. We have mod_security with a higher value configured for that.

I think what is needed is something like ZnServer>>#withMaximumEntitySizeDo: which uses the server option #maximumEntitySize. Would that work for you, you think ?

Yes, it would. I've implemented the change, closely following the semantics of #withMaximumEntitySizeDo:. While I was working on that I discovered another problem I had which I also fixed by dispatching to ZnConstants. The problem being, that I set the codec on the server to GRNullCodec, but ZnPercentEncoder would still always use a ZnUTF8Encoder to decode requests. The result was an error when the server tried to write wide strings onto the stream (usually ZnUTF8Encoder: UTF-8 -> WideString, GRPharoUtf8Codec: WideString -> UTF-8).

I've attached the patches.

A couple of notes:
- #defaultMaximumNumberOfDictionaryEntries and #maximumDictionaryEntries: have the same implementation, which is unnecessary in my opinion but I've copied the code from #maximumEntitySize:.
- I'm not sure whether storing the option a second time on ZnServer makes sense, but, again, I stuck to the existing implementation
- #withMaximumNumberOfDictionaryEntriesDo: also checks the dynamic variable, which is overkill I think, as ZnConstants does the same thing. Again, I stuck to the existing implementation (which means that ZnConstants is actually thread agnostic and references to dynamic variables should probably all be on ZnConstants)


Cheers,
Max

Sven