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

asynchronously defining api version #1009

Closed
kewinbrand opened this issue Jul 19, 2023 · 3 comments · Fixed by #1052
Closed

asynchronously defining api version #1009

kewinbrand opened this issue Jul 19, 2023 · 3 comments · Fixed by #1052

Comments

@kewinbrand
Copy link

kewinbrand commented Jul 19, 2023

hi,

would you mind making SelectVersion from the interface IApiVersionSelector async?

why?

well, we develop an ERP that releases 4 versions per year, for example 2023.1, 2023.2, 2023.3 and 2023.4.
we support the last 3 versions, so in this example would be 2023.2, 2023.3 and 2023.4
at the end of the day, we know in what version each of our customers are, by consuming an internal api

with that in mind, i would like to make an request to that internal api and fetch the correct version
only when no API version is especified

is that possible?

thank you

@commonsensesoftware
Copy link
Collaborator

Changing the signature or adding a new method to IApiVersionSelector would introduce a breaking change. The process of version selection is based on the current request and the model of known possible associated versions. That process should be synchronous. If ValueTask had existed way back when, I probably would have considered something like that.

Fear not! There are other options. While IApiVersionSelector is probably the most natural way to achieve this behavior, it can be done other ways. A few possible solutions:

  1. Read/load/cache the customer version mapping upfront. I don't know how many customers you have, but that would determine it's viability. Looking up the data every time seems time consuming yet changes infrequently (< once per day)
  2. You could use .GetAwaiter().GetResult(). If that were combined with some type of cache and ValueTask you can detect sync execution and avoid that when possible.
  3. Perform the mapping outside of IApiVersionSelector, such as middleware, where the context is intrinsically async

Consider the world's simplest middleware:

app.Use((context, next) =>
{
    var feature = context.ApiVersioningFeature();
    
    if (feature.RawRequestedApiVersions.Count == 0 &&
        context.Request.Headers.TryGetValue(HttpHeaders.From, out var from))
    {
        var version = await customers.GetDefaultApiVersion(from);

        // this can also be RequestedApiVersion if the value is already parsed
        feature.RawRequestedApiVersion = version;
    }
    
    return await next(context);
});

This is essentially what would happen later. Something to the effect of:

if (feature.RawRequestedApiVersions.Count == 0 &&
    options.AssumeDefaultVersionWhenUnspecified)
{
    var model = AggregateApiVersionModelFromCandidates(candidates);
    feature.RequestedApiVersion = selector.SelectApiVersion(context.Requested, model);
}

In your case, you don't have to worry about AssumeDefaultVersionWhenUnspecified because you handle it yourself before it reaches that point in API Versioning.

Adding an async method isn't completely off the table, but I likely wouldn't add it until the next major version change. That would coincide with the .NET 8 release (~Nov).

@commonsensesoftware
Copy link
Collaborator

I'm currently considering whether to add this capability. Even as additive, it will technically be a breaking change. The support for default interface implementations will help reduce the impact. I'm thinking of something like:

public interface IApiVersionSelector
{
    ApiVersion SelectVersion( HttpRequest request, ApiVersionModel model );

    ValueTask<ApiVersion> SelectVersionAsync(
        HttpRequest request,
        ApiVersionModel model,
        CancellationToken cancellationToken ) =>
        ValueTask.FromResult( SelectVersion( request, model ) );
}

What I would like to understand better is what will happen when you cannot resolve anything from the HttpRequest? By contract, you are not allowed to return null. Based on your description, your use case is similar to the original thinking of why anyone would do this or need the current request in the first place. Be it by design or for historical reasons, I can see how clients might implicitly be bound to a varying number of API versions. The current design aligns to the capabilities of the Configuration and Options APIs, neither of which support asynchronous paths. This means that loading a mapping from Configuration or Options is intrinsically synchronous.

Aside from just changing the signature, I wonder what the thoughts are of implementing two method variants of the interface are? All of the current implementations are synchronous and there are numerous scenarios where the current HTTP context may not be available. I'm debating refactoring the parameter to be nullable or provide a default, empty HTTP request. In either case, the request is useless for the purposes of resolving an API version. A clear and obvious use case for such a scenario would be in documentation (e.g. OpenAPI). The default version could vary by API, which can only be known through the IApiVersionSelector, but it isn't used that way today. Using IApiVersionSelector in that situation makes perfect sense, but what are the expectations by extenders when there is no usable HTTP request?

What I would expect an implementer to do given no useful information in the HTTP request is one of the following:

  • Choose the highest supported version in the ApiVersionModel
  • Choose the lowest supported version in the ApiVersionModel
  • Always use ApiVersioningOptions.DefaultApiVersion
  • Always use a constant ApiVersion

As you may recognize, these are effectively the policies the out-of-the-box IApiVersionSelector implementations provide.

The other downside of this approach is that there is no guarantee that this will always be the behavior. There could be a scenario where it logically makes sense or things are intrinsically synchronous. If the expected information is in the HTTP request, the this should result in a blocking call with .GetAwaiter().GetResult() in the sync-over-async path. Naturally, the library would move to use async everywhere it can, but there will be some cases where that is not possible (ex: documentation). If that were to happen, hopefully very infrequently, is that a problem? Is providing two different implementations painful?

I know there are people out there that have written custom IApiVersionSelector implementations, but I don't get a lot of comments or feedback from them. As someone legitimately trying to do this, I'd like to collect some more feedback before committing to the change.

@commonsensesoftware
Copy link
Collaborator

8.0.0 has been released and includes this support a la SelectVersionAsync as shown above. Be aware, it is not possible to completely remove sync execution. It definitely affects OData support and there is no way to refactor to make it async. If you're not using OData, then it's probably not an issue. The sync path will often be used for the API Explorer, but there is no real request in those scenarios. The recommended process there is to fall back to a sync approach using the ApiVersionModel when there is no useful information in the HttpRequest or you can use a separate IApiVersionSelector for the API Explorer that is solely used for version selection when exploring APIs.

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

Successfully merging a pull request may close this issue.

2 participants