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

[csharp-netcore] Adding generic host library #10627

Merged
merged 47 commits into from
Jan 28, 2022

Conversation

devhl-labs
Copy link
Contributor

@devhl-labs devhl-labs commented Oct 18, 2021

What is this?

This PR will add integration with Microsoft's Generic Host. The host manages application lifetime, dependency injection, application configuration, and much more. This library will provide extension methods for the host. This makes it easy to integrate a generated API wrapper into an existing application.

How do I use this?

Here is sample code showing what it looks like to use this library.

public class Program
{
    public static async Task Main(string[] args)
    {
        var host = CreateHostBuilder(args).Build();
        IFooApi fooApi = host.Services.GetRequiredService<IFooApi>();
        Foo foo = await fooApi.GetFooAsync("todo");
    }

    public static IHostBuilder CreateHostBuilder(string[] args) => Host.CreateDefaultBuilder(args)
        .ConfigureApi((context, options) =>
        {
            ApiKeyToken token = new("<your token>");
            options.AddTokens(token);

            // optionally configure the token provider strategy, default is RateLimitProvider
            options.UseProvider<RateLimitProvider<ApiKeyToken>, ApiKeyToken>();

            options.ConfigureJsonOptions((jsonOptions) =>
            {
                // your custom converters if any
            });

            options.AddApiHttpClients(builder: builder => builder
                .AddRetryPolicy(2)
                .AddTimeoutPolicy(TimeSpan.FromSeconds(5))
                .AddCircuitBreakerPolicy(10, TimeSpan.FromSeconds(30))
                // any other middleware
            );
        });
}

Other Changes

api.mustache

The api.mustache template was also rewritten to take full advantage of proper dependency injection. There is also no longer a need for the Configuration class, so this was another reason to make changes in this class.

Tokens

Each type of security protocol (like JWT and others) will generate a different class to represent the token. This class will store the token's credentials as well as a timespan. The timespan is used in client side rate limiting so we can ensure we don't get server throttled.

RateLimitProvider

This class provides tokens to the IFooApi. Tokens will be rate limited at the client side to ensure we do not get rate limited by the server. The token timeout is defined in the token's constructor.

TokenContainer

This is an internally used class that end users don't have to see. It merely holds the tokens in a way that is conducive for dependency injection.

HostConfiguration

This class is used by extension methods on the generic host.

ApiResponse<T> and ApiException

When do we expect exceptions when the response is not 200?

  • GetFooAsyncWithHttpInfo -> will not throw but HttpStatus is in the ApiResponse<T>
  • GetFooAsync -> will throw
  • GetFooOrDefaultAsync -> will return null

Future Changes

  • full annotation of nullable reference types
  • migrate to System.Text.Json

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@devhl-labs
Copy link
Contributor Author

cc technical committee @mandrean @frankyjuang @shibayan @Blackclaws @lucamazzanti

@Blackclaws
Copy link
Contributor

If I'm not mistaken this is mainly relevant for aspnetcore apps using api clients to access other services right? At least I don't know of any usage of GenericHost outside of aspnetcore.

I think this is in general a good idea to have, especially because DI is much more relevant in a serverside application.

So far a quick LGTM from me, as it doesn't seem to change any existing behaviour, it just adds a new generator subclass.

I would, in general really like to clean up the netcore generator for the 6.x series of releases. We can finally cut some old baggage off and drop support for anything < Netcore 3.1

@devhl-labs
Copy link
Contributor Author

devhl-labs commented Oct 19, 2021

If I'm not mistaken this is mainly relevant for aspnetcore apps using api clients to access other services right?

The host manages application lifetime, so it's not limited to any particular use case. It used to be called Web Host and was only for aspnet core, so maybe that is the confusion.

@Blackclaws
Copy link
Contributor

Ah, true they did actually extend the usage to console apps as well. I'm not doing a lot of csharp work outside of aspnetcore that's probably why I never used it in a different context as well.

@devhl-labs
Copy link
Contributor Author

@wing328 Can i get a merge?

@Blackclaws
Copy link
Contributor

Just tried it out, produces non compiling code for me. See the attached spec

openapi: 3.0.3
info:
  title: MangoInc Core API Specification
  description: MangoInc Core API Specification
  version: 1.0.0

