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

fix: multi value header handling #1888

Closed
wants to merge 5 commits into from
Closed

fix: multi value header handling #1888

wants to merge 5 commits into from

Conversation

sdelamo
Copy link
Contributor

@sdelamo sdelamo commented Sep 20, 2023

Close #1861

Multi-header values should use a comma:

Multiple message-header fields with the same field-name MAY be
present in a message if and only if the entire field-value for that
header field is defined as a comma-separated list [i.e., #(values)].
It MUST be possible to combine the multiple header fields into one
"field-name: field-value" pair, without changing the semantics of the
message, by appending each subsequent field-value to the first, each
separated by a comma.

But there are headers which have a , without using it to designate a multi-value. E.g. Date. Thus, a mess 🤷🏼‍♂️

@sdelamo sdelamo added the type: bug Something isn't working label Sep 20, 2023
@sdelamo sdelamo requested a review from timyates September 20, 2023 10:41
@sdelamo
Copy link
Contributor Author

sdelamo commented Sep 20, 2023

ping @driverpt

import spock.lang.Specification

@MicronautTest
class MapCollapseUtilsSpec extends Specification {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The examples Foo and Key in this test are taken directly from the AWS Lambda Console. I did request and logged them to see how they are represented in the Lambda Event Objects.

@@ -31,6 +35,8 @@
@Internal
public final class MapCollapseUtils {

private static final List<String> HEADERS_ALLOWING_COMMAS = Arrays.asList(HttpHeaders.DATE, HttpHeaders.USER_AGENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how Netty handles this @yawkat do you know?

Copy link
Contributor Author

@sdelamo sdelamo Sep 20, 2023

Choose a reason for hiding this comment

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

Should this be configurable?

but it should be configurable at not in AWS right?, should this configuration be in servlet-core?

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

81.6% 81.6% Coverage
2.4% 2.4% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@sdelamo sdelamo self-assigned this Sep 22, 2023
@sdelamo sdelamo closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug] API Gateway Payload V2: Multiple Headers with same name come as one
2 participants