aws-lambda-dotnet icon indicating copy to clipboard operation
aws-lambda-dotnet copied to clipboard

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

Open duncanista opened this issue 9 months ago • 1 comments

Issue

Idea for #2093

Changes

Looks up for headers without any case restrictions.

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

duncanista avatar Jun 17 '25 20:06 duncanista

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"
      ]
    }
  ]
}

normj avatar Jun 18 '25 17:06 normj

Hey @normj,

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

duncanista avatar Jun 23 '25 18:06 duncanista

https://github.com/aws/aws-lambda-dotnet/actions/runs/16104625097

GarrettBeatty avatar Jul 07 '25 00:07 GarrettBeatty

I rebased your changes on top of dev @duncanista

GarrettBeatty avatar Jul 07 '25 00:07 GarrettBeatty

Hey @GarrettBeatty,

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

I appreciate the modifications!

duncanista avatar Jul 07 '25 04:07 duncanista

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

GarrettBeatty avatar Jul 07 '25 15:07 GarrettBeatty

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 avatar Jul 08 '25 14:07 GarrettBeatty

@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.

duncanista avatar Jul 28 '25 23:07 duncanista

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 avatar Jul 28 '25 23:07 GarrettBeatty

https://github.com/aws/aws-lambda-dotnet/actions/runs/16602456157 running tests

GarrettBeatty avatar Jul 29 '25 16:07 GarrettBeatty

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

GarrettBeatty avatar Jul 29 '25 17:07 GarrettBeatty

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

GarrettBeatty avatar Jul 29 '25 17:07 GarrettBeatty

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

GarrettBeatty avatar Jul 31 '25 14:07 GarrettBeatty