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

Issue #81: Fix case insensitivity bug #103

Conversation

ichbinsteffen
Copy link
Contributor

@ichbinsteffen ichbinsteffen commented Jun 29, 2023

In Issue #81 @seruminar described why URIs don't contain query parameters when the models they are derived from have pascal cased property names.

Here is my short take on the problem:
- When the URL template contains query parameters with upper case first letters and the QueryParameters contains them in lowerCase (because they are added using x.Name.ToFirstCharacterLowerCase()) then the names are not matched and the resulting URI does not contain the parameters.
- When in the URI getter the parameters are actually set using uriTemplate.SetParameter then in the dictionary the casing does not match, and therefore they are silently failing to be set.
- The UriTemplate ctor contains the optional setting (caseInsensitiveParameterNames) to use a case insensitive dictionary. IMHO this is the best solution because then it does not matter what kind of casing patterns the API models have.

The alternative solution to this would be to change AddQueryParameters where
x.Name.ToFirstCharacterLowerCase() is called for each parameter. Lowercasing the first char is what causes this bug in the first place. The strings in the dictionary would naturally match the API model from which they are collected.
What do you think would be the better fix?

Here a little demo of the problem:
Before:
grafik
After:
grafik

    - When the URL template contains query parameters with upper case first letters and the QueryParameters contains them in lowerCase (because they are added using x.Name.ToFirstCharacterLowerCase()) then the names are not matched and the resulting URI does not contain the parameters.
    - When in the URI getter the parameters are actually set using uriTemplate.SetParameter then in the dictionary the casing does not match, and therefore they are silently failing to be set.
    - The UriTemplate ctor contains the optional setting (caseInsensitiveParameterNames) to use a case insensitive dictionary. IMHO this is the best solution because then it does not matter what kind of casing patterns the API models have.
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @ichbinsteffen

Would you please mind adding:

  • an entry to the changelog (patch, date to Monday)
  • a unit test
  • bump the version in the csproj

@ichbinsteffen
Copy link
Contributor Author

@ichbinsteffen please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
[...]

Currently my legal department is looking into this. They will let me know soon if I can give a license acceptance.

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. @andrueastman for final review and merge

@baywet baywet enabled auto-merge July 3, 2023 13:28
@baywet
Copy link
Member

baywet commented Jul 3, 2023

@andrueastman I just realized the CLA was not a required check on this repo main branch policy. I think this is due to the recent migration. Can you double check the other Kiota dotnet repos please?

@andrueastman
Copy link
Member

@andrueastman I just realized the CLA was not a required check on this repo main branch policy. I think this is due to the recent migration. Can you double check the other Kiota dotnet repos please?

Thanks for pointing this @baywet. Will look into this and resolve.

@ichbinsteffen
Copy link
Contributor Author

@andrueastman I just realized the CLA was not a required check on this repo main branch policy. I think this is due to the recent migration. Can you double check the other Kiota dotnet repos please?

Nonetheless, our legal department allowed me to sign it. So I'll sign it in a dedicated comment.

@ichbinsteffen
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Init Innovation In Traffic Systems SE"

@baywet baywet merged commit 8e0189e into microsoft:main Jul 4, 2023
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