ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.32k stars 1.63k forks source link

Documentation about middlewares seems to be incorrect #1686

Closed lvyyljn closed 12 months ago

lvyyljn commented 1 year ago

I'm currently working on updating the response from downstream and noticed some inconsistency As it's stated here https://ocelot.readthedocs.io/en/latest/features/middlewareinjection.html Obviously you can just add middleware as normal before the call to app.UseOcelot() It cannot be added after as Ocelot does not call the next middleware.

And it's not true, as the last middleware in the BuildOcelotPipeline method is HttpRequesterMiddleware that calls the next middleware, I think documentation should be changed

Specifications

raman-m commented 1 year ago

Hi lvyyljn! Thanks for your interest in Ocelot!

Specifications

  • Version: 18.0.0
  • Platform: .NET 6.0

Why do you use previous version? Your project is based on .NET 6 ? It is time to upgrade to .NET 7 and Ocelot 19.0.2.

lvyyljn commented 1 year ago

Hi @raman-m, it was predefined before me to use .NET 6, so we have to use 18.0.0, would be glad to update but still) For the 19.0.2 documentation to looks outdated

raman-m commented 1 year ago

What is your real name? Where are you from? Why is your GitHub profile empty?

lvyyljn commented 1 year ago

Is this relevant?) I am only concerned about documentation being outdated, My name is Denys and I am from Ukraine, my profile is empty as it's first time I need to create issue here

raman-m commented 1 year ago

@lvyyljn commented on 2023-08-29 at 07:39:

I'm currently working on updating the response from downstream and noticed some inconsistency As it's stated here https://ocelot.readthedocs.io/en/latest/features/middlewareinjection.html Obviously you can just add middleware as normal before the call to app.UseOcelot() It cannot be added after as Ocelot does not call the next middleware.

We have most actual docs in the repo: Middleware Injection and Overrides | Ocelot/docs/features/middlewareinjection.rst at develop · ThreeMammals/Ocelot It is better to share the links of the repo. So, the text we are discussing is here: docs/features/middlewareinjection.rst | Lines 40-41


Obviously you can just add middleware as normal before the call to app.UseOcelot()

It says actually that develop should add all custom middlewares and after that developer should call UseOcelot. The argument of this method should be configuration object.


It cannot be added after as Ocelot does not call the next middleware.

It says you cannot configure the pipeline after Ocelot's builder has ran. It is correct. Tom explains here the reason which is Ocelot does not call the next middleware. This reason is strange. And it looks like incomplete... But I believe after changing to Ocelot does not call the next middleware configuration. it should become 100% correct.

lvyyljn commented 1 year ago

@raman-m thanks for quick response, few questions

It says you cannot configure the pipeline after Ocelot's builder has ran.

You mean here the Ocelot's pipeline correct? And maybe better would be

Next called middlewares won't affect Ocelot configuration

Maybe I misunderstood something?

raman-m commented 1 year ago

@lvyyljn commented on Aug 28:

And it's not true, as the last middleware in the BuildOcelotPipeline method is HttpRequesterMiddleware that calls the next middleware, I think documentation should be changed

Why would you like to discuss the internal HttpRequesterMiddleware ? It is a part of pipeline but it is private and cannot be overridden because this middleware is not user's one! From other side, yes, this is the last middleware in overall Ocelot & ASP.NET pipeline, but it serves non-user operation. The last user middleware which can be overridden is: PreQueryStringBuilderMiddleware being read from pipeline configuration object (pipelineConfiguration). In general the docs says that PreQueryStringBuilderMiddleware is the last user middleware! And there are no another user middlewares in pipeline.

You seem to be confused about the distinction between system (private) and user (public) middleware. And yes, the docs can be somehow corrected to eliminate this confusion.

raman-m commented 1 year ago

@lvyyljn commented on 2023-08-29 at 08:14:

Maybe I misunderstood something?

Yes, you did.


And maybe better would be "Next called middlewares won't affect Ocelot configuration"

Denys, you are welcome to open PR!

lvyyljn commented 1 year ago

@raman-m Totally agree with you on the private and public, and I don't intend to override any of the private middlewares I wanted to add middleware after UseOcelot, and was confused as the documentation stated Ocelot doesn't call the next middleware

raman-m commented 1 year ago

I cannot get you!... What does it mean "to add middleware after UseOcelot"?! The UseOcelot method runs the pipeline builder which builds ASP.NET pipeline. I guess it is impossible to change middlewares chain in run-time. After building & running ASP.NET pipeline it cannot be changed. It remains unchanged till the next app restart based on pipeline configuration. We are discussing the docs, not exotic feature, right?... Are you going to correct the docs or not?

lvyyljn commented 1 year ago

Yes, you're right UseOcelot builds a pipeline, but the build method underhood doesn't close to the ability to edit the pipeline further, it's just a checkpoint, You can try it on a local machine with a few minutes of your time, this code works fine without any error

app.UseSwaggerForOcelotUI(opt =>
{
    opt.PathToSwaggerGenerator = "/swagger/docs";
    opt.ReConfigureUpstreamSwaggerJson = AlterUpstream.AlterUpstreamSwaggerJson;
}).UseOcelot().Wait();

app.UseMiddleware<RequestCultureMiddleware>();

And no, I won't create a PR for docs

raman-m commented 1 year ago

@lvyyljn commented on Aug 29

👌 You've investigated that a custom middleware can be attached after building pipeline. Great! But it is configuration stage during app startup.

You can do any Ocelot research you like.

raman-m commented 1 year ago

@lvyyljn commented on Aug 29

And no, I won't create a PR for docs

FYI Your issue-question will be closed by #1678 I have added all results of this discussion to mentioned PR. See the commit bd19513