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

Bug fix: Make get_route_info() case insensitive #60

Merged
merged 6 commits into from
Aug 1, 2024

Conversation

mynhardtburger
Copy link
Contributor

@mynhardtburger mynhardtburger commented Aug 1, 2024

This PR makes TGISBackend.get_route_info()'s operation case insensitive.

Both gRPC and HTTP defines their metadatum/headers as case insensitive. This requires any lookup of keys to be done in a case insensitive manner.

Additionally fixed the test_get_route_info test:

  • The expected to raise error case was incorrectly being identified
  • updated TestServicerContext to inherit from grpc.ServicesContext. This was required because TGISBackend.get_route_info() test if the servicer context is of grpc.ServicerContext before attempting to access the metadata.
  • Set encoding to latin-1 as per http spec and what starlette expects.

Note:
Depending on how thefastapi.Request object (actually a starlette.requests.Request object) is created, it might not instantiate its headers in a case insensitive manner. If the starlette.datastructures.Headers is created via the scope= parameter the header keys still remain case sensitive. This is why Headers.get() is not reliable and a custom TGISBackend._request_header_get() was necessary.

Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
…get_route_info will extract the route info

Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
…jects as well

Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Signed-off-by: Mynhardt Burger <Mynhardt.Burger@ibm.com>
Copy link
Contributor

@gabe-l-hart gabe-l-hart 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 this improvement Mynhardt!

Copy link
Collaborator

@evaline-ju evaline-ju left a comment

Choose a reason for hiding this comment

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

LGTM!

@evaline-ju evaline-ju merged commit 057b5fb into caikit:main Aug 1, 2024
6 checks passed
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