paths:
  /v1/mangos:
    get:
      operationId: "getAllMangos"
      tags:
        - Mango
      summary: Returns a list of all mangos.
      description: mangos for all
      responses:
        '200': # status code
          description: A JSON array of mangos
          content:
            application/json:
              schema:
                type: array
                items:
                  $ref: '#/components/schemas/Mango'
    post:
      operationId:  "mangoCreator"
      tags:
        - Mango
      requestBody: 
        content: 
          application/json:
            schema:
              $ref: '#/components/schemas/Mango'
      responses: 
        '200':
          description: A JSON Mango
          content:
            application/json:
              schema: 
                $ref: '#/components/schemas/Mango'
  /v1/manga:
    post:
      operationId:  "mangaMan"
      tags:
        - Mango
      requestBody: 
        content: 
          image/jpeg:
            schema:
              type: string
              format: binary
      responses: 
        '200':
          description: A JSON Mango
          content:
            application/json:
              schema: 
                $ref: '#/components/schemas/Mango'

  /v1/mangaMO:
    post:
      operationId:  "mangaManMo"
      tags:
        - Mango
      requestBody: 
            content:
              multipart/form-data:
                schema:
                  type: object
                  properties:
                    orderId:
                      type: integer
                    userId:
                      type: integer
                    fileName:
                      type: string
                      format: binary
      responses: 
        '200':
          description: A JSON Mango
          content:
            application/json:
              schema: 
                $ref: '#/components/schemas/Mango'

components:
  schemas:
    Mango:
      type: object
      properties:
        id:
          type: string
          format: uuid
        weight:
          type: integer
          minimum: 0
          maximum: 100
  
    Greeting:
        type: object
        properties:
          reply:
            type: string
            maxLength: 20
            enum:
              - hi
              - howdy

With this config:

library: generichost
additionalProperties:
  reUseHttpClient: true
  supportsRetry: false
  validatable: false
  buildTarget: library
  operationIsAsync: true
  operationResultTask: true
  packageName: MangoInc.Core.API.Client
  packageVersion: 0.1.0
  packageTitle: MangoInc Core Client API Contract
  sourceFolder: "src"
  targetFramework: net5.0

Copy link
Contributor

@Blackclaws Blackclaws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the test project that is generated doesn't build because there is no longer a parameterless constructor.

@Blackclaws
Copy link
Contributor

I do like a lot of the general cleanup that was done in CSharpNetCoreClientCodegen, so I'd love to see the bugs fixed and this merged.

@devhl-labs
Copy link
Contributor Author

Wonderful! Thanks for the help. I will take a look at the comments next week.

@wing328 wing328 modified the milestones: 5.3.1, 5.4.0 Dec 21, 2021
@devhl-labs
Copy link
Contributor Author

devhl-labs commented Dec 21, 2021

Fixed these issues. Mostly problems regarding nullable reference types. I usually have it enabled so I missed. Note that since .NET 6 NRT is enabled by default in new projects, so we should also be at least encouraging it.

Fixing this caused issues with NRT enabled. I will take a closer look, but not until next week.

@wing328
Copy link
Member

wing328 commented Dec 21, 2021

@devhl-labs thanks for the quick fix. Do you need to manually change the target framework in order to make the project compiles in your environment?

Fixing this caused issues with NRT enabled. I will take a closer look, but not until next week.

Please take your time. We'll target to include this change in the 5.4.0 release scheduled in 2022 Jan.

@devhl-labs
Copy link
Contributor Author

devhl-labs commented Dec 28, 2021

@wing328 Compiles in net3.1 and net5.0 with and without NRT using the swagger file you used.

edit - renamed projectName to apiName and added to the cli options so it is documented. If this should be changed, just tell me please.

@wing328
Copy link
Member

wing328 commented Jan 28, 2022

@devhl-labs thanks for the update. May I know if you've time for a quick chat via Slack?

@wing328
Copy link
Member

wing328 commented Jan 28, 2022

Tested locally and the result is good. Thanks for the PR.

@wing328 wing328 merged commit 24366be into OpenAPITools:master Jan 28, 2022
@devhl-labs devhl-labs deleted the generic-host branch January 28, 2022 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants