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

Incomplete response when incorrect version requested #876

Closed
1 task done
bordecal opened this issue Sep 12, 2022 · 8 comments
Closed
1 task done

Incomplete response when incorrect version requested #876

bordecal opened this issue Sep 12, 2022 · 8 comments
Assignees

Comments

@bordecal
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

We use this library for versioning of a .NET 6.0 REST API for which supporting forced update of client application is a key requirement. Unfortunately, after update to v6.0.0, it seems that the information that would help the client determine that there is a version mismatch is no longer exposed in the HTTP response.

The change in status code from 400 to 404 is a minor inconvenience that we can work around. However, when an incorrect API version is requested, there does not seem to be any information returned in the response that would help identify the problem:

  • The response does not include problem details specifying that the cause of the 404 was a request for an unsupported API version.
  • The api-supported-versions is absent from the response even if ApiVersioningOptions.ReportApiVersions has been set to true.

This behaviour can be easily reproduced using the BasicExample at https://github.com/dotnet/aspnet-api-versioning/tree/main/examples/AspNetCore/WebApi/BasicExample:

Expected Behavior

When an unsupported API version is requested, a problem details response identifying the problem should be returned. If supported version reporting has been enabled via ApiVersioningOptions.ReportApiVersions, an api-supported-versions header should be included in the response.

Steps To Reproduce

