Skip to content

[Amazon.Lambda.RuntimeSupport] fix: Find header key with insensitive comparison #2094

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

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

duncanista
Copy link

@duncanista duncanista commented Jun 17, 2025

Issue

Idea for #2093

Changes

Looks up for headers without any case restrictions by creating a caseInsensitiveHeaders dictionary from the start, so lookups are O(1)

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

@boblodgett boblodgett requested a review from normj June 18, 2025 17:35
@normj normj changed the base branch from master to dev June 18, 2025 17:35
@normj normj changed the base branch from dev to master June 18, 2025 17:42
@normj
Copy link
Member

normj commented Jun 18, 2025

The change looks good but can you rebase the PR to the dev branch? The PR has picked up some commits that are in master that haven't yet been backported back to dev.

Also the PR needs a change file to take care of change log and versioning. Can you add the following content, feel free to change the change log messages, in the following file: .autover\changes\<random-guid>.json

{
  "Projects": [
    {
      "Name": "Amazon.Lambda.RuntimeSupport",
      "Type": "Patch",
      "ChangelogMessages": [
        "Fix issue making HTTP header comparisons be case insensitive"
      ]
    }
  ]
}

@duncanista
Copy link
Author

Hey @normj,

Thanks for the review, I'll rebase from the dev branch and add a change log soon!

@GarrettBeatty GarrettBeatty changed the base branch from master to dev July 7, 2025 00:04
@GarrettBeatty GarrettBeatty changed the base branch from dev to master July 7, 2025 00:04
@GarrettBeatty GarrettBeatty changed the base branch from master to dev July 7, 2025 00:07
@GarrettBeatty GarrettBeatty force-pushed the jordan.gonzalez/runtime-support/case-insensitive-header-key-lookup branch from 8af8033 to 950a7df Compare July 7, 2025 00:07
@GarrettBeatty
Copy link
Contributor

@GarrettBeatty
Copy link
Contributor

I rebased your changes on top of dev @duncanista

@duncanista
Copy link
Author

Hey @GarrettBeatty,

Thanks – sorry for taking a while, had a busy last week.

I appreciate the modifications!

@GarrettBeatty
Copy link
Contributor

running integration tests on latest commits https://github.com/aws/aws-lambda-dotnet/actions/runs/16121546157

@GarrettBeatty
Copy link
Contributor

GarrettBeatty commented Jul 8, 2025

hi @duncanista i ran the tests locally and it looks like the unit tests are failing related to the change. Amazon.Lambda.RuntimeSupport.UnitTests
image

are you able to take a look? looks like most of them have the same error message System.ArgumentNullException : Value cannot be null. (Parameter 'key')

@GarrettBeatty GarrettBeatty self-requested a review July 17, 2025 16:36
@duncanista
Copy link
Author

@GarrettBeatty updated the code to instead of checking a key on every Get, now an internal case insensitive headers dictionary is generated during the constructor.

Let me know if this is better, it should pass now. Sorry for taking so long.

On another note, do you want me to add unit tests for the RuntimeApiHeaders file? There seems to be none, I know it's kinda trivial, but still.

@GarrettBeatty
Copy link
Contributor

thanks i will rerun your changes through the test pipeline tomorrow morning

< On another note, do you want me to add unit tests for the RuntimeApiHeaders file? There seems to be none, I know it's kinda trivial, but still.

I think the other tests cases probably cover RuntimeApiHeaders indirectly, but if you would like to add more test cases feel free. I will approve it in its current state assuming the current tests pass anyway

@GarrettBeatty
Copy link
Contributor

@GarrettBeatty GarrettBeatty force-pushed the jordan.gonzalez/runtime-support/case-insensitive-header-key-lookup branch from 0951630 to 0ef4dff Compare July 29, 2025 17:46
@GarrettBeatty
Copy link
Contributor

rebased your changes on top of dev (which has a fix in the unit tests)

@GarrettBeatty
Copy link
Contributor

https://github.com/aws/aws-lambda-dotnet/actions/runs/16603533641 rerunning the tests

@GarrettBeatty
Copy link
Contributor

rerunning the tests - they are a little flaky and failing due to some unrelated issues. will try and get this merged in today

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