OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.84k stars 6.59k forks source link

Add switch to suppress auto-insertion of operationIndex [csharp-netcore] #13229

Open protegesolutions opened 2 years ago

protegesolutions commented 2 years ago

Since version 6.0.0 an optional parameter operationIndex is being automatically added to all calls in csharp-netcore - and possibly other* - client side SDK code.

Eg

/// <summary>Get a list of lists.</summary>
/// <param name="listType">The type of list to return.</param>
/// <param name="operationIndex">Index associated with the operation.</param>
/// <returns>CollectionOfList</returns>
public CollectionOfList GetLists(string listType, int operationIndex = 0);

This behavior persists in version 6.0.1.

I can't find any reference to this in the changes for 6.0.0, or anywhere other a very brief comment in https://github.com/OpenAPITools/openapi-generator/pull/12090.

This a problem for us as it adds a parameter that is likely to confuse users of the third-party consumed .NET Standard 2.0 class library we generate from nearly 20 lengthy OpenAPI documents on a regular basis.

Assuming this behavior is intentional (ie, not a bug), we would like to see a configuration switch - eg, suppressOperationIndexes (default true) - to suppress the creation of these parameters. (Personally, I think it should really be addOperationIndexes (default false) but that's just an opinion based on previous version behavior.)

We have the option of using scripts to post-process the generated code but this is laborious to maintain and shouldn't really be necessary (IMHO).

* Tested version 6.0.1 with csharp and operationIndex is not added to the SDK.

wing328 commented 2 years ago

That's sound reasonable.

What about setting the operation index in the API classes instead (e.g. PetApi, UserApi ,StoreApi in the petstore example)?

Or only add operationIndex if there are path level servers defined?

cc @ajrice6713

protegesolutions commented 2 years ago

Thanks for your reply.

  1. Can you explain or point me to what purpose the operationIndex is supposed to serve?

  2. Can you point me to the change note/log/doco that introduced it (in 6.0.0)?

wing328 commented 2 years ago

https://github.com/OpenAPITools/openapi-generator/issues/10639 is the related issue. There's a link to the PR implementing this feature.

protegesolutions commented 2 years ago

Ok. No wonder I couldn't find explicit refs to operationIndex. That issue doesn't really explain - from my POV as a client class library developer - the need for it but I'll take your word. Again, from where I sit, this addition just adds "noise" to the methods - I can't see how it would be useful in a client library SDK. Just some feedback.

Regarding "Or only add operationIndex if there are path level servers defined?", I have no idea; I don't use generated code in a way that is impacted by "path level servers" (I actually don't even know what that refers to, sorry.) Basically, I just would like to not see any auto-generated parameters or anything unnecessary or without any means to turning it off. Aka, as it was pre-version 6.

wing328 commented 2 years ago

Thanks for the feedback. I agree with you that ideally it shouldn't be added if it's not needed.

protegesolutions commented 2 years ago

BTW, version 6 seems to have an issue where it inserts a lot of unnecessary name spacing that wasn't occurring as much in previous versions. I'll post a bug report about that separately.

ajrice6713 commented 2 years ago

I could see where only adding the operationIndex argument if path level serves exists being a happy medium.

Supporting path level or operation level servers IMO should be considered in all of the generators since it's valid OpenAPI and there are use cases that require it - for context we take multiple API specs that represent services hosted at different base URLs and stitch them together at build time - and then insert each services base url as a server under the operation in the spec to create a single client for multiple APIs.

I'm not familiar with how the dotnet generator was implemented this functionality but it sounds like the parameter is optional - creating the optional parameter for only methods that have path/operation level servers could be a happy medium - but is this work that would be required/adopted if the parameter is optional?

protegesolutions commented 2 years ago

@ajrice6713 This might be a bit beyond our use case. But we, too, combine 20-odd APIs into a single class library SDK but the base URI takes care of the call path. So I'm a still a little confused by the need to have an operation index, especially as a method parameter.

I'm not even sure how the auto-added optional parameter operationIndex (int, default 0) can direct a call to a particular server in any case (refer example code above). I'd be happy to read through some doco on this if you can point me to it.

Long and short is, its addition to the code generator without any way to suppress it leaves it "hanging out in the breeze" as far as users of our library are concerned. ("We see a new parameter in all of the SDK methods called operationalIndex - what's it for? How does it work?", "Can we ignore it?", "What happens if we provide a value?", etc.)

