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.

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

api: make iOS Headers and HeadersBuilder case-insensitive #2383

Merged
merged 39 commits into from
Jun 28, 2022

Conversation

Augustyniak
Copy link
Contributor

@Augustyniak Augustyniak commented Jun 23, 2022

Description: Make the lookup of headers in HeadersBuilder and Headers case-insensitive and preserve the original casing of headers. Implemented on iOS only. Android side-changes are going to be implemented in a follow up PR. GH issue: #2390.
Risk Level: Low
Testing: Unit Tests
Docs Changes: Updated
Release Notes: Updated

Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
@Augustyniak Augustyniak force-pushed the make-headers-builder-case-insensitive branch from 59cf05f to c3c9015 Compare June 23, 2022 14:06
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
@Augustyniak Augustyniak force-pushed the make-headers-builder-case-insensitive branch from ec84cea to 15f722b Compare June 23, 2022 14:57
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
@Augustyniak Augustyniak force-pushed the make-headers-builder-case-insensitive branch from f371032 to 516a1fc Compare June 23, 2022 15:21
@Augustyniak Augustyniak force-pushed the make-headers-builder-case-insensitive branch from 6746393 to 42897f2 Compare June 23, 2022 15:24
@Augustyniak Augustyniak marked this pull request as ready for review June 23, 2022 15:40
@Augustyniak Augustyniak requested a review from goaway June 23, 2022 15:41
@Augustyniak Augustyniak changed the title Make headers builder case insensitive api: make headers builder case insensitive Jun 23, 2022
@Augustyniak Augustyniak changed the title api: make headers builder case insensitive api: make HeadersBuilder case insensitive Jun 23, 2022
@Augustyniak Augustyniak changed the title api: make HeadersBuilder case insensitive api: make HeadersBuilder case-insensitive Jun 23, 2022
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
@Augustyniak Augustyniak changed the title api: make HeadersBuilder case-insensitive api: make Headers and HeadersBuilder case-insensitive Jun 25, 2022
@Augustyniak Augustyniak requested a review from jpsim June 25, 2022 01:29
@Augustyniak
Copy link
Contributor Author

@goaway This is now ready for review. I incorporate your offline feedback with regard to making Headers case-insensitive too.

Signed-off-by: Rafal Augustyniak <[email protected]>
@Augustyniak
Copy link
Contributor Author

Augustyniak commented Jun 25, 2022

I would expect DrString to catch the issue that I found manually in 6389322. @jpsim any idea as to why this issue wasn't caught?

Signed-off-by: Rafal Augustyniak <[email protected]>
Co-authored-by: JP Simard <[email protected]>

Signed-off-by: Rafał Augustyniak <[email protected]>
Co-authored-by: JP Simard <[email protected]>

Signed-off-by: Rafał Augustyniak <[email protected]>
Co-authored-by: JP Simard <[email protected]>

Signed-off-by: Rafał Augustyniak <[email protected]>
@Augustyniak
Copy link
Contributor Author

I'm wondering if the internal APIs that currently take header dictionaries should be updated to take a Headers instance instead.

Created an issue to track this #2394.

Also, we should consider adding subscripting support to Headers for more ergonomic lookups.

Created an issue to track this #2393

Augustyniak and others added 2 commits June 28, 2022 13:20
Co-authored-by: JP Simard <[email protected]>

Signed-off-by: Rafał Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
jpsim
jpsim previously approved these changes Jun 28, 2022
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
jpsim
jpsim previously approved these changes Jun 28, 2022
Copy link
Contributor

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

🙌

Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
@@ -90,9 +90,9 @@ - (void)performRequest {
NSString *message = [NSString stringWithFormat:@"received headers with status %i", statusCode];

NSMutableString *headerMessage = [NSMutableString new];
for (NSString *name in headers.allHeaders) {
for (NSString *name in headers.caseSensitiveHeaders()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this rename. 👍

goaway
goaway previously approved these changes Jun 28, 2022
Copy link
Contributor

@goaway goaway left a comment

Choose a reason for hiding this comment

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

I like this solution. Nice work @Augustyniak.

Signed-off-by: Rafal Augustyniak <[email protected]>
@Augustyniak Augustyniak enabled auto-merge (squash) June 28, 2022 20:29
@Augustyniak Augustyniak merged commit 45ee2c3 into main Jun 28, 2022
@Augustyniak Augustyniak deleted the make-headers-builder-case-insensitive branch June 28, 2022 20:42
jpsim added a commit that referenced this pull request Jul 5, 2022
* origin/main:
  Update Envoy (#2403)
  connectivity_manager: rename/refactor from Network::Configurator (#2401)
  test: fixing up build targets to use the extension registry (#2397)
  test: Adds an integration test for SDS (#2395)
  iOS: add `forceIPv6(...)` builder option (#2396)
  api: make iOS Headers and HeadersBuilder case-insensitive (#2383)

Signed-off-by: JP Simard <[email protected]>
Augustyniak added a commit that referenced this pull request Jul 6, 2022
Description: An Android change that mimics iOS changes from #2383. Make the lookup of headers in HeadersBuilder and Headers case-insensitive and preserve the original casing of headers.
Risk Level: Low
Testing: Unit
Docs Changes: Done
Release Notes: Done
Fixes: #2390

Signed-off-by: Rafal Augustyniak <[email protected]>
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 29, 2022
Description: An Android change that mimics iOS changes from envoyproxy/envoy-mobile#2383. Make the lookup of headers in HeadersBuilder and Headers case-insensitive and preserve the original casing of headers.
Risk Level: Low
Testing: Unit
Docs Changes: Done
Release Notes: Done
Fixes: envoyproxy/envoy-mobile#2390

Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: JP Simard <[email protected]>
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.

3 participants