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

[Versioned Clients][API Notifications] Fails to read new versions when available #1070

Closed
1 task done
chamweer opened this issue Feb 12, 2024 · 2 comments · Fixed by #1073
Closed
1 task done

[Versioned Clients][API Notifications] Fails to read new versions when available #1070

chamweer opened this issue Feb 12, 2024 · 2 comments · Fixed by #1073
Assignees
Labels

Comments

@chamweer
Copy link

chamweer commented Feb 12, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

As described in API Notifications section, the default ApiVersionHandlerLogger<ApiVersionHandler> is expected to log an Information message when a new version is available; however, the ApiVersionParser fails to parse the api-supported-versions header's comma separated values generated by the DefaultApiVersionReporter and as a result no new version will be reported.

The problem here is that the client side's ApiVersionEnumerator expects the version values to be in multiple header keys (Headers: .... [api-supported-versions, 0.9], [api-supported-versions, 1.0] ...) whereas the header, generated through DefaultApiVersionReporter in the server application, contains all supported versions in a single header (Headers: .... [api-supported-versions, 0.9, 1.0] ...).

The following is a sample response generated for a GET request:

❯ curl http://localhost:5282/api/v0.9/products/

StatusCode        : 200
StatusDescription : OK
Content           : .....
RawContent        : HTTP/1.1 200 OK
                    Transfer-Encoding: chunked
                    api-supported-versions: 0.9, 1.0
                    api-deprecated-versions: 0.8
                    Sunset: Mon, 31 Mar 2025 23:00:00 GMT
                    Link: <https://docs.api.com/policy.html?api-version=...
Forms             : {}
Headers           : {[Transfer-Encoding, chunked], [api-supported-versions, 0.9, 1.0], [api-deprecated-versions, 0.8], [Sunset, Mon, 31 Mar 2025 23:00:00 GMT]...}
...

Expected Behavior

New version details should be notified by default through the logger when available.

Steps To Reproduce

Sample Repo to reproduce the issue: https://github.com/chamweer/api-version-notification-bug

This repository contains 3 versioned endpoints representing 3 different use cases:

  • V0.8: Deprecated
  • V0.9: Live and sunset policy is set.
  • V1.0: New version.

Steps to reproduce the issue:

  1. Start the Api.Versioning.Notification.Bug.Api project using dotnet run command.
  2. Run the Api.Versioning.Notification.Bug.Client console app and monitor the logs to see whether you can see any information logs reporting the available versions.

Exceptions (if any)

.NET Version

8.0.101

Anything else?

Asp.Versioning.Http.Client 8.0.0

Console Logs:

info: System.Net.Http.HttpClient.V0.8Client.LogicalHandler[100]
      Start processing HTTP request GET http://localhost:5128/api/v%7Bversion%7D/products
info: System.Net.Http.HttpClient.V0.8Client.ClientHandler[100]
      Sending HTTP request GET http://localhost:5128/api/v0.8/products
info: System.Net.Http.HttpClient.V0.8Client.ClientHandler[101]
      Received HTTP response headers after 87.3609ms - 200
warn: Asp.Versioning.Http.ApiVersionHandler[1]
      API version 0.8 for http://localhost:5128/api/v0.8/products has been deprecated and will sunset on <unspecified>. Additional information:
info: System.Net.Http.HttpClient.V0.8Client.LogicalHandler[101]
      End processing HTTP request after 133.8124ms - 200
info: System.Net.Http.HttpClient.V0.9Client.LogicalHandler[100]
      Start processing HTTP request GET http://localhost:5128/api/v%7Bversion%7D/products
info: System.Net.Http.HttpClient.V0.9Client.ClientHandler[100]
      Sending HTTP request GET http://localhost:5128/api/v0.9/products
info: System.Net.Http.HttpClient.V0.9Client.ClientHandler[101]
      Received HTTP response headers after 27.1239ms - 200
info: System.Net.Http.HttpClient.V0.9Client.LogicalHandler[101]
      End processing HTTP request after 40.1566ms - 200
info: System.Net.Http.HttpClient.V1Client.LogicalHandler[100]
      Start processing HTTP request GET http://localhost:5128/api/v%7Bversion%7D/products
info: System.Net.Http.HttpClient.V1Client.ClientHandler[100]
      Sending HTTP request GET http://localhost:5128/api/v1.0/products
info: System.Net.Http.HttpClient.V1Client.ClientHandler[101]
      Received HTTP response headers after 5.5906ms - 200
info: System.Net.Http.HttpClient.V1Client.LogicalHandler[101]
      End processing HTTP request after 9.453ms - 200
@commonsensesoftware
Copy link
Collaborator

I see what happened. I'll consider this a bug.

Honestly, this feels like a bug in System.Net.Http, but I doubt I'll move the mountain much if I file an issue. I'm not sure why, but I'm willing to bet if I go down in the implementation far enough, I'll find that comma-separated values are only handled in some cases (🤷🏽‍♂️ no idea why).

The ApiVersionEnumerator is actually doing the right thing, but HttpHeaders.TryGetValues is not. By spec, any multi-value HTTP header (most are) can be listed multiple times:

api-supported-versions: 0.9
api-supported-versions: 1.0

or collapsed into a single header:

api-supported-versions: 0.9, 1.0

provided there is no change in meaning. If the headers are listed multiple times, then HttpHeaders.GetValues and HttpHeaders.TryGetValues will do the correct thing. If the header values are collapsed into one, then those methods are retrieving a single string value without splitting them. This is why ApiVersionEnumerator isn't reporting the correct results. In fact, multiple, multi-value headers are even allowed:

api-supported-versions: 0.9, 1.0
api-supported-versions: 2.0, 2.1

There are various reasons why the server might do that. In reviewing the tests, I don't have any scenarios that cover these cases. The solution is pretty straight forward. I'll work on getting a fix out.

Thanks for reporting it and providing a comprehensive repro. It's very helpful.

@chamweer
Copy link
Author

Glad I could help identify the issue. As you mentioned, the current implementation of the HttpHeaders extension methods does not support splitting comma-separated values, leading to invalid behaviour.

Thanks again for acknowledging the report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants