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

FileCacheOptions not working after the header was introduced in FileCache settings in version 23.0.0 #2059

Closed
SheruGaur opened this issue May 2, 2024 Discussed in #2054 · 4 comments · Fixed by #2058
Closed
Assignees
Labels
bug Identified as a potential bug Caching Ocelot feature: Caching highest Highest priority Mar-Apr'24 March-April 2024 release merged Issue has been merged to dev and is waiting for the next release

Comments

@SheruGaur
Copy link

Discussed in #2054

Hi

We are facing issue regarding cache option, however in earlier version till v22.0.1 FileCacheOptions was working fine but after the release of v23.0.0 we are facing issue mentioned below in two cases.

Case 1: V.22.0.1
"FileCacheOptions": { "TtlSeconds": 15, "Region": "europe-central" }

It was working fine with the multiple requests. before the latest update
case2:
After the addition of new parameter named Header in FileCacheOptions there, we are facing issue on multiple requests in case one request is already generated and in the same configured cache expiry time if another request come with different values, then we are getting same response as per the first request.

Case 2: v23.2.2
"FileCacheOptions": {
"TtlSeconds": 15,
"Region": "europe-central"
}

As per documentation header is not mandatory, hence the cache option should work without header also, which is not the case.

Observed Behaviour

  1. File cache returns same data for different request payload on same Route in new version 23.2.2 (we tested in this version directly)
  2. File cache returns different data for different request payload on same Route in version 22.0.1.

Please can you help us fix this as this seems to be a breaking change since version 23.0.0 as mentioned by Raman in the discussion.

Thanks,
Sheru Kumar Gaur

@sberwal
Copy link

sberwal commented May 2, 2024

Hi Team

we have also observed the same behaviour. It seems the request payload is not considered when caching the data.

Hence cache returns the data for 1st request payload from the cache, for subsequent requests with different request payload.

This is happening when Header is not set!

Thanks
Suraj

@raman-m
Copy link
Member

raman-m commented May 2, 2024

@SheruGaur

Please can you help us fix this as this seems to be a breaking change since version 23.0.0 as mentioned by Raman in the discussion.

Certainly...

  1. Can you provide the specific lines of code where the breaking change occurs?
  2. We are willing to assist you if you can contribute further by demonstrating the precise breaking change in a PR, could you do that?

@raman-m raman-m added needs validation Issue has not been replicated or verified yet needs feedback Issue is waiting on feedback before acceptance waiting Waiting for answer to question or feedback from issue raiser labels May 2, 2024
@ggnaegi
Copy link
Member

ggnaegi commented May 9, 2024

@raman-m it's probably related to the fact that the body hashing is now disabled by default and it must be activated explicitly. But, as mentioned by @thiagoloureiro, the needed property isn't available in file configuration. So, I would recommend to deal with this issue before anything else...

@ggnaegi ggnaegi added accepted Bug or feature would be accepted as a PR or is being worked on Caching Ocelot feature: Caching critical Critical priority, higher than Highest. It requires hotfix ASAP Mar-Apr'24 March-April 2024 release and removed needs validation Issue has not been replicated or verified yet needs feedback Issue is waiting on feedback before acceptance waiting Waiting for answer to question or feedback from issue raiser labels May 10, 2024
@raman-m raman-m changed the title FileCacheOption not working after the header was introduced in FileCache settings in Ocelot V23.0.0 FileCacheOptions not working after the header was introduced in FileCache settings in version 23.0.0 May 11, 2024
@raman-m raman-m added highest Highest priority bug Identified as a potential bug and removed critical Critical priority, higher than Highest. It requires hotfix ASAP labels May 11, 2024
@raman-m raman-m added this to the March-April'24 milestone May 11, 2024
@raman-m
Copy link
Member

raman-m commented May 11, 2024

@ggnaegi commented on May 9

Understood! I guess we need to link the PR 2058 ... The bug fix will be released in current milestone. I don't want to make separate hotfix release: developers may to rollback to previous versions temporarily. So, the bugfix will be a part of version 23.3

P.S.

public FileCacheOptions FileCacheOptions { get; set; }

Firstly added in commit 47afc85#diff-e1943063e63a6438191010c566111bea5c47f4cb063c04c7cc6c91a8c4267812 by Tom many many years ago.
I wonder the property was named with File prefix. And the most appropriate name is CacheOptions.
Will renaming be a breaking change?
It will be better to rename 👉

- public FileCacheOptions FileCacheOptions { get; set; }
+ public FileCacheOptions CacheOptions { get; set; }

ggnaegi added a commit that referenced this issue May 13, 2024
…ions` (#2058)

* EnableContentHashing not being considered from appsettings

* Adding CacheOptionsCreator, Injected IRegionCreator as Singleton. Should still add some acceptance tests that are definitely missing!

* Adding caching global configuration since we messed up, ignoring an important breaking change with EnableContentHashing set to false by default

* Adding some further acceptance tests, validating EnableContentHashing, validating global config too.

* removing some debug content

* TtlSeconds must be set

* updating documentation

* Update docs/features/caching.rst

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* Update docs/features/caching.rst

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* Removing RegionCreator, moving service collection extension method to dependencyInjection\Features etc.

* adding unit tests for FileCacheOptions

* some more null tests...

* slight refactoring, updating ICacheOptionsCreator signature

* some more design refactoring

* Update src/Ocelot/Configuration/Creator/CacheOptionsCreator.cs

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* Code review by @raman-m

* Rename `FileCacheOptions` -> `CacheOptions`

* Subtly transition to `CacheOptions`, ensuring compatibility with `FileCacheOptions` to avoid a breaking change

* Not obsolete

---------

Co-authored-by: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com>
Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release and removed accepted Bug or feature would be accepted as a PR or is being worked on labels May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug Caching Ocelot feature: Caching highest Highest priority Mar-Apr'24 March-April 2024 release 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.

4 participants