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

Documentation about middlewares seems to be incorrect #1686

Closed
lvyyljn opened this issue Aug 28, 2023 · 13 comments · Fixed by #1678
Closed

Documentation about middlewares seems to be incorrect #1686

lvyyljn opened this issue Aug 28, 2023 · 13 comments · Fixed by #1678
Assignees
Labels
documentation Needs a documentation update merged Issue has been merged to dev and is waiting for the next release

Comments

@lvyyljn
Copy link

lvyyljn commented Aug 28, 2023

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

  • Version: 18.0.0
  • Platform: .NET 6.0
@raman-m
Copy link
Member

raman-m commented Aug 29, 2023

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
Copy link
Author

lvyyljn commented Aug 29, 2023

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
Copy link
Member

raman-m commented Aug 29, 2023

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

@lvyyljn
Copy link
Author

lvyyljn commented Aug 29, 2023

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
Copy link
Member

raman-m commented Aug 29, 2023

@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
Copy link
Author

lvyyljn commented Aug 29, 2023

@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
Copy link
Member

raman-m commented Aug 29, 2023

@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
Copy link
Member

raman-m commented Aug 29, 2023

@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!

@raman-m raman-m added the question Initially seen a question could become a new feature or bug or closed ;) label Aug 29, 2023
@lvyyljn
Copy link
Author

lvyyljn commented Aug 29, 2023

@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
Copy link
Member

raman-m commented Aug 29, 2023

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
Copy link
Author

lvyyljn commented Aug 29, 2023

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
Copy link
Member

raman-m commented Sep 5, 2023

@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 added a commit that referenced this issue Sep 5, 2023
Add section "ASP.NET Core Middlewares and Ocelot Pipeline Builder"
@raman-m raman-m marked this as a duplicate of #1678 Sep 5, 2023
@raman-m raman-m linked a pull request Sep 5, 2023 that will close this issue
@raman-m
Copy link
Member

raman-m commented Sep 5, 2023

@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

@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on documentation Needs a documentation update labels Sep 5, 2023
raman-m added a commit that referenced this issue Sep 23, 2023
Add section "ASP.NET Core Middlewares and Ocelot Pipeline Builder"
raman-m added a commit that referenced this issue Sep 25, 2023
* #1676 Update authorization.rst

* #1646 Update bigpicture.rst

* #1628 Update websockets.rst

* #1614 Update README.md

* #1552 Update configuration.rst

* #1547 Update caching.rst

* #1542 Update requestaggregation.rst

* Revert "#1639 Update ClientRateLimitMiddleware.cs"

This reverts commit 996719e.

* #1537 Update routing.rst

* #1520 Update authentication.rst

* #1459 Update building.rst

Fix inline code block

* #1412 Update errorcodes.rst

* #1407 Update headerstransformation.rst

* #1406 Update headerstransformation.rst

Remove extra '!'

* #1320 Update README.md

* Update README.md

Mark classes. Split sentences.

* #1284 Update claimstransformation.rst

* #1232 Update servicediscovery.rst

* #1224 Update loadbalancer.rst

* #1189 Update caching.rst

* Update gettingstarted.rst

Mark up 'net7.0' as code block. Convert script block to PowerShell one. Change the link to NuGet.

* #1386 Update loadbalancer.rst

* Update loadbalancer.rst: Make code snippet shorter

* #1686 Update middlewareinjection.rst

Add section "ASP.NET Core Middlewares and Ocelot Pipeline Builder"

* Update logging.rst

Review RST markup

* Update methodtransformation.rst

Review RST markup

* Update README.md

Sync to "Big Picture" page in Introduction
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release and removed question Initially seen a question could become a new feature or bug or closed ;) accepted Bug or feature would be accepted as a PR or is being worked on labels Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Needs a documentation update merged Issue has been merged to dev and is waiting for the next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants