Skip to content

Behavior should be the same when ?api-version is missing, as when it is incorrect. #61

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

Closed
colemickens opened this issue Nov 25, 2016 · 5 comments

Comments

@colemickens
Copy link

Setup:

I have an InfoController that looks like this:

using System;
using System.Security.Cryptography;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Logging;
using Api.ContractsV1;

namespace Api.Controllers {
    [ApiVersion("1.0")]
    [Route("/info")]
    public class InfoController {
        public InfoController(){}

        [HttpGet()]
        public InfoContract Index() {
            return new InfoContract{
                Hostname = Environment.MachineName,
            };
        }
    }
}

Here's observed queries, through nginx:

127.0.0.1 - - [25/Nov/2016:21:08:16 +0000] "GET /info HTTP/1.1" 404 0 "-" "curl/7.51.0"
127.0.0.1 - - [25/Nov/2016:21:09:04 +0000] "GET /info?api-version=1.0 HTTP/1.1" 200 31 "-" "curl/7.51.0"
127.0.0.1 - - [25/Nov/2016:21:09:07 +0000] "GET /info?api-version=2.0 HTTP/1.1" 400 192 "-" "curl/7.51.0"

The behavior for missing api-version query parameter should be the same as if a bad version is specified. I expected the first to result in a 400 as well, rather than 404.

@commonsensesoftware
Copy link
Collaborator

Once you opt into API versioning, all API routes are explicitly versioned. This means a client cannot request a resource without explicitly providing an API version. This is why you receive a 404 when no version is specified. Unless you have existing services that didn't require versioning, I always recommend that explicit versioning be specified by clients. That provides the most reliable behavior for clients (IMHO).

In order to make your scenario work, you need to allow matching a default API version when a client doesn't specify anything. This is achieved in the setup with:

service.AddApiVersioning( options => options.AssumeDefaultVersionWhenUnspecified = true );

The choice of the default version can also be configured and is up to you. The default configured value is 1.0. There are several topics in the wiki that can guide you through some of these more advanced behaviors. I hope that helps.

@colemickens
Copy link
Author

I understand that. I want the api-version to be required. But returning a 404 when it's absent is inconsistent, and per my last reading of the doc, not in alignment with the MS Rest API Guidelines.

@colemickens
Copy link
Author

Well, the doc I read was fairly different and had a lot more verbiage about specific versioning behavior. Not sure if the internal one is significantly different or if it's evolved that much since I read it last.

Anyway, when I'd implemented something like this internally, we failed requests missing the header/query-param as 400s as well. I prefer that behavior. It seems more semantically consistent and less confusing to callers.

@commonsensesoftware
Copy link
Collaborator

Ah ... I see. You may be right. Getting the behavior right for could match and could never match is tricky. In retrospect, I believe this should be 400 since it's possible to match the route, but no route is matched. The cause in this case is that the client didn't provide a version. Basically, it should be the same behavior as specifying 2.0, which is also possible, but is unmatched because there's no such implemented version.

I'll investigate further and promote this to bug after I've triaged it.

@commonsensesoftware
Copy link
Collaborator

Confirmed. This is a bug. It's a little surprising that I missed such an obvious case, but so is the way of software. I'm tracking the issue with a detailed explanation in #62. I should have the fix published soon. Thanks for reporting this.

commonsensesoftware pushed a commit that referenced this issue Nov 27, 2016
…n for a valid resource is not specified. Resolves #61 and fixes #62.
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