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

Issue when using AddApiExplorer and JsonTranscoding #1109

Open
1 task done
mcastagna-sa opened this issue Sep 10, 2024 · 3 comments
Open
1 task done

Issue when using AddApiExplorer and JsonTranscoding #1109

mcastagna-sa opened this issue Sep 10, 2024 · 3 comments
Assignees

Comments

@mcastagna-sa
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Hey guys, i'm having a problem with a project i'm working on and i wanted to see if i'm missing anything or maybe i've stumbled on a bug.

I have created a minimal project where i'm able to reproduce the behaviour, I'm goint to try to guide you pasting snippets of code here.

When i run the program, and try to hit the endpoint v2 with postman everything goes as expected.

image

But, instead, if i call v1, which should be responded by the jsontranscode, i get 404.

image

I've tracked this down to the call of AddApiExplorer. If i remove that call i'm able to get the v1 as i should.

What do you think about this? Is this the expected behaviour? Am i doing something wrong? Is this a bug? If it is, is a bug in the jsontranscoding or asp.versioning library?

Thank you so much for your time and sorry the lenght of this post. Let me know if you need anything else that could help you answering this.

Expected Behavior

Endpoint is found and executed.

Steps To Reproduce

Program.cs

static void Main(string[] args)
 {
     var app = CreateHostBuilder(args).Build();

     app.UseRouting();

     app.MapControllers();

     app.MapGrpcService<GreeterService>();

     app.Run();
 }
  
 private static WebApplicationBuilder CreateHostBuilder(string[] args)
 {
     var builder = WebApplication.CreateBuilder(args);

     builder
         .WebHost.ConfigureKestrel((context, options) =>
         {
             options.ListenLocalhost(8080);
         });

     builder.Services.AddControllers();

     builder.Services.AddApiVersioning(options =>
          {
              options.ReportApiVersions = true;
          })
          .AddApiExplorer(options =>
          {
              options.GroupNameFormat = "'v'VVV";
              options.SubstituteApiVersionInUrl = true;                     
          });

     builder.Services
         .AddGrpc()
         .AddJsonTranscoding();

     return builder;
 }

V2Controller.cs

[ApiController]
[ApiVersion("2.0")]
[Produces("application/json")]
[Route("api/v{version:apiVersion}/controller/{param}")]
public class V2Controller : Controller
{
    [HttpGet]
    [Route("route")]
    public async Task<IActionResult> GetEndpoint()
    {
        return Ok("ok get v2");
    }
}

And greet.proto (This is the wiki example as you can see, just added the route i need to demostrate my point)

syntax = "proto3";

option csharp_namespace = "TestVersioningGRPC.Proto";
import "google/api/annotations.proto";

package greet;

service Greeter {
  rpc SayHello (HelloRequest) returns (HelloReply) {
    option (google.api.http) = {        
      get: "/api/v1/controller/{name}/route"
    };
  }
}

// The request message containing the user's name.
message HelloRequest {
  string name = 1;
}

message HelloReply {
  string message = 1;
}

Exceptions (if any)

No response

.NET Version

8.0.304

Anything else?

No response

@commonsensesoftware
Copy link
Collaborator

I would be very surprised that anything about AddApiExplorer caused this behavior. It doesn't change any routing behavior whatsoever. It's purely discovery. I can't say I have much firsthand experience with JSON transcoding with gRPC, but there are some incongruencies I see that don't make a lot of sense.

  1. Why use Controller (usually full MVC with views) instead of ControllerBase
  2. Why are there two different routes to GetEndpoint?
  3. Why does V2 have a route parameters with no parameter?
    a. I don't even see how this can work since the parameter is required

I would have expected 2.0 to look something like:

[ApiController]
[ApiVersion(2.0)]
[Produces("application/json")]
[Route("api/v{version:apiVersion}/controller")]
public class V2Controller : Controller
{
    [HttpGet("{param}/route""]
    public IActionResult GetEndpoint(string param) => Ok("ok get v2");
}

You can include ApiVersion as parameter if you don't want to hardcode v2

I'm not sure I understand exactly how ProtoBuf matches up to a controller. Is that code-generated?

I suspect that the next problem is a mismatch in names. API Version collation is performed on logic name, not route template. Route template may seem intuitive, but there are many nuances. order/{id} and order/{id:int} are not identical, but are semantically equivalent. Should order/{id}/items be part of the Orders or LIne Items (if there is one) API?

By default, ASP.NET Core uses the suffix convention Controller. If this present in the type name, it is removed. A type named Controller will end up as "" and a type named V2Controller will end up as "V2". Unless you otherwise configure it (and there are a few ways), this will be the logical name of the controller and API. These must match for collation to work. If the types are in different namespaces, then it's easy to retain the same name. If they aren't, then they need distinct names because it is enforced by the type system. In this case, API Versioning adds a second (but configurable) convention that supports the format <Name>[V]#Controller. As such, OrderController and OrderV2Controller will both have the logical name "Order" and be collated. There are some edge cases where you might not want that; for example S3Controller.

"" and "V2" are not the same so they will not be collated. You are also versioning by URL segment. By doing so, you have violated the Uniform Interface REST constraint, which makes the API version part of the resource identifier. Since 1.0 is not collated into the set of API versions, the identifier cannot exist, therefore 404 is returned in the response. If you had AddProblemDetails enabled, you would probably see additional details indicating that the API version doesn't match/exist.

My recommendation for your next steps is to:

  1. Use a more specific name; Controller is too generic
    a. If this is not possible due to backward compatibility, then apply [ControllerName("controller")] to V2Controller
  2. Make sure your controller implementations correct and complete
  3. If you're not using OpenAPI, don't both with AddApiExplorer; just use AddMvc

@mcastagna-sa
Copy link
Author

Hi, thanks for taking your time to answer me. This is just an example i've put together just to show that is reproducible. I created the same issue in the grpc repo but i havent heard from them. (See: grpc/grpc-dotnet#2500)
I can upload a zip with the code i posted above so you can check that commenting out that line shows the described behaviour. I appreciate your feedback though.

@mcastagna-sa
Copy link
Author

I followed your suggestions and updated the project i wrote. I got the same behaviour. It would be great if we, at least, can rule out the versioning library as the problem.

TestVersioningGRPC.zip

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