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

Refactor the whole project #69

Open
xuzhg opened this issue Jun 3, 2020 · 2 comments
Open

Refactor the whole project #69

xuzhg opened this issue Jun 3, 2020 · 2 comments
Labels
status:needs-discussion An issue that requires more discussion internally before resolving type:feature

Comments

@xuzhg
Copy link
Contributor

xuzhg commented Jun 3, 2020

Short summary (3-5 sentences) describing the issue.

Recently, I got a lot of requirements, for example

  1. I don't want to expand on certain element
  2. I don't need this operation
  3. I need this ...
  4. etc..

Yes, we can meet all of these by modifying the codes, by adding settings, etc.
However, i'd think maybe it's time to refactor it in the next major release.

Design

I'd like to use the Dependency Injection to allow customer to customize the converting process.

We need figure out the working flow from OData to OpenAPI. The process steps are certain, at each step, we can create a service to work on it.

Services

We need figure out the services used during converting.
for example:

  1. we should have a OData path provider service
  2. we should have a OpenApi path generator service
  3. We should have a convert engine

Extension methods

I think we need

public static IServiceCollection UseOpenApiOData(IServiceCollection services)
{
        services.AddSingleton<IOpenApiODataConverter>(); // converter or generator
        services.AddSingleton<IODataPathProvider>();
        services.AddSingleton<IODataOpenApiPathHandler>();
        services.AddSingleton<IODataOpenApiOperationHandler>();
.....
        service.AddSingleton<ODataOpenApiOptions>();
        return services;
}

Usage

  1. you can create the OpenApiODataConverter instance (normal way)
  2. you can retrieve it from service provider.
IServiceCollection services= new ServiceCollection();
services.UserOpenApiOData();
IServiceProvider sp = services.CreateServiceProvider();
IOpenApiODataConverter converter = sp.GetService<IOpenApiODataConverter>();
...

Advantage

  1. It can be used in the ASP.NET Core pipeline, for example we can create a middleware to generate the OpenAPI description for a OData service.

  2. It can support to customize the converting. For example, we can replace a certain service using customized service.

@jukkahyv
Copy link
Contributor

This library could be useful to us and we also have the need to customize the generation replace services.

However, the way you're describing it sounds more like service locator pattern, which is more commonly considered an anti-pattern.

In my understanding, in pure dependency injection, you would inject the services (usually as constructor parameters), not the service provider. Using the service provider makes it more like service locator. The main drawback of service locator is that it's hard to figure out what other services a class depends on. So if Operation handler depends on Path hander, then it's much more obvious if Operation handler receives IODataOpenApiPathHandler as constructor parameter. Then there can be a separate "activator" or "factory" class which constructs the services, possibly making use of the dependency injection library and service provider.

@garethj-msft
Copy link
Member

In general, making substantial breaking changes to the API surface of a library makes it less useful to Microsoft product teams consuming it, who simply can't rewrite their code to consume new, disruptive changes.

This simply stops us from consuming the new version and leaves us stuck on legacy versions.

Unless there is overwhelming new value, we should take the hit that the API surface which exists when a library becomes popular is the API surface for the forseeable future. The idea that the existence of major versions means it's OK to regularly make breaking change is a fallacy as a consumer.

@adhiambovivian adhiambovivian added the status:needs-discussion An issue that requires more discussion internally before resolving label Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:needs-discussion An issue that requires more discussion internally before resolving type:feature
Projects
None yet
Development

No branches or pull requests

4 participants