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.

Sign up for GitHub

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

Jump to bottom

#1172 Default CacheKeyGenerator #1849

Merged
merged 16 commits into from
Dec 14, 2023
Merged

#1172 Default CacheKeyGenerator #1849

merged 16 commits into from
Dec 14, 2023

Conversation

raman-m
Copy link
Member

@raman-m raman-m commented Dec 9, 2023 •
edited
Loading

Follows up #1172

Related to #1808

During 22.0 release feature #1172 was excluded from delivery list. Now it is time to deliver it once again with some enhancements after team's discussion (start).

Proposed Changes

  • Disable request content hashing for performance reasons
  • Introduce new EnableRequestBodyHashing property of the CacheOptions class
  • Make CacheKeyGenerator default via class renaming

EngRajabi and others added 5 commits December 9, 2023 14:56
@EngRajabi @raman-m
Cache by header value: a new Header property in (File)CacheOptions co…
075d8c0 
…nfiguration of a route (#1172)

@EngRajabi, Mohsen Rajabi (7):
      add header to file cache option
      fix private set
      fix
      <none>
      <none>
      fix build fail
      fix: fix review comment. add unit test for change

@raman-m, Raman Maksimchuk (1):
      Update caching.rst

@raman-m (7):
      Fix errors
      Fix errors
      Fix styling warnings
      Refactor tests
      Add Delimiter
      Refactor generator
      Add unit tests
@raman-m
Review after cherry pick
cd6720a
@raman-m
Some refactoring
30d6d48
@raman-m
Disable request body hashing
a92d65e
@raman-m
Make CacheKeyGenerator default
c12e812
@raman-m raman-m self-assigned this Dec 9, 2023
@raman-m raman-m requested a review from RaynaldM December 9, 2023 16:09
@raman-m raman-m added feature A new feature Caching Ocelot feature: Caching Nov'23 November 2023 release labels Dec 9, 2023
@raman-m raman-m added this to the November'23 milestone Dec 9, 2023
@raman-m
Copy link
Member Author

raman-m commented Dec 9, 2023 •
edited
Loading

@ggnaegi You are welcome to review code of this PR!

We have some dev plan on this:

  • your comments: 1st and 2nd
  • after merging this PR you'll create new PR on top of this code

Please review!
FYI I didn't extract the feature from DI aka Ocelot Builder. But it should be. Hope you will do that in the next PR.
The scope of work of this PR is delivery of Mohsen's feature with disabling ugly content hashing: no more.

@raman-m
Copy link
Member Author

raman-m commented Dec 9, 2023 •
edited
Loading

Should I add acceptance tests? : 😇 😋

ggnaegi
ggnaegi requested changes Dec 9, 2023
View reviewed changes
Copy link
Member

@ggnaegi ggnaegi left a comment

Choose a reason for hiding this comment

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

@raman-m a few changes i would recommend. Please no virtual, since we don't have any use cases with overridden properties and methods. The only use case here is testing...

src/Ocelot/Cache/DefaultCacheKeyGenerator.cs Outdated Show resolved Hide resolved
src/Ocelot/Request/Middleware/DownstreamRequest.cs Outdated Show resolved Hide resolved
src/Ocelot/Request/Middleware/DownstreamRequest.cs Outdated Show resolved Hide resolved
src/Ocelot/Request/Middleware/DownstreamRequest.cs Outdated Show resolved Hide resolved
src/Ocelot/Request/Middleware/DownstreamRequest.cs Outdated Show resolved Hide resolved
src/Ocelot/Request/Middleware/DownstreamRequest.cs Outdated Show resolved Hide resolved
test/Ocelot.UnitTests/Cache/DefaultCacheKeyGeneratorTests.cs Outdated Show resolved Hide resolved
test/Ocelot.UnitTests/Cache/DefaultCacheKeyGeneratorTests.cs Outdated Show resolved Hide resolved
test/Ocelot.UnitTests/Cache/DefaultCacheKeyGeneratorTests.cs Outdated Show resolved Hide resolved
test/Ocelot.UnitTests/Cache/DefaultCacheKeyGeneratorTests.cs Outdated Show resolved Hide resolved
@ggnaegi
Copy link
Member

ggnaegi commented Dec 9, 2023

Should I add acceptance tests? : 😇 😋

😇

@raman-m
Copy link
Member Author

raman-m commented Dec 12, 2023 •
edited
Loading

@ggnaegi suggested changes on Dec 9
@RaynaldM approved these changes on Dec 11

Code review issues have been fixed ✅
Caching Header is a new feature, so seems I have to add acceptance tests... But I've done with unit tests, they are written.

And could you review docs once again please?

@raman-m raman-m requested a review from ggnaegi December 12, 2023 18:56
@raman-m
Copy link
Member Author

raman-m commented Dec 13, 2023 •
edited
Loading

@ggnaegi commented on Dec 9

Acceptance test has been added! ✔️
See commit 3d536be
By default body content is not hashed.

@ggnaegi Looks good now?

@raman-m
Copy link
Member Author

raman-m commented Dec 13, 2023

Ready for delivery! ✔️

@raman-m raman-m merged commit ba641b2 into develop Dec 14, 2023
2 checks passed
@raman-m raman-m deleted the follow-up-1172 branch December 14, 2023 09:18
@ggnaegi ggnaegi mentioned this pull request Jan 9, 2024
Merged
This was referenced Feb 11, 2024
Merged
Merged
@raman-m raman-m mentioned this pull request Mar 23, 2024
Closed
@raman-m raman-m mentioned this pull request Nov 11, 2024
Open
@raman-m
Copy link
Member Author

raman-m commented Dec 13, 2024

Dear @wast, don't remove your comments: better to discuss what was happen. Anyway I have email notifications.

+        public bool EnableContentHashing { get; }

I think this was breaking change that causes wrong caches with POST requests with payload. Rolled back to version 21 where there is no issue. Haven't had time to confirm, but I'm 90% sure. Gonna try to add a unit test in a PR.

The problem was reported in #2054 #2059, and the bug was fixed by #2058 with merging on May 13, 2024, see commit 6e9a975 please. So, the fix was delivered in version 23.3.0.

Gonna try to add a unit test in a PR.

What is the purpose? The bug was fixed by #2058! Therefore, please use the latest version and specify global options. Enjoy!

@wast
Copy link
Contributor

wast commented Dec 13, 2024

Comment was deleted as it wasn't relevant anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggnaegi ggnaegi ggnaegi approved these changes

@RaynaldM RaynaldM RaynaldM approved these changes

Assignees

@raman-m raman-m

Labels
Caching Ocelot feature: Caching feature A new feature Nov'23 November 2023 release
Projects
None yet
Milestone
Nov-December'23
Development

Successfully merging this pull request may close these issues.
5 participants
@raman-m @ggnaegi @wast @RaynaldM @EngRajabi