feature 2034: SdkHttpFullRequest builder.URI conditionally accept que… #2082

Strat1987 · 2020-10-07T12:48:39Z

Description

When calling the SdkHttpFullRequest builder using the URI default method, query parameters are now supported in the provided URI if keepUriQueryParams is set to true on the builder.\nThis can be useful in case you want to provide an already fully formed URI like a callback URI.

Motivation and Context

Query parameters that were provided to the builder in the form of the passed URI were omitted before
#2034

Testing

added test cases in SdkHttpRequestResponseTest

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

License

I confirm that this pull request can be released under the Apache 2 license

Strat1987 · 2020-10-08T08:05:32Z

I'm unsure as to why the CI failed, is there a way I can retrigger?

dagnir

Thank you for the PR!

http-client-spi/src/main/java/software/amazon/awssdk/http/DefaultSdkHttpFullRequest.java

dagnir · 2020-10-10T00:00:52Z

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpRequest.java

+                       .map(s -> s.split("=", 2))
+                       .collect(groupingBy(a -> a[0], mapping(a -> a[1], toList())))
+                       .forEach((paramKey, paramValues) -> paramValues
+                           .forEach(paramValue -> this.appendUriQueryParameter(paramKey, paramValue)));


I don't think appending would be the correct behavior here; the current set of query parameters should be cleared; otherwise something like this

builder.uri(myUri1).uri(myUri2);

will contain the query parameters from both URIs.

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpRequest.java

dagnir · 2020-10-12T19:33:33Z

Hmm it's probably easier to reset your branch, something like:

git fetch origin
git checkout -b reset-branch origin/master
git checkout 9d6fae0647cc2516d6cefb7ad6ea70a2b11a89dd http-client-spi
git checkout 9d6fae0647cc2516d6cefb7ad6ea70a2b11a89dd .changes
git commit

dagnir · 2020-10-12T19:44:09Z

BTW, I suspect what happened was that the when attempting to squash the commits, the merge commit was included in the commit range

Strat1987 · 2020-10-13T17:06:49Z

@dagnir I understand this leaves me with a local reset-branch including the latest changes from upstream (as I have origin set to my fork). It's unclear to me how I can then make sure to force push this to the httpclientspi-uri-with-query-params branch on my origin?

Strat1987 · 2020-10-13T17:10:12Z

@dagnir I've temporary deleted my local httpclientspi-uri-with-query-params branch, checkout -b from the reset-branch to a new local httpclientspi-uri-with-query-params and then force pushed that.
Seems cumbersome to me, as well as it does not store my local changes in case something goes wrong (although I could've temporary branched of in a backup branch before deleting the local copy)

dagnir · 2020-10-13T17:55:06Z

I understand this leaves me with a local reset-branch including the latest changes from upstream (as I have origin set to my fork). It's unclear to me how I can then make sure to force push this to the httpclientspi-uri-with-query-params branch on my origin?

You can use a refspec to push between your branches

git push origin reset-branch:httpclientspi-uri-with-query-params

dagnir · 2020-10-13T17:57:08Z

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpRequest.java

-                       .host(uri.getHost())
-                       .port(uri.getPort())
-                       .encodedPath(SdkHttpUtils.appendUri(uri.getRawPath(), encodedPath()));
+                                                 .host(uri.getHost())
+                                                 .port(uri.getPort())
+                                                 .encodedPath(SdkHttpUtils.appendUri(uri.getRawPath(), encodedPath()));
+        }


Please revert formatting change here

dagnir · 2020-10-13T17:57:35Z

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpFullRequest.java

            return this.protocol(uri.getScheme())
-                       .host(uri.getHost())
-                       .port(uri.getPort())
-                       .encodedPath(SdkHttpUtils.appendUri(uri.getRawPath(), encodedPath()));
+                    .host(uri.getHost())
+                    .port(uri.getPort())
+                    .encodedPath(SdkHttpUtils.appendUri(uri.getRawPath(), encodedPath()));
+        }


