Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-Value Query Overrides #52

Open
ajohnstonTE opened this issue Jun 11, 2019 · 2 comments
Open

Multi-Value Query Overrides #52

ajohnstonTE opened this issue Jun 11, 2019 · 2 comments

Comments

@ajohnstonTE
Copy link
Contributor

In the Query class, you can override values via the Query#put methods. However, Query#getStrings and the other multi-value methods do not use the override map. They can't really, because the override field is a Map<String, String>. This means that if you were to use a transformative validator, for example Lowercase, then the result of Query#getStrings is unaffected. This is inconsistent with the behavior you'd expect from that validator as well as the result/behavior you'd get from calling Query#get instead.

On the note of the override field, I think that it should instead be a Map<String, List<String>, so in the case of single-valued keys it would be a singleton list. Then add another Query#put method that accepts an array of strings (as well as ones accepting ints, longs, and booleans, though the only critical one is the array of strings).

This could become tricky with regards to InputTransformer. But to keep it simple, it should just use getStrings every time, then have the transformation process be performed on each element in the list individually so that it can then set the override to the transformed list as a whole. This way it avoids the need to change any transformative validators. If someone wants to transform whole lists of values as a group, that's fine and they can still do that as a custom class, but InputTransformer would simply be a class for transforming each matching value, not each collective set of values.

See:

@bhauer
Copy link

bhauer commented Jun 12, 2019

Good find and I agree that making the behavior of getStrings for multi-value keys work as expected is important.

I am 95% satisfied with the plan to change the override map to use List<String> as the values. Although that is wasteful for the majority circumstance (the majority of values will be a String encapsulated in a single-entry List), the use of overrides in general is fairly limited, giving this a relatively small impact on garbage generation overall.

That said, if there is a tactic that avoids the construction of 1-entry Lists for each value and is not especially difficult to implement or constraining, I am interested in exploring that option.

@ajohnstonTE
Copy link
Contributor Author

FWIW, treating them all as lists isn't any different from how HttpRequest handles it (which is where these values are being pulled from in a typical request).

Still, just to throw an idea out there, you could manage two override maps, one with single string values, the other with multi-string values. The single string Query#put method would place the given key-value into the single-string map, and clear out that key in the multi-string map. The multi-string Query#put method would do the same, but in reverse. Query#get would need to first check the single string map, then the multi-string map at which point it would just grab the first value (see below). Query#getStrings would first check the multi-string map, then the single string map (and in this case, just return a single-valued array). Finally, the remaining methods that interact with override (Query#has, Query#remove, Query#clear, Query#names, Query#lazyInitializeOverride) would all do exactly what you'd expect. Test/clear/initialize both maps, delete from both maps, etc.

Note: For consistency with how HttpRequest#getParameter works, Query#get should just grab the first value in the list if it does end up pulling from the multi-string override map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants