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

#1351 #1510 New AddOcelot extensions to support custom ASP.NET MVC pipeline building via the IMvcCoreBuilder interface #1655

Merged

Conversation

guoyongchang
Copy link
Contributor

@guoyongchang guoyongchang commented May 21, 2023

Closes #1351 #1510

Background

The default AddOcelot method adds .AddNewtonsoftJson(), which was necessary when Microsoft did not launch the System.Text.Json library, but now it affects normal use, so this PR is mainly to solve the problem problem.

默认的AddOcelot方法内部添加了.AddNewtonsoftJson(),在微软没有推出System.Text.Json库的时候是有必要的,但是现在反而影响了正常使用,所以本PR主要是解决该问题。

New Feature

Added the following methods in Ocelot.DependencyInjection.ServiceCollectionExtensions

Ocelot.DependencyInjection.ServiceCollectionExtensions添加了以下方法

  • AddOcelotWithCustomMvcCoreBuilder(this IServiceCollection services, Func<IMvcCoreBuilder, Assembly, IMvcCoreBuilder> customMvcCoreBuilder)
  • AddOcelotWithCustomMvcCoreBuilder(this IServiceCollection services, IConfiguration configuration, Func<IMvcCoreBuilder, Assembly, IMvcCoreBuilder> customMvcCoreBuilder)

Proposed Changes

  • Support custom MvcCoreBuilder to adapt to more changes in the future, this change is mainly to support System.Text.Json

    支持自定义MvcCoreBuilder,以适应未来更多的变化,本次更改主要是为了支持System.Text.Json

@guoyongchang
Copy link
Contributor Author

guoyongchang commented May 21, 2023

@raman-m This PR allows users to use their desired JSON library for serialization, such as System.Text.Json.

For example:

service.AddOcelotUsingBuilder((builder, assembly) =>
{
    return builder
        .AddApplicationPart(assembly)
        .AddControllersAsServices()
        .AddAuthorization()
        .AddJsonOptions(); // use System.Text.Json
});

This is just one of the common usages, users can add more modules they need in the builder.

@raman-m
Copy link
Member

raman-m commented May 22, 2023

Related to: #1515

@raman-m raman-m added feature A new feature medium effort Likely a few days of development effort proposal Proposal for a new functionality in Ocelot needs validation Issue has not been replicated or verified yet labels May 22, 2023
@raman-m raman-m requested review from raman-m and geffzhang May 22, 2023 08:30
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

Hi @guoyongchang ! I'm just curious, what's your name?
I've made some refactoring and styling issue fixes. Hope it is useful! 😸

Could you add your user scenario from your the comment as acceptance test please, to make sure new Json lib is used?
You are welcome to show more techniques with MVC core builder.

Also, I am pleasantly asking you to add unit tests for new extension methods to files: OcelotBuilderTests
Or make new code files in the folder: DependencyInjection

@raman-m raman-m added waiting Waiting for answer to question or feedback from issue raiser and removed needs validation Issue has not been replicated or verified yet labels May 22, 2023
@raman-m raman-m changed the title AddOcelot support custom mvc builder. New AddOcelot extensions to support custom ASP.NET MVC pipeline building via the IMvcCoreBuilder interface May 22, 2023
@guoyongchang
Copy link
Contributor Author

Hi @guoyongchang ! I'm just curious, what's your name? I've made some refactoring and styling issue fixes. Hope it is useful! 😸

Could you add your user scenario from your the comment as acceptance test please, to make sure new Json lib is used? You are welcome to show more techniques with MVC core builder.

Also, I am pleasantly asking you to add unit tests for new extension methods to files: OcelotBuilderTests Or make new code files in the folder: DependencyInjection

@raman-m

You can call me guo, and the name of github is my full Chinese name in pinyin. Maybe you can understand Yongchang Guo?

Nice change. I've tested it with no problems, and there will be no other confusion caused by shortened method names.


I think this function is usually used when integrating with existing API projects, so I put the unit test in Ocelot.IntegrationTests, which I think is more appropriate here.
If it is a pure gateway project, this extension method should not be used, and Newtonsoft.JSON can meet all the needs of users.

About more techniques with MVC core builder, I think that as long as a custom method is provided, all the default things can be overridden, for example, I only need to change the default Json Provider, maybe others need to add more assemblies, or change the default authentication.


Wrote into Ocelot.IntegrationTests

raman-m
raman-m previously approved these changes May 27, 2023
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

@guoyongchang Thanks, Guo!
Good job!

@raman-m
Copy link
Member

raman-m commented May 27, 2023

@geffzhang Could you review please?

geffzhang
geffzhang previously approved these changes May 28, 2023
@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed waiting Waiting for answer to question or feedback from issue raiser labels May 28, 2023
@raman-m raman-m requested a review from TomPallister May 28, 2023 14:57
@TomPallister
Copy link
Member

Thank you for the PR, please fix the merge conflicts and update the docs so people know how to use this and I'm happy to merge!

@raman-m raman-m dismissed stale reviews from geffzhang and themself via ed0ab09 June 12, 2023 12:31
@raman-m raman-m removed the proposal Proposal for a new functionality in Ocelot label Jun 15, 2023
@raman-m raman-m requested a review from geffzhang June 15, 2023 16:40
@raman-m
Copy link
Member

raman-m commented Jun 15, 2023

@geffzhang Geff,
Sorry for dismissing of your stale review!
Could you review and approve the PR once again please?

@raman-m raman-m self-requested a review June 15, 2023 16:44
raman-m
raman-m previously approved these changes Jun 15, 2023
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

  • Merge conflicts have been resolved after updating with new top commits from develop branch
  • Added new Dependency Injection feature in documentation
  • Added unit tests for the OcelotBuilder class to cover AddOcelotUsingBuilder methods

@raman-m
Copy link
Member

raman-m commented Jun 16, 2023

@TomPallister wrote on Jun 4, 2023

Thank you for the PR, please fix the merge conflicts and update the docs so people know how to use this and I'm happy to merge!

Done! See the message above plz!

Tom, could you review and merge it please?

guoyongchang and others added 26 commits September 23, 2023 17:20
Fix target framework as net7.0
Use correct heading levels for (sub)sections
Readability improvements
"IServiceCollection extensions" paragraph.
"The OcelotBuilder class" paragraph.
@raman-m raman-m force-pushed the feat/supportCustomOcelotMvcBuilder branch from f7f695a to 420bc45 Compare September 23, 2023 14:26
@raman-m raman-m merged commit b7976fc into ThreeMammals:develop Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Bug or feature would be accepted as a PR or is being worked on feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with default Newtonsoft Serializer Ocelot overides System.Text.Json parser and uses Newtonsoft
5 participants