Please revert formatting change

dagnir · 2020-10-13T18:02:51Z

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpFullRequest.java

+                Pattern.compile("\\s*&|%26\\s*")
+                       .splitAsStream(uri.getQuery().trim())
+                       .map(s -> s.split("=|%3D", 2))
+                       .collect(groupingBy(a -> a[0], mapping(a -> a[1], toList())))
+                       .forEach((paramKey, paramValues) -> paramValues
+                           .forEach(paramValue -> this.appendUriQueryParameter(paramKey, paramValue)));


This code is pretty complex and is duplicated here and in SdkHttpRequest. Let's move this out into SdkHttpUtils instead. Should also make the parsing easier to test in isolation

dagnir · 2020-10-13T18:19:53Z

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpFullRequest.java

+        default Builder uriWithQueryParams(URI uri) {
+            Builder builder = this.uri(uri);
+            if (uri.getQuery() != null) {
+                Pattern.compile("\\s*&|%26\\s*")


I don't think we should including %26 here as the literal & should be the only delimiter we look for.

Compiling a pattern each time is inefficient. Once this logic is moved into SdkHttpUtils, let's extract the pattern out into a static constant

In appendUriQueryParameter impl I now decode the incoming paramValue as was suggested before, which is why I reasoned to also split on the decoded delimiter as otherwise the value would not be split properly before appending?

Unfortunately I don't think that's the right strategy. We should do the decoding after we have the individual components of the query parsed out. Otherwise, if the the input URI has and encoded & or =, then we would be parsing it incorrectly. It's perfectly valid for the parameter name or value to have & or = it as long as it's encoded.

For example, the pair [contains=equals, foo] in a URI would be https://foo.bar?contains%3Dequals=foo, but the code as is will split this as [contains, equals=foo]

I think the general strategy should be

get components of query string

foreach component

decode name

decode value if present

put raw query param

I haven't thought about it in that way, makes sense to split like that and then decode both parts before adding.

On reusing the put raw query param bullet, that would mean we cannot differ on where te params come from as I did now in:

private Map<String, List<String>> determineQueryParameters(Builder builder) { if (builder.uriQueryParameters.isEmpty()) { return builder.queryParametersAreFromToBuilder ? builder.queryParameters : deepUnmodifiableMap(builder.queryParameters, () -> new LinkedHashMap<>()); } return builder.uriQueryParameters; }

Hmm I don't think it should be necessary to distinguish the source. uriWithQueryParams acts basically like a static constructor for SdkHttpRequest

Attempted to reflect this train of thought

dagnir · 2020-10-13T18:26:53Z

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpFullRequest.java

+            if (uri.getQuery() != null) {
+                Pattern.compile("\\s*&|%26\\s*")
+                       .splitAsStream(uri.getQuery().trim())
+                       .map(s -> s.split("=|%3D", 2))


Same here, should only splitting on the literal =.

Also seems like this assumes each query parameter is a name-value pair; we should support parameters that don't have a value as well, like in https://github.com/aws/aws-sdk-java-v2/pull/2082?foo

dagnir · 2020-10-13T18:31:36Z

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpRequest.java

+         * @param paramName  The name of the query parameter to add
+         * @param paramValue The un-encoded value for the query parameter.
+         */
+        Builder appendUriQueryParameter(String paramName, String paramValue);


Hmm why do we need this instead of using putRawQueryParameter?

since I wanted to make sure not to interfere with already in place param handling

dagnir · 2020-10-13T18:34:25Z

.changes/next-release/feature-HTTPclientspi-25d1ed9.json

@@ -0,0 +1,5 @@
+{
+    "category": "HTTP client spi",


Please capitalize here to HTTP Client SPI to be consistent with the rest of the log entries

I made an educated guess before so was unaware of the casing significance

dagnir · 2020-10-13T19:17:46Z

utils/src/main/java/software/amazon/awssdk/utils/http/SdkHttpUtils.java

+     */
+    public static Map<String, List<String>> uriParams(URI uri) {
+        return Pattern.compile(QUERY_PARAM_PATTERN)
+               .splitAsStream(uri.getQuery().trim())


BTW, I think we should be looking at getRawQuery() here, since getQuery() will give you the decoded form which is not what we want

You're right, updated and adapted unit test

dagnir · 2020-10-13T19:25:46Z

http-client-spi/src/test/java/software/amazon/awssdk/http/SdkHttpRequestResponseTest.java

+        URI uri = new URI("https://github.com/aws/aws-sdk-java-v2/issues/2034?reqParam=1234&oParam=3456%26reqParam%3D5678");
+        final SdkHttpFullRequest sdkHttpFullRequest =
+            SdkHttpFullRequest.builder().method(SdkHttpMethod.POST).uriWithQueryParams(uri).build();
+        assertThat(sdkHttpFullRequest.getUri().getQuery()).contains("reqParam=1234", "oParam=3456", "reqParam=5678");


I don't think this assertion is correct per my comment earlier re: parsing.

Splitting reqParam=1234&oParam=3456%26reqParam%3D5678 on the & delimiter should give reqParam=1234 and oParam=3456%26reqParam%3D5678 as the two name-value pairs, then parsing each as a pair gives [reqParam, 1234] and [oParam, 3456%26reqParam%3D5678]

Today I learned! updated and adapted unit test

dagnir · 2020-10-13T19:58:30Z

http-client-spi/src/main/java/software/amazon/awssdk/http/DefaultSdkHttpFullRequest.java

+                this.uriQueryParameters.computeIfAbsent(paramName, k -> new ArrayList<>())
+                                       .add(URLDecoder.decode(paramValue, StandardCharsets.UTF_8.toString()));
+            } catch (UnsupportedEncodingException e) {
+                log.warn("Could not decode {}={} using {}", paramName, paramValue, StandardCharsets.UTF_8);


We should just fail instead of logging a warning here

method is no longer needed

dagnir · 2020-10-13T23:25:45Z

Discussed with millems@ and we think just changing the uri() behavior is okay; we don't need to introduce a separate method. We acknowledge the possibility of this can break some use case out there, but the opposite is more likely that customers are expecting the request to contain the query params from the input URI.

roexber · 2020-10-14T19:45:21Z

I'm hoping the current version is aligned with the expectations

codecov-io · 2020-10-14T20:20:14Z

Codecov Report

Merging #2082 into master will increase coverage by 0.00%.
The diff coverage is 76.74%.

@@            Coverage Diff            @@
##             master    #2082   +/-   ##
=========================================
  Coverage     77.12%   77.13%           
- Complexity      298      299    +1     
=========================================
  Files          1204     1206    +2     
  Lines         37957    37924   -33     
  Branches       2984     2963   -21     
=========================================
- Hits          29274    29251   -23     
+ Misses         7241     7228   -13     
- Partials       1442     1445    +3

Flag	Coverage Δ	Complexity Δ
#unittests	`77.13% <76.74%> (+<0.01%)`	`299.00 <0.00> (+1.00)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...va/software/amazon/awssdk/http/SdkHttpRequest.java	`33.33% <0.00%> (-5.13%)`	`4.00 <0.00> (+1.00)`	⬇️
...nced/dynamodb/model/UpdateItemEnhancedRequest.java	`64.28% <ø> (ø)`	`0.00 <0.00> (ø)`
...amodb/internal/operations/UpdateItemOperation.java	`97.36% <81.81%> (-1.68%)`	`0.00 <0.00> (ø)`
...ed/dynamodb/internal/mapper/UpdateBehaviorTag.java	`85.71% <85.71%> (ø)`	`0.00 <0.00> (?)`
...on/awssdk/profiles/internal/ProfileFileReader.java	`93.38% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...oftware/amazon/awssdk/http/SdkHttpFullRequest.java	`92.30% <100.00%> (+54.80%)`	`1.00 <0.00> (ø)`
.../internal/mapper/BeanTableSchemaAttributeTags.java	`100.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`
.../enhanced/dynamodb/mapper/StaticAttributeTags.java	`91.11% <100.00%> (+0.20%)`	`0.00 <0.00> (ø)`
...wssdk/enhanced/dynamodb/mapper/UpdateBehavior.java	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
...mazon/awssdk/utils/async/DelegatingSubscriber.java	`77.77% <0.00%> (-22.23%)`	`0.00% <0.00%> (ø%)`
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c55b6b6...54154d9. Read the comment docs.

dagnir · 2020-10-14T20:44:11Z

utils/src/main/java/software/amazon/awssdk/utils/http/SdkHttpUtils.java

+     * Extracts query parameters from the given URI
+     */
+    public static Map<String, List<String>> uriParams(URI uri) {
+        return Pattern.compile(QUERY_PARAM_PATTERN)


The result of Pattern.compile() itself should be reused, so we don't need to call compile() each time.

private static final Pattern QUERY_PARAM_PATTERN = ...

Same for split() below since it also uses regex compile under the hood

BTW, I think a more accurate name would be "QUERY_PARAM_DELIMETER_PATTERN"

the split is safe to use in my opinion since it uses a StringTokenizer if only a single character is provided since java 7 according to http://hg.openjdk.java.net/jdk7/jdk7/jdk/rev/1ff977b938e5

dagnir · 2020-10-14T21:09:05Z

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpRequest.java

+            if (uri.getRawQuery() != null) {
+                SdkHttpUtils.uriParams(uri)
+                            .forEach(this::putRawQueryParameter);
+            }
+            return builder;


The existing set of query parameters set on the builder should be cleared first

dagnir · 2020-10-14T21:09:31Z

http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpFullRequest.java

+            if (uri.getRawQuery() != null) {
+                SdkHttpUtils.uriParams(uri)
+                            .forEach(this::putRawQueryParameter);
+            }
+            return builder;


Same here, the existing set of query parameters set on the builder should be cleared first

dagnir · 2020-10-14T21:15:03Z

utils/src/test/java/software/amazon/awssdk/utils/SdkHttpUtilsTest.java

@@ -173,4 +178,16 @@ public void headersFromCollectionWorksCorrectly() {
        assertThat(SdkHttpUtils.firstMatchingHeaderFromCollection(headers, asList("foo", "nothing"))).hasValue("bar");
        assertThat(SdkHttpUtils.firstMatchingHeaderFromCollection(headers, asList("foo", "other"))).hasValue("foo");
    }
+
+    @Test
+    public void uriParams() throws URISyntaxException {


We should have tests for the two requests classes that we changed as well.

dagnir · 2020-10-14T21:22:27Z

utils/src/main/java/software/amazon/awssdk/utils/http/SdkHttpUtils.java

+    public static Map<String, List<String>> uriParams(URI uri) {
+        return Pattern.compile(QUERY_PARAM_PATTERN)
+               .splitAsStream(uri.getRawQuery().trim())
+               .map(s -> s.contains("=") ? s.split("=", 2) : new String[] {s, ""})


Hmm I don't think using an empty string when there's no = is what we want; we should be setting null if there's no = for the parameter. There should be a test for the result of something like this is

URI myUri = URI.create("https://github.com/aws/aws-sdk-for-java-v2?foo"); SdkHttpFullRequest request = SdkHttpFullRequest.builder().uri(myUri).build(); request.getUri();

to make sure that the result is not https://github.com/aws/aws-sdk-for-java-v2?foo=, which will likely be interpreted differently by the server

dagnir · 2020-10-14T21:23:07Z

utils/src/test/java/software/amazon/awssdk/utils/SdkHttpUtilsTest.java

+        Map<String, List<String>> uriParams = SdkHttpUtils.uriParams(uri);
+        assertThat(uriParams).contains(entry("reqParam", Arrays.asList("1234", "5678")),
+                                       entry("oParam", Collections.singletonList("3456")),
+                                       entry("noval", Arrays.asList("")),


As mentioned above, the value for noval should be null, not a list containing "".

roexber · 2020-10-15T19:16:08Z

Hopeful we're getting to a common understanding of the solution 🙏

dagnir

LGTM! I'm going kick off an integ test just to make sure none of those tests were affected by this change.

… provided URI

dagnir · 2020-10-16T17:45:31Z

! Thank you for all your work on this!

sonarqubecloud · 2020-10-16T20:32:47Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

36.8% Coverage
0.0% Duplication

roexber · 2020-10-19T06:44:09Z

! Thank you for all your work on this!

I'm happy I was able to contribute and grateful for the review. I've learned a thing or two!

…b4814c620 Pull request: release <- staging/5c7164ee-e3ea-4091-9540-da3b4814c620

dagnir requested changes Oct 10, 2020

View reviewed changes

Strat1987 force-pushed the httpclientspi-uri-with-query-params branch from 9d6fae0 to 7dc5615 Compare October 13, 2020 17:08

Strat1987 force-pushed the httpclientspi-uri-with-query-params branch 2 times, most recently from 46ee998 to ee07422 Compare October 13, 2020 18:35

dagnir reviewed Oct 13, 2020

View reviewed changes

Strat1987 force-pushed the httpclientspi-uri-with-query-params branch 4 times, most recently from c42de89 to f0d18f9 Compare October 13, 2020 19:09

dagnir reviewed Oct 13, 2020

View reviewed changes

Strat1987 force-pushed the httpclientspi-uri-with-query-params branch 4 times, most recently from 4cd0e2d to a5f5dad Compare October 14, 2020 19:41

dagnir reviewed Oct 14, 2020

View reviewed changes

Strat1987 force-pushed the httpclientspi-uri-with-query-params branch from a5f5dad to 8ef3f77 Compare October 15, 2020 19:15

dagnir reviewed Oct 15, 2020

View reviewed changes

debora-ito linked an issue Oct 16, 2020 that may be closed by this pull request

SdkHttpFullRequest builder.URI removes query parameters #2034

Closed

feature 2034: SdkHttpFullRequest builder.uri keep query parameters of…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

c91e770

… provided URI

Strat1987 force-pushed the httpclientspi-uri-with-query-params branch from 819cd77 to c91e770 Compare October 16, 2020 08:38

dagnir approved these changes Oct 16, 2020

View reviewed changes

dagnir added 2 commits October 16, 2020 10:46

Merge branch 'master' into httpclientspi-uri-with-query-params

54154d9

Merge branch 'master' into httpclientspi-uri-with-query-params

90d7143

dagnir merged commit 30660f4 into aws:master Oct 17, 2020

aws-sdk-java-automation added a commit that referenced this pull request Jul 5, 2022

Merge pull request #2082 from aws/staging/5c7164ee-e3ea-4091-9540-da3…

2b598d1

…b4814c620 Pull request: release <- staging/5c7164ee-e3ea-4091-9540-da3b4814c620

feature 2034: SdkHttpFullRequest builder.URI conditionally accept que… #2082

feature 2034: SdkHttpFullRequest builder.URI conditionally accept que… #2082

Conversation

Strat1987 commented Oct 7, 2020

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Strat1987 commented Oct 8, 2020

dagnir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagnir commented Oct 12, 2020

dagnir commented Oct 12, 2020

Strat1987 commented Oct 13, 2020

Strat1987 commented Oct 13, 2020

dagnir commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagnir commented Oct 13, 2020

roexber commented Oct 14, 2020

codecov-io commented Oct 14, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roexber commented Oct 15, 2020

dagnir left a comment

Choose a reason for hiding this comment

dagnir commented Oct 16, 2020

sonarqubecloud bot commented Oct 16, 2020

roexber commented Oct 19, 2020

codecov-io commented Oct 14, 2020 •

edited

Loading