search

ThreeMammals / Ocelot

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

#2054 #2059 Manage `EnableContentHashing` setting by global `CacheOptions` #2058

Closed thiagoloureiro closed 2 months ago

thiagoloureiro commented 2 months ago

Fixes #2059

Proposed Changes

This PR is prepared to fix the EnableContentHashing not being considered on ocelot.json.

FileCacheOptions didn't have the option to fetch from the config and also the RoutesCreator didn't call the overload with the settings.

raman-m commented 2 months ago

Thank you, Thiago! What issue is it related to?

ggnaegi commented 2 months ago

It is related to the following issue: https://github.com/ThreeMammals/Ocelot/issues/2059

ggnaegi commented 2 months ago

@raman-m @thiagoloureiro I have added a global configuration for the cache options. Since we introduced a breaking change by adding the property EnableContentHashing and setting it to false by default, I think the users shouldn't bear the burden. That's why I thought adding the global configuration point EnableContentHashing would help most of them. The documentation will be updated too.

ggnaegi commented 2 months ago

@raman-m It's ready for review. I'm still using Bddfy, it was straightforward. And there is a silly problem the application is expecting TtlSeconds to be explicitly set for each route, since TtlSeconds > 0 -> IsCached.

ggnaegi commented 2 months ago

@raman-m I have added some unit tests for the CacheOptionsCreator, and implemented most of your suggestions

ggnaegi commented 2 months ago

@raman-m it's sound for me, should I squash and merge?

raman-m commented 2 months ago

@ggnaegi Please don't merge yet! Hold on for a 10-20 minutes... I want to present a commit related to the property name.

raman-m commented 2 months ago

@ggnaegi The commit found at https://github.com/ThreeMammals/Ocelot/pull/2058/commits/3290aa64f0b2f4b16d929d5acee4ddb10ae1dae5 reflects my earlier suggestion regarding the proposed renaming. I'm confident that this commit does not introduce any breaking changes. Please share your thoughts.

raman-m commented 2 months ago

@ggnaegi commented:

it's sound for me, should I squash and merge?

Yes, please do, provided you have no objections to my last commit https://github.com/ThreeMammals/Ocelot/pull/2058/commits/3290aa64f0b2f4b16d929d5acee4ddb10ae1dae5.

raman-m commented 2 months ago

Ready for delivery ✅

ggnaegi commented 2 months ago

@raman-m I'm not sure about FileCacheOptions being renamed to CacheOptions. I would prefer keeping FileCacheOptions for now. For me it was all sound and we are going now a step too far. I would prefer having those changes in another PR.

raman-m commented 2 months ago

@ggnaegi Understood! Reverting...

raman-m commented 2 months ago

@ggnaegi The recent changes have been reverted❕ The proof -> link I think we're ready to merge now. 😃 You could press the magic button 😜

ggnaegi commented 2 months ago

@raman-m Ok, I got it, I just realized I named the Global FileCacheOptions CacheOptions... Well... Are you ok with that, and we announce the community that in a next version we will rename it?

raman-m commented 2 months ago

TODO

thiagoloureiro commented 2 months ago

thanks for adding all the comments and commits guys!