BTW, it's only been introduced in the csharp-netcore generator (since v6.0.0); so far the standard csharp generator doesn't create it. (Fingers crossed it's not changed, at least without a kill switch option.)

ajrice6713 commented 2 years ago

To preface - I didn't author this contribution - I implemented functionality in the java generator to support path and operation level servers objects.

It looks like the Configuration class has a Servers object and an OperationServers object - Servers coming from the top level servers array in the OpenAPI spec (if that exists - otherwise defaults to localhost) and OperationServers generated from the servers array of each operation (if they exists)

I generated a client from a spec with no top-level servers array in the OpenAPI spec + unique servers arrays for each operation, and stepping through the client it looks like the operation is passed by default into the clients Exec function to execute http requests - which then checks the client's configuration for whether or not an operation level server is defined to use as the base url - otherwise the client sends the request to the default baseUrl

# ExampleSpec.yml
---
openapi: 3.0.3
info:
  title: Swagger Petstore - OpenAPI 3.0
  description: Sample
  version: 1.0.11
tags:
  - name: pet
    description: Everything about your Pets
    externalDocs:
      description: Find out more
      url: http://swagger.io
paths:
  /pet:
    get: 
      servers: 
        - url: https://operation1.com
          description: Server for operation1
      tags:
        - pet
      summary: Get a pet
      description: get a pet
      operationId: getPet
      responses:
        '200':
          description: Successful operation
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Pet'          
    post:
      servers:
        - url: https://operation2.com
          description: Server for operation2
      tags:
        - pet
      summary: Add a new pet to the store
      description: Add a new pet to the store
      operationId: addPet
      requestBody:
        description: Create a new pet in the store
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/Pet'
        required: true
      responses:
        '200':
          description: Successful operation
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Pet'          
components:
  schemas:
    Pet:
      type: object
      properties:
        id:
          type: integer
          format: int64
          example: 10
  securitySchemes:
    api_key:
      type: apiKey
      name: api_key
      in: header
# ApiClient.cs

public ApiClient()
        {
            _baseUrl = Org.OpenAPITools.Client.GlobalConfiguration.Instance.BasePath;
        }

private ApiResponse<T> Exec<T>(RestRequest req, RequestOptions options, IReadableConfiguration configuration)
        {
            var baseUrl = configuration.GetOperationServerUrl(options.Operation, options.OperationIndex) ?? _baseUrl;
            RestClient client = new RestClient(baseUrl);
# Configuration.cs

        public Configuration()
        {
            Proxy = null;
            UserAgent = "OpenAPI-Generator/1.0.0/csharp";
            BasePath = "http://localhost";
            DefaultHeaders = new ConcurrentDictionary<string, string>();
            ApiKey = new ConcurrentDictionary<string, string>();
            ApiKeyPrefix = new ConcurrentDictionary<string, string>();
            Servers = new List<IReadOnlyDictionary<string, object>>()
            {
                {
                    new Dictionary<string, object> {
                        {"url", ""},
                        {"description", "No description provided"},
                    }
                }
            };
            OperationServers = new Dictionary<string, List<IReadOnlyDictionary<string, object>>>()
            {
                {
                    "PetApi.GetPet", new List<IReadOnlyDictionary<string, object>>
                    {
                        {
                            new Dictionary<string, object>
                            {
                                {"url", "https://operation1.com"},
                                {"description", "Server for operation1"}
                            }
                        },
                    }
                },
                {
                    "PetApi.AddPet", new List<IReadOnlyDictionary<string, object>>
                    {
                        {
                            new Dictionary<string, object>
                            {
                                {"url", "https://operation2.com"},
                                {"description", "Server for operation2"}
                            }
                        },
                    }
                },

Above demonstrates a client generated that fits my use case (one server per operation) - but it would be perfectly valid to have an operation level server with more than one URL in the servers array (prod, test, etc) - which is where the operationIndex seems to come into play here. If i were to add a second test server for my getPet operation:

  /pet:
    get: 
      servers: 
        - url: https://operation1.com
          description: Server for operation1
        - url: https://test.operation1.com
          description: Test server for operation1

The OperationServers object in my Configuration.cs class would contain both servers in the dictionary & alllow me to point my requests to the test server by setting the operationIndex to 1

  "PetApi.GetPet", new List<IReadOnlyDictionary<string, object>>
  {
      {
          new Dictionary<string, object>
          {
              {"url", "https://operation1.com"},
              {"description", "Server for operation1"}
          }
      },
      {
          new Dictionary<string, object>
          {
              {"url", "https://test.operation1.com"},
              {"description", "Test server for operation1"}
          }
      },
  }
},

but the base URI takes care of the call path

Are all of your APIs under the same Base URI?

It sounds like your use case doesn't require this so i understand not wanting a parameter on every method that your users never have to touch - but I am by no means a C# pro - so i don't really know the best way to provide support for multiple servers without having to set it somewhere, especially not without introducing a breaking change

Edit: a switch to suppress this would solve the issue it sounds like - you just don't want me implementing that 😅

wing328 commented 2 years ago

@ajrice6713 no worries. I'll try to come up with a fix before the v6.1.0 release.

Can you please share your spec with me publicly or privately so that I can use it to test the fix I'm going to submit?

ajrice6713 commented 2 years ago

@wing328 I will provide via slack

protegesolutions commented 2 years ago

@ajrice6713 So you're saying that the operationIndex parameter is used to make calls to different servers? If so, I honestly can not envision a situation where we would want to direct method/call to different servers within the same library. It seems like quite an esoteric need to me.

Each of the 20-ish APIs we compile into our library have different base URIs, eg

https://api.sky.blackbaud.com/list/v1 https://api.sky.blackbaud.com/event https://api.sky.blackbaud.com/school

We have no control over the actual servers these URLs are served from.

@wing328 I'm not sure if you wanted a spec from me, but here's one anyway:

https://developer.sky.blackbaud.com/docs/services/list/export?DocumentFormat=Swagger

Let me know if you'd like any more detail or background. And thanks for your engagement on this.

ajrice6713 commented 2 years ago

@protegesolutions that is correct - the operation index allows you to choose which server to send the request to if it is defined in the spec. IMO this could be very useful for testing if the server has test/mock environments. It also makes it very easy to point to your own mock sever and perform integration tests if you have the need to do so.

Our use case requires this functionality, as we have multiple APIs hosted at different servers that accept the same auth credentials.

So we need to be able to support the operation level servers - at build time for our client SDK we take the api specs for each server and bundle them together into one singular API spec organized by tags, with each server's base url defined in the operation server array.

With your use case - it looks like your endpoints have the same base url - so you are probably utilizing the top level servers array and bundling all of your paths and operations under the same server - which is the more popular use case no doubt - but operation level servers are valid OpenAPI and our use case requires it - hence why I think this functionality is important.

OpenAPI also supports multiple top level server definitions and multiple path level server definitions - which i don't know if most generators support (I believe Java does) - but one could argue that those need to be added to the clients - so a common place to define which server you want to point your request at IMO makes a lot of sense, with a default to production (the first value in the servers array) with a logical default hierarchy (operation > path > spec). Of course the most flexible place to define this is in the method arguments - but it does add extra parameters.

To implement this in java I overloaded the api call - I would assume that a default optional parameter that is well documented in c# (and the other generators that support default/optional parameters nicely) should be an acceptable workaround to support everyone's usage of OpenAPI - but it's @wing328's project - so how it's implemented should fall on his team.

As long as the generated clients support the OpenAPI features we need and the interface is logical - I'm satisfied. A flag to suppress that argument at build time sounds like it would work for everyone - we should just consider the fact that we may want/need more optional arguments to support more OpenAPI features in the future - so the flag should be built with that in mind.

protegesolutions commented 2 years ago

Just out of interest, why wasn't the parameter called something like serverId? Wouldn't that have been much clearer? Or can it be used in other ways?

I think adding it as a method parameter is overkill; how often are you going to want/need to split out individual methods between servers? Why not just make it a client setting? The overhead for 20 APIs with hundred of methods each, each with an operationIndex parameter added seems a bit extreme (to me). Especially given how few clients need it. It's a bit like putting a baby seat in every car just in case someone needs one. Sure, put mounts in every car, but don't make everyone cart the actual seat around with them.

ajrice6713 commented 2 years ago

@protegesolutions sorry for the delay - was out of town last week on vacation

why wasn't the parameter called something like serverId

Good question - operationServerIndex might have been a better choice as well, especially if we were to add a pathServerIndex and serverIndex for the other supported server arrays.

how often are you going to want/need to split out individual methods between servers

If i have test environments at different base urls that i want to test against pre-deployment/release, than maybe very often. We run tests on our clients pre deployment and every night, and plan to do so in 6 OpenAPI Generator clients. This functionality could also gives our users the ability to run tests with our clients were we to provide test server urls to them.

In general, if i was building an API I would build a test server and I'd have my integration tests run against the test server wherever needed/necessary. It really varies from use case to use case/developer wants & needs.

It sounds to me like a flag at build time or a field in the config file to suppress extra method arguments would be the best way to go.

protegesolutions commented 2 years ago

If i have test environments at different base urls that i want to test against pre-deployment/release

Yes, but per method? We use server contextualization for test/dev/live enviros as well but we would never - or very seldom - need to do this on a per call basis. The overhead it adds to the code for the rare use case would be too high.

ajrice6713 commented 2 years ago

If I have multiple methods in a client that use different base URIs, then I would need to be able to define it per method.

Other generators (python is a good example) have a plethora of optional method arguments that could be passed in.

hamermike commented 2 years ago

Yes, but per method?

Yes, this follows the OpenAPI spec which supports these features whether or not your implementation includes them. You can take a look at the latest OpenAPI spec which the SDKs are generated from here.

glitchedmob commented 6 months ago

Wanted to chime in with an issue we're seeing because of this. We actually have a query parameter on an endpoint called operationIndex This parameter being automatically added is causing the generator to create a project that can't even be compiled because there are now two arguments with the exact same name in some of the methods.

ajrice6713 commented 5 months ago

Changing the name to operationServerIndex sounds like it would solve your issue, that would be a breaking change but IMO thats ok with the project being versioned.

That change would then break anyone who happened to have operationServerIndex as a query parameter.