Skip to content

Conversation

@abrooksv
Copy link
Contributor

@abrooksv abrooksv commented Jan 2, 2019

  • Also include a Location header if the request is 3xx in SdkHttpClientTestSuite

Description

Always disable following redirects in UrlConnectionHttpClient since SDK handles redirects manually.

Motivation and Context

Fixes #975

Testing

Fixed unit tests not adding in Location header, which caused software.amazon.awssdk.http.SdkHttpClientTestSuite#supportsResponseCode302 to fail

Types of changes

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

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • A short description of the change has been added to the CHANGELOG
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you be more specific about what was changed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment why we do this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually exercise this code path?

Copy link
Contributor Author

@abrooksv abrooksv Jan 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, supportsResponseCode301 and supportsResponseCode302 both trigger it.

supportsResponseCode302 -> testForResponseCode -> stubForMockRequest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without disable leads to test failure:

java.lang.NullPointerException: delegate must not be null.

	at software.amazon.awssdk.utils.Validate.paramNotNull(Validate.java:118)
	at software.amazon.awssdk.http.AbortableInputStream.<init>(AbortableInputStream.java:35)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 awesome that you added a base-test for this so that we catch it for other implementations!

* Also include a Location header if the request is 3xx in SdkHttpClientTestSuite
@abrooksv abrooksv force-pushed the disableHtppUrlRedirects branch from 9d530e9 to c279296 Compare January 2, 2019 19:20
@codecov-io
Copy link

codecov-io commented Jan 2, 2019

Codecov Report

Merging #989 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #989      +/-   ##
============================================
- Coverage     55.36%   55.36%   -0.01%     
+ Complexity     4493     4492       -1     
============================================
  Files           799      799              
  Lines         27449    27450       +1     
  Branches       2220     2220              
============================================
  Hits          15198    15198              
  Misses        11533    11533              
- Partials        718      719       +1
Impacted Files Coverage Δ Complexity Δ
...dk/http/urlconnection/UrlConnectionHttpClient.java 80.64% <100%> (+0.31%) 10 <0> (ø) ⬇️
...are/amazon/awssdk/core/internal/util/Mimetype.java 78.04% <0%> (-2.44%) 14% <0%> (-1%)

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 d6d2022...e39efcd. Read the comment docs.

@millems millems merged commit 2e9e10d into aws:master Jan 2, 2019
@abrooksv abrooksv deleted the disableHtppUrlRedirects branch January 2, 2019 21:46
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

Successfully merging this pull request may close these issues.

HttpUrlConnection handles failures differently compared to Apache

4 participants