Refactor Request Validation #391
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fabianfett
added
the
semver/patch
fabianfett
force-pushed
the
ff-refactor-request-validation
branch
from
6e1fa34
to
0d47360
Compare
Lukasa
reviewed
Jul 7, 2021
| struct RequestFramingMetadata { | ||
| enum Body { | ||
| struct RequestFramingMetadata: Equatable { | ||
| enum Body: Equatable { |
There was a problem hiding this comment.
Make these Hashable: Equatable by itself is rarely sensible.
| // This assert can go away when (if ever!) the above `if` correctly handles other HTTP versions. For example | ||
| // in HTTP/1.0, we need to treat the absence of a 'connection: keep-alive' as a close too. | ||
| assert(head.version == HTTPVersion(major: 1, minor: 1), | ||
| "Sending a request in HTTP version \(head.version) which is unsupported by the above `if`") |
There was a problem hiding this comment.
I don't think this assert is meaningfully enforcing here, is it? You (rightly) kept the original, but this one seems superfluous.
There was a problem hiding this comment.
Ok, I still don't think this assertion is useful. We literally set the HTTP version to a hardcoded constant just above.
Co-authored-by: Cory Benfield <[email protected]>
fabianfett
force-pushed
the
ff-refactor-request-validation
branch
from
6a6fd09
to
f9f1050
Compare
Lukasa
approved these changes
Jul 7, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Currently the request validation (mainly checking for correct headers is sprinkled around). This makes reuse in new code hard.
Changes
TaskHandlerinto new methodHTTPClient.Request.createRequestHeadthat creates anHTTPRequestHeadand matchingRequestFramingMetadatarequestFramingMetadatatoHTTPExecutingRequestrequestFramingMetadatatoRequestBag