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. 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 <[email protected]> wrote: > >> >> On 5 May 2017, at 17:11, Sven Van Caekenberghe <[email protected]> wrote: >> >> Max, >> >>> On 5 May 2017, at 16:59, Max Leske <[email protected]> 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
