Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1520943002:
Support the same parameter key more than once in Uri query parameters. (Closed)
|
Created:
5 years ago by Lasse Reichstein Nielsen Modified:
4 years, 11 months ago CC:
reviews_dartlang.org Base URL:
https://github.com/dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionSupport the same parameter key more than once in Uri query parameters.
R=floitsch@google.com, sgjesse@google.com
Committed: https://github.com/dart-lang/sdk/commit/f70bef4a2c7c6cc6f0a6670cfae2f621842f163c
Patch Set 1 #
Total comments: 8
Patch Set 2 : Adding tests. #
Total comments: 12
Patch Set 3 : Added CHANGELOG. Name is queryParametersAll. #Patch Set 4 : Address some comments #
Total comments: 8
Patch Set 5 : Address comments. #
Total comments: 1
Messages
Total messages: 28 (4 generated)
lrn@google.com changed reviewers: + floitsch@google.com, sgjesse@google.com
WDYT? (I'll add tests if it's acceptable).
I think it's ok, but I'm not sure how important it is. Soeren? https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart#newco... sdk/lib/core/uri.dart:1115: * one of the values. The [queryParameterValues] getter can provide a which one? https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart#newco... sdk/lib/core/uri.dart:2213: void parsePair(int start, int equalsIndex, int end) { new line before and after nested functions. https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart#newco... sdk/lib/core/uri.dart:2221: _uriDecode(encodedComponent, start, equalsIndex, encoding, true); nit: it looks like this would fit on the previous line.
lgtm, but it is missing tests I think it is relevant. It is a valid use-case, and it has been raised from time to time. https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart#newco... sdk/lib/core/uri.dart:1386: if (value == null) { Maybe this is OK, but if value is null or the empty string we get key in the query string. However if value is the empty list we don't get anything in the query string. Should the empty list also result in key in the query string? Maybe not.
https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart#newco... sdk/lib/core/uri.dart:1386: if (value == null) { I think a "foo":[] entry should not introduce a "foo" key. This allows you to take a collection and store it without special casing the empty case. It does mean that when reading it back you will never get an empty list, so you will have to special case that. That's still better done if you get null than if you get the empty string.
https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:79: Map<String, List<String>> _queryParameterLists; I'm wondering if these can share something, but I don't think so. It's still annoying to have all these extra fields. Maybe we should use an expando instead? https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1144: Map<String, List<String>> get queryParameterLists { I'm not particularly happy with the name. Suggestions appreciated. https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1408: } Maybe I should extract some of this into a helper function. https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:2213: static Map _splitQueryStringAll( This one is private, while splitQueryString is public. I'm not sure why it's public.
https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:79: Map<String, List<String>> _queryParameterLists; On 2015/12/14 13:16:54, Lasse Reichstein Nielsen wrote: > I'm wondering if these can share something, but I don't think so. > It's still annoying to have all these extra fields. > Maybe we should use an expando instead? I don't really like expandos, but they would save 3 fields... I don't think there are enough URIs in a program to justify it. Furthermore we would implicitly drag in Expandos (increasing the size of snapshots...). https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1144: Map<String, List<String>> get queryParameterLists { On 2015/12/14 13:16:54, Lasse Reichstein Nielsen wrote: > I'm not particularly happy with the name. Suggestions appreciated. multiQueryParameters?
https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1144: Map<String, List<String>> get queryParameterLists { On 2015/12/14 17:24:54, floitsch wrote: > On 2015/12/14 13:16:54, Lasse Reichstein Nielsen wrote: > > I'm not particularly happy with the name. Suggestions appreciated. > > multiQueryParameters? +1
https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1144: Map<String, List<String>> get queryParameterLists { I don't think that works very well. It sounds like there are multiple queries, not multiple parameters of the query. So, queryMultiParameters? queryParametersAll?
https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1144: Map<String, List<String>> get queryParameterLists { On 2015/12/15 08:11:28, Lasse Reichstein Nielsen wrote: > I don't think that works very well. It sounds like there are multiple queries, > not multiple parameters of the query. > > So, > queryMultiParameters? > queryParametersAll? alternatively we could go the long way: queryParametersWithDuplicates
https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1144: Map<String, List<String>> get queryParameterLists { On 2015/12/15 09:17:52, floitsch wrote: > alternatively we could go the long way: > > queryParametersWithDuplicates Longer could do it, but "with duplicates" doesn't tell me what it does. It needs to be read in the context of "queryParameters" and you need to know that that one has no duplicates. It's hard to find a name because the best name is already taken.
On 2015/12/16 13:06:21, Lasse Reichstein Nielsen wrote: > https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart > File sdk/lib/core/uri.dart (right): > > https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... > sdk/lib/core/uri.dart:1144: Map<String, List<String>> get queryParameterLists { > On 2015/12/15 09:17:52, floitsch wrote: > > > alternatively we could go the long way: > > > > queryParametersWithDuplicates > > Longer could do it, but "with duplicates" doesn't tell me what it does. It needs > to be read in the context of "queryParameters" and you need to know that that > one has no duplicates. > > It's hard to find a name because the best name is already taken. and I am not sure it is even duplicates - each key can have several values, but the values ae not necessarily "duplicates". queryParametersWithMultiValues ? queryParametersWithValueLists ? queryParametersFull ? queryParametersComplete ? Still nothing really precise...
On 2015/12/16 19:59:51, Søren Gjesse wrote: > On 2015/12/16 13:06:21, Lasse Reichstein Nielsen wrote: > > https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart > > File sdk/lib/core/uri.dart (right): > > > > > https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... > > sdk/lib/core/uri.dart:1144: Map<String, List<String>> get queryParameterLists > { > > On 2015/12/15 09:17:52, floitsch wrote: > > > > > alternatively we could go the long way: > > > > > > queryParametersWithDuplicates > > > > Longer could do it, but "with duplicates" doesn't tell me what it does. It > needs > > to be read in the context of "queryParameters" and you need to know that that > > one has no duplicates. > > > > It's hard to find a name because the best name is already taken. > > and I am not sure it is even duplicates - each key can have several values, but > the values ae not necessarily "duplicates". > > queryParametersWithMultiValues ? > > queryParametersWithValueLists ? > > queryParametersFull ? > > queryParametersComplete ? > > Still nothing really precise... queryParametersAll? allQueryParameters? - it's still weird because saying "all" shouldn't be necessary, it's only relevant relative to the other queryParameters which isn't "all". That is: The precise thing is "queryParameters" - it is all of the query parameters (as a map to lists). It's the current behavior of queryParameters that should have the modifier because it actually doesn't reveal the query parameters. Do you think it would be breaking to change queryParameters to return lists when there are more than one value. It would break existing code that depended on getting one value out of many, but that code would likely be wrong anyway.
On 2015/12/17 08:38:40, Lasse Reichstein Nielsen wrote: > On 2015/12/16 19:59:51, Søren Gjesse wrote: > > On 2015/12/16 13:06:21, Lasse Reichstein Nielsen wrote: > > > https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart > > > File sdk/lib/core/uri.dart (right): > > > > > > > > > https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... > > > sdk/lib/core/uri.dart:1144: Map<String, List<String>> get > queryParameterLists > > { > > > On 2015/12/15 09:17:52, floitsch wrote: > > > > > > > alternatively we could go the long way: > > > > > > > > queryParametersWithDuplicates > > > > > > Longer could do it, but "with duplicates" doesn't tell me what it does. It > > needs > > > to be read in the context of "queryParameters" and you need to know that > that > > > one has no duplicates. > > > > > > It's hard to find a name because the best name is already taken. > > > > and I am not sure it is even duplicates - each key can have several values, > but > > the values ae not necessarily "duplicates". > > > > queryParametersWithMultiValues ? > > > > queryParametersWithValueLists ? > > > > queryParametersFull ? > > > > queryParametersComplete ? > > > > Still nothing really precise... > > queryParametersAll? > allQueryParameters? > > - it's still weird because saying "all" shouldn't be necessary, it's only > relevant relative to the other queryParameters which isn't "all". > > That is: The precise thing is "queryParameters" - it is all of the query > parameters (as a map to lists). > It's the current behavior of queryParameters that should have the modifier > because it actually doesn't reveal the query parameters. > > Do you think it would be breaking to change queryParameters to return lists when > there are more than one value. It would break existing code that depended on > getting one value out of many, but that code would likely be wrong anyway. Unfortunately yes. You could easily crash servers this way.
On 2015/12/17 08:58:08, floitsch wrote: > On 2015/12/17 08:38:40, Lasse Reichstein Nielsen wrote: > > On 2015/12/16 19:59:51, Søren Gjesse wrote: > > > On 2015/12/16 13:06:21, Lasse Reichstein Nielsen wrote: > > > > > https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart > > > > File sdk/lib/core/uri.dart (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... > > > > sdk/lib/core/uri.dart:1144: Map<String, List<String>> get > > queryParameterLists > > > { > > > > On 2015/12/15 09:17:52, floitsch wrote: > > > > > > > > > alternatively we could go the long way: > > > > > > > > > > queryParametersWithDuplicates > > > > > > > > Longer could do it, but "with duplicates" doesn't tell me what it does. It > > > needs > > > > to be read in the context of "queryParameters" and you need to know that > > that > > > > one has no duplicates. > > > > > > > > It's hard to find a name because the best name is already taken. > > > > > > and I am not sure it is even duplicates - each key can have several values, > > but > > > the values ae not necessarily "duplicates". > > > > > > queryParametersWithMultiValues ? > > > > > > queryParametersWithValueLists ? > > > > > > queryParametersFull ? > > > > > > queryParametersComplete ? > > > > > > Still nothing really precise... > > > > queryParametersAll? > > allQueryParameters? > > > > - it's still weird because saying "all" shouldn't be necessary, it's only > > relevant relative to the other queryParameters which isn't "all". > > > > That is: The precise thing is "queryParameters" - it is all of the query > > parameters (as a map to lists). > > It's the current behavior of queryParameters that should have the modifier > > because it actually doesn't reveal the query parameters. > > > > Do you think it would be breaking to change queryParameters to return lists > when > > there are more than one value. It would break existing code that depended on > > getting one value out of many, but that code would likely be wrong anyway. > > Unfortunately yes. You could easily crash servers this way. I can live with queryParametersAll - indirectly indicate that queryParameters does not give you everything. Remember to rename the reference in the dartdoc for queryParameters.
On 2015/12/17 12:01:19, Søren Gjesse wrote: > On 2015/12/17 08:58:08, floitsch wrote: > > On 2015/12/17 08:38:40, Lasse Reichstein Nielsen wrote: > > > On 2015/12/16 19:59:51, Søren Gjesse wrote: > > > > On 2015/12/16 13:06:21, Lasse Reichstein Nielsen wrote: > > > > > > > https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart > > > > > File sdk/lib/core/uri.dart (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... > > > > > sdk/lib/core/uri.dart:1144: Map<String, List<String>> get > > > queryParameterLists > > > > { > > > > > On 2015/12/15 09:17:52, floitsch wrote: > > > > > > > > > > > alternatively we could go the long way: > > > > > > > > > > > > queryParametersWithDuplicates > > > > > > > > > > Longer could do it, but "with duplicates" doesn't tell me what it does. > It > > > > needs > > > > > to be read in the context of "queryParameters" and you need to know that > > > that > > > > > one has no duplicates. > > > > > > > > > > It's hard to find a name because the best name is already taken. > > > > > > > > and I am not sure it is even duplicates - each key can have several > values, > > > but > > > > the values ae not necessarily "duplicates". > > > > > > > > queryParametersWithMultiValues ? > > > > > > > > queryParametersWithValueLists ? > > > > > > > > queryParametersFull ? > > > > > > > > queryParametersComplete ? > > > > > > > > Still nothing really precise... > > > > > > queryParametersAll? > > > allQueryParameters? > > > > > > - it's still weird because saying "all" shouldn't be necessary, it's only > > > relevant relative to the other queryParameters which isn't "all". > > > > > > That is: The precise thing is "queryParameters" - it is all of the query > > > parameters (as a map to lists). > > > It's the current behavior of queryParameters that should have the modifier > > > because it actually doesn't reveal the query parameters. > > > > > > Do you think it would be breaking to change queryParameters to return lists > > when > > > there are more than one value. It would break existing code that depended on > > > getting one value out of many, but that code would likely be wrong anyway. > > > > Unfortunately yes. You could easily crash servers this way. > > I can live with queryParametersAll - indirectly indicate that queryParameters > does not give you everything. > > Remember to rename the reference in the dartdoc for queryParameters. "All" seems like the best choice.
kevmoo@google.com changed reviewers: + kevmoo@google.com
Tests? Changelog update?
If we accept this, I'll add a changelog (I have a test, it's currently in another CL).
PTAL https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart#newco... sdk/lib/core/uri.dart:1115: * one of the values. The [queryParameterValues] getter can provide a I really don't want to promise that. It's likely going to be the last one because of the way the map is being built. https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart#newco... sdk/lib/core/uri.dart:2213: void parsePair(int start, int equalsIndex, int end) { On 2015/12/11 19:00:58, floitsch wrote: > new line before and after nested functions. Done. https://codereview.chromium.org/1520943002/diff/1/sdk/lib/core/uri.dart#newco... sdk/lib/core/uri.dart:2221: _uriDecode(encodedComponent, start, equalsIndex, encoding, true); It does. Incredible! https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1144: Map<String, List<String>> get queryParameterLists { Going with queryParametersAll - which isn't perfect, but it's broken in a well-established way. https://codereview.chromium.org/1520943002/diff/20001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1408: } Done.
LGTM. https://codereview.chromium.org/1520943002/diff/60001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/60001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:144: * except for the unreserved characters, and replaces spaces with `+`. Consider breaking it into paragraphs. Use http://spec.commonmark.org/dingus/ for an approximate look at the current output. https://codereview.chromium.org/1520943002/diff/60001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:149: * the URI will have no query part. has no https://codereview.chromium.org/1520943002/diff/60001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1116: * one of the values. The [queryParametersAll] getter can provide a which one? Either explicitly state a "random" one, or state which one it is. https://codereview.chromium.org/1520943002/diff/60001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1132: * 17.13.4](http://www.w3.org/TR/REC-html40/interact/forms.html#h-17.13.4 "HTML 4.01 section 17.13.4"). make sure that this encodes correctly with dartdoc.
lgtm
Description was changed from ========== Support the same parameter key more than once in Uri query parameters. ========== to ========== Support the same parameter key more than once in Uri query parameters. R=floitsch@google.com, sgjesse@google.com Committed: https://github.com/dart-lang/sdk/commit/f70bef4a2c7c6cc6f0a6670cfae2f621842f163c ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as f70bef4a2c7c6cc6f0a6670cfae2f621842f163c (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1520943002/diff/60001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/1520943002/diff/60001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:144: * except for the unreserved characters, and replaces spaces with `+`. More paragraphs added. https://codereview.chromium.org/1520943002/diff/60001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:149: * the URI will have no query part. Changed here, and in other places in the file. https://codereview.chromium.org/1520943002/diff/60001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1116: * one of the values. The [queryParametersAll] getter can provide a Now say "arbitrary". https://codereview.chromium.org/1520943002/diff/60001/sdk/lib/core/uri.dart#n... sdk/lib/core/uri.dart:1132: * 17.13.4](http://www.w3.org/TR/REC-html40/interact/forms.html#h-17.13.4 "HTML 4.01 section 17.13.4"). Checks out correctly in the commonmark checker linked above.
Message was sent while issue was closed.
whesse@google.com changed reviewers: + whesse@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1520943002/diff/80001/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/1520943002/diff/80001/CHANGELOG.md#newcode15 CHANGELOG.md:15: * Addad `Uri.queryParametersAll` to handle multiple query parameters with This should go in a 1.15.0 section, since the change is not being cherry-picked to 1.14.
Message was sent while issue was closed.
ACK. Same should probably happen to ??-as-const since the VM change hasn't even landed yet. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
