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.

Sign up for GitHub

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

Jump to bottom

feat: update http requests, responses and endpoint with URI #719

Merged
merged 19 commits into from
May 29, 2024
Merged

feat: update http requests, responses and endpoint with URI #719

merged 19 commits into from
May 29, 2024

Conversation

AndrewFossAWS
Copy link
Contributor

@AndrewFossAWS AndrewFossAWS commented May 8, 2024 •
edited
Loading

Description of changes

Update http requests, responses and endpoint with uri to align with Smithy SRA design

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@AndrewFossAWS AndrewFossAWS force-pushed the http-request branch 2 times, most recently from f176753 to 1b21b73 Compare May 8, 2024 20:07
@AndrewFossAWS AndrewFossAWS marked this pull request as ready for review May 8, 2024 21:35
@AndrewFossAWS AndrewFossAWS mentioned this pull request May 8, 2024
Merged
@AndrewFossAWS AndrewFossAWS force-pushed the http-request branch 5 times, most recently from c411694 to e0b8ca0 Compare May 14, 2024 18:16
milesziemer
milesziemer requested changes May 14, 2024
View reviewed changes
Sources/ClientRuntime/Networking/Http/Middlewares/ContentTypeMiddleware.swift Outdated Show resolved Hide resolved
Sources/ClientRuntime/Networking/Http/Middlewares/FlexibleChecksumsRequestMiddleware.swift Outdated Show resolved Hide resolved
Sources/ClientRuntime/Networking/Http/SdkHttpRequest.swift Outdated Show resolved Hide resolved
Sources/ClientRuntime/Networking/Http/URI.swift Outdated Show resolved Hide resolved
Sources/ClientRuntime/Networking/Http/URI.swift Outdated Show resolved Hide resolved
Sources/ClientRuntime/Networking/Http/URI.swift Outdated
Comment on lines 134 to 140
return URI(scheme: self.scheme,
path: self.path,
host: self.host,
port: self.port,
query: self.query,
username: self.username,
password: self.password)
Copy link
Contributor

Choose a reason for hiding this comment

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

If my understanding is correct, URIBuilder is using URLComponents to throw errors when an invalid value is passed. But URLComponents also percent-encodes stuff when necessary - like if you did urlComponents.query = "#", it'll encode the query to be "%23". I think you could just use URLComponents instead of storing the other members.

Copy link
Contributor Author

@AndrewFossAWS AndrewFossAWS May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Removed members except path (See above comment) and port. Port is kept because I am not assigning it to URLComponents, otherwise the port number will appear in the url string which is not what we want.

Sources/ClientRuntime/Networking/Http/URI.swift Outdated Show resolved Hide resolved
Sources/ClientRuntime/Networking/Http/URI.swift Outdated Show resolved Hide resolved
Sources/ClientRuntime/Networking/Http/URI.swift Outdated Show resolved Hide resolved
Sources/ClientRuntime/Networking/Http/URI.swift Outdated Show resolved Hide resolved
@AndrewFossAWS AndrewFossAWS changed the title chore: update http requests, responses and endpoint with uri feat: update http requests, responses and endpoint with URI May 14, 2024
@AndrewFossAWS AndrewFossAWS force-pushed the http-request branch 5 times, most recently from 5a58fc9 to d329cb8 Compare May 15, 2024 02:04
milesziemer
milesziemer requested changes May 15, 2024
View reviewed changes
Sources/ClientRuntime/Networking/Http/URI.swift Outdated
Comment on lines 82 to 86
if self.path.contains("%") {
self.urlComponents.percentEncodedPath = self.path
} else {
self.urlComponents.path = self.path
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A consequence of this though is that callers of URI.path now can't know whether the URI is percent encoded or not. I guess they use the String extension you added though. But if they wanted it to be encoded and it's not, they have to do it themselves?

Sources/ClientRuntime/Networking/Http/URI.swift Outdated Show resolved Hide resolved
@AndrewFossAWS
Copy link
Contributor Author

AndrewFossAWS commented May 15, 2024 •
edited
Loading

port in SdkHttpRequest and Endpoint have been updated to be nullable.

port should remain nil if it is not set (and should not be auto-populated) This allows URL/url string to be constructed without a port. (RFC 3986 states port is not mandatory)

In aws-sdk-swift, the pre-signed reques url becomes invalid if we auto-populate the default port 443 in URL. (Not entirely sure why)

A URI.defaultPort member has also been added to provide a default port value based on Scheme/ProtocolType. (See usecase in CRTClientEngine)

@AndrewFossAWS AndrewFossAWS force-pushed the http-request branch 2 times, most recently from d3baa32 to 8553684 Compare May 17, 2024 15:45
@AndrewFossAWS AndrewFossAWS force-pushed the http-request branch 3 times, most recently from b84f52f to d61a21b Compare May 24, 2024 06:52
sichanyoo
sichanyoo requested changes May 28, 2024
View reviewed changes
Copy link
Contributor

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

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

Comments on addressing swiftlint warning & a compiler warning

Sources/ClientRuntime/Message/URI.swift Outdated Show resolved Hide resolved
Sources/ClientRuntime/Networking/Http/SdkHttpRequest.swift Show resolved Hide resolved
@sichanyoo sichanyoo self-requested a review May 28, 2024 22:27
@AndrewFossAWS
chore: update http requests, responses and endpoint with uri
f014e0e
@AndrewFossAWS
feat: add validations and builder to URI
006a81f
@AndrewFossAWS
Remove try, update uri.query to uri.queryItems and add `destina…
52dcba4 
…tion` to `RequestMessage`
@AndrewFossAWS
Update URI.query to URI.queryItems
6b8842c
@AndrewFossAWS
Refactor URI and add docs
e11c263
@AndrewFossAWS
Set SdkHttpRequest.destination to let
985036b
@AndrewFossAWS
Update URIBuilder to always return a percent encoded path in URI
96c5cae
@AndrewFossAWS
Update URIBuilder to add port to URLComponents and to be able to cl…
786b3f8 
…ear queryItems
@AndrewFossAWS
Update port to be nullable in SdkHttpRequest and Endpoint
bf58e53
@AndrewFossAWS
Add URITests
6c59920
@AndrewFossAWS
Remove try
9f3fc17
@AndrewFossAWS
Update query item related vars to func
33f0b9a
@AndrewFossAWS
Add fragment
e86d31b
@AndrewFossAWS
Address swiftlint warnings in PR
ff32e8d
@AndrewFossAWS
Add more URI tests
3de76d3
@AndrewFossAWS
Add isPercentEncoded check to username, password and host in URI
d2f6d23
@AndrewFossAWS
Set urlComponents.percentEncodedHost if os is not linux
b2525b6
@AndrewFossAWS
Add URITests test cases with encoded and unencoded reserved characters
1e2b745
@AndrewFossAWS
Address swiftlint warnings
7dfc184
@AndrewFossAWS AndrewFossAWS dismissed milesziemer’s stale review May 29, 2024 21:38

Addressed all of Miles's comments

@AndrewFossAWS AndrewFossAWS merged commit 16eb445 into main May 29, 2024
17 checks passed
@AndrewFossAWS AndrewFossAWS deleted the http-request branch May 29, 2024 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sichanyoo sichanyoo sichanyoo approved these changes

@milesziemer milesziemer milesziemer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AndrewFossAWS @milesziemer @sichanyoo