Run the BasicExample from this repo and request an unsupported API version (e.g.: https://localhost:5001/api/values?api-version=5.0).

Exceptions (if any)

No response

.NET Version

6.0.400

Anything else?

We see 400 responses with meaningful problem details when using v5.0.0 of this library with .NET 6.0, so this does not appear to be a .NET 6.0 issue.

@commonsensesoftware
Copy link
Collaborator

The short answer, which you're probably not going to like, is that this is (now) by design. I really try to avoid breaking changes - especially behavioral changes, but sometimes it's unavoidable. This behavioral, breaking change has been called out in the roadmap and release notes since the first previews.

As you pointed out, changing from 400 to 404 is a fairly trivial change, but it's breaking nevertheless. If your API itself was updated, then this is a completely allowed change. The thing that kept me up at night was knowing that some people may update the library (e.g. implementation) without realizing this change nor bumping the version on their API. This can obviously break existing clients in unexpected ways, which is bad. Unfortunately, I don't think there is a way for 5.x and 6.0+ to live side-by-side with the behaviors. For those that must retain the older behavior, they likely need different deployments or need to sunset their old versions before putting up a new one; neither of which are ideal.

The primary reason that the functionality is not preserved is simply due to implementation oversights in ASP.NET Core itself. This is being tracked in dotnet/aspnetcore#32957, but it will not land until .NET 7.0. I've even commented further on the issue. I'm not sure there is a way to address the issue. If you have an idea or proposal as to how it might be mitigated, I'm open to it.

TL;DR

This history of how and why this happened is a bit long-winded. It started with #744. That issue uncovered a bug, which I thought was a bug in the ASP.NET Core routing system, which I reported in dotnet/aspnetcore#33865. After further analysis, I determined that it was not a bug, but rather a misunderstanding by me as to how the routing system works. Previously, API Verisoning's role in routing was pretty simple - allow duplicate routes to match and disambiguate by API version.

There have always been edge cases, namely 400, 404, 405, and 415. Achieving 404, 405, and 415 previously required reimplementing or otherwise trying to hoist the logic from the routing system at an earlier phase to a later phase. I never really liked that because a long-standing goal was to not change the expectations of the routing system. To correct the errors of my ways, this required a significant change to how API Versioning integrates with routing. Under the hood, ASP.NET Core builds a directed graph of nodes for each endpoint. API Versioning now builds versioned nodes that are parents of other normal nodes. This affords the behavior that once a matching API version is selected, you'll get the out-of-the-box behavior of 404, 405, and 415 when the rest of the graph is evaluated.

There are a couple of consequences to this change. First, there are some cases where you'll now get 404 instead of 400. This occurs because all possible candidates are eliminated very early in the pipeline and when there are none left, ASP.NET Core treats that as 404. The next issue is due to dotnet/aspnetcore#32957 as there is no hook or other opportunity for API Versioning to look for associated 404, 405, and 415 cases and add or expand ProblemDetails. Every scenario API Versioning is able to step in to add ProblemDetails or report API versions by via a header, it does so.

The other 400 scenarios are still supported, but 404 will typically be the new status code for an unmatched endpoint. That is arguably correct and has always been a debate among some. I thought about trying to make that configurable, but there simply no way to really make that work. When API Versioning knows ahead of time there are possible candidates, but there are no matches, you should receive 404 with ProblemDetails and expected headers. When it doesn't, we're at the mercy of what ASP.NET Core exposes.

I've always felt it was interesting and useful for a client to know when a server supports an API, but not the version requested; that's how the 400 came to be in the first place. My thinking has evolved a little bit and I'm OK with 404 as that does express the correct meaning. While 400 has been nice, it's been pretty lack-luster on the client side, even with the HTTP headers. Starting in 6.0, there is now a client library that addresses several common tasks:

  1. Adding the configured API version to requests
  2. Detecting a deprecated API version is being used
  3. Detecting an API version is going to sunset (e.g. go away) - deprecated or not

2 and 3 can wire up to ILogger<T> to report this information into client telemetry. Client's can then use that information to proactively detect and notify developers to update their code before they ever get into a situation where they'd receive a 400 or 404. In order to take advantage of 3, you'd need to also define a Sunset Policy, which is a new capability in 6.0. This allows a server to report the date an API version will be gone for good as well as provide links to API versioning policies. Non-.NET clients can leverage that information too as long as they know to look for it.

@bordecal
Copy link
Author

Unfortunately, sunsetting is not an approach that would work in our particular scenario since there is no guarantee that any given client would attempt to connect to the REST API during a sunsetting period.

As I mentioned above, the 400 vs 404 result isn't really a big issue for us as long as there's some way to detect the version incompatibility in the response that is sent when a client requests an unsupported version. Are there plans for making that possible again, or should we be looking for other ways to support that going forward? (Version 5.0 is working fine for us at the moment, so we could just stick with it for now.)

@commonsensesoftware
Copy link
Collaborator

If dotnet/aspnetcore#32957 does the right thing and enables the necessary hooks, then - yes - the functionality will return in the 7.0 timeframe. Currently, there is simply no way to enhance or participate in what ASP.NET Core does for many of the 404, 405, and/or 415 responses.

@bordecal
Copy link
Author

Fingers crossed in that case -- thanks!

@commonsensesoftware
Copy link
Collaborator

In revisiting this for .NET 7, I think there may be some possibility to improve things. In previous versions, it was assumed that candidates could simply be eliminated by API version. That is ultimately how the old 400 behavior worked. API versions could be reported from the candidate[s]. Unfortunately, that was problematic for 405 and broken for 406 or 415.

Starting in 6.0 the graph for endpoints built by the routing system has an API version node (the acceptance tests output a link you can use to view the graph in a browser). This allows making routing decisions earlier, which improves performance and solves the other edge cases, such as 405, by allowing them to continue to work as they always have. As you've seen, this alters the behavior which causes 404 in a number of cases instead of 400. Since we don't have a candidate, there's really nothing that can be accurately reported. We need candidates to know what is and isn't possible.

One thing that I did notice, however, is that the 404 could include ProblemDetails that indicates the API version is not implemented - anywhere. It's not possible to know which mapped API versions are supported or deprecated for the incoming request at that part of the pipeline. It's kind of like a Bloom Filter in the sense that the request is "possibly in the set" or "definitely not in the set" if there isn't any corresponding API version from this point forward.

In short, this could work:

GET /api/values?api-version=5.0 HTTP/2
Host: localhost:5000
HTTP/2 404 Not Found
Content-Type: application/problem+json
Content-Length: 420

{
 "type": "https://docs.api-versioning.org/problems#unsupported",
 "title": "Unsupported API version",
 "status": 404,
 "detail": "The HTTP resource that matches the request URI 'https://localhost:5001/api/values' does not support the API version '5.0'."
}

As opposed to:

GET /api/fake?api-version=1.0 HTTP/2
Host: localhost:5000
HTTP/2 404 Not Found

Which would be something that truly doesn't exist.

I have to do some more research, but it might - just might - be possible to report API versions if I can capture supported and deprecated separately. In theory, if there's no match, we've already reached /api/values and know the separate buckets, we can report api-supported-versions and api-deprecated-versions when configured. If there is any overlap in the underlying jump table that could report the wrong info. For example, if /orders and /orders/items are considered different APIs and are in the same table, there's no way to know that. There's also a chance that never happens with how the tree is built. 🤞🏽 This is way down in the bowels of the routing system.

@bordecal
Copy link
Author

That's good news! 🤞

@commonsensesoftware
Copy link
Collaborator

My preliminary research shows that I can collate supported and deprecated API versions as they are built up in the routing system. The metadata for each endpoint has already been computed so I do know what has been defined. The issue is that the only thing I have to pivot on from a policy standpoint is the incoming API version. I'd have to rely on the way the routing system builds the route tree based on templates to collate and report the appropriate versions. Honestly, it's best effort. As far as I can tell, it does seem to produce the correct and expected behavior, but I can't be 100% certain and there may be edge cases that I've missed.

I don't have control over how the routing system builds the tree from route templates and even if I could change it, I'm not sure I want to. It's very complex. The case I worry about is logically different APIs that overlap by template, but are technically different APIs. For example, is order/123 and order/123/items the same or different APIs? This is exactly why the collation logic is based on name and not route template. I've tried a few contrived variations and I haven't been able to produce a result that would give the wrong answer, but I also don't see how I can guarantee the right answer either.

I think this is a reasonable compromise. My investigation suggests that getting an understated or overstated set of supported and deprecated API versions is unlikely with this approach.

This has certainly been a challenge. I actually didn't think that many people liked this in the error case. There have certainly been more voiced opinions on the opposite side (voting for 404). In conjunction with this proposed change, I think I will add a new configuration option that follow the desired status code through. This has been a long-standing limitation. 400, 404, and even 405 (when possible) have always returned the same error message, but with a different status code. While there are ways to hook into the pipeline to change it, it's not all that straight forward to know when and where to do that. I'm also a little hesitant to change the response status code a minor or patch version, even if it is the right thing to do. That would silently break people that are expecting the current behavior. I kind of feel this addition would have to default to 404 for 6.x, but be changeable to 400 (or even 501 [Not Implemented]). 7.0 is just around the corner and it would be safer to return to 400 then. I could also start a discussion to get community opinion, but I'm not sure there will be a timely set of input. In the meantime, people like yourself would be waiting. ☹️

@bordecal
Copy link
Author

Thanks! In the long term, I'm not overly bothered by the exact response status code as long as there's some way for the client to infer that it's trying to use a version that isn't supported. A 404 that includes the supported versions header would be sufficient. However, having the option to use the alternate code(s) could indeed make the upgrade path easier since existing clients would expect the old codes.

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

No branches or pull requests

2 participants