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

CIF-1653 - TTL-based caching is skipped by the SpecificPageFilterFactory #412

Merged
merged 1 commit into from
Oct 1, 2020

Conversation

cjelger
Copy link
Contributor

@cjelger cjelger commented Sep 30, 2020

  • configure filter ranking to lowest possible value, to avoid skipping the DispatcherCacheHeaderFilter filters from ACS-Commons

Motivation and Context

When it matches, the SpecificPageFilterFactory forwards the request to another page, and this "breaks" the chain of filters. We hence set the ranking to the lowest possible value, in particular to avoid skipping the TTL-based filters.

How Has This Been Tested?

Manually tested with dispatcher and ACS-Commons filters.

Screenshots (if appropriate):

Screenshot 2020-09-30 at 12 59 25

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes and the overall coverage did not decrease.
  • All unit tests pass on CircleCi.
  • I ran all tests locally and they pass.

- configure filter ranking to lowest possible value, to avoid skipping the DispatcherCacheHeaderFilter filters from ACS-Commons
@cjelger cjelger added the bug Something isn't working label Sep 30, 2020
@codecov
Copy link

codecov bot commented Sep 30, 2020

Codecov Report

Merging #412 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #412   +/-   ##
=========================================
  Coverage     86.11%   86.11%           
  Complexity      950      950           
=========================================
  Files           188      188           
  Lines          4918     4918           
  Branches        697      697           
=========================================
  Hits           4235     4235           
  Misses          541      541           
  Partials        142      142           
Flag Coverage Δ Complexity Δ
#integration 67.29% <ø> (ø) 691.00 <ø> (ø)
#jest 81.55% <ø> (ø) 0.00 <ø> (ø)
#karma 94.24% <ø> (ø) 0.00 <ø> (ø)
#unittests 86.29% <ø> (ø) 919.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...s/internal/servlets/SpecificPageFilterFactory.java 90.90% <ø> (ø) 6.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b33d20...004a605. Read the comment docs.

Copy link
Contributor

@mhaack mhaack left a comment

Choose a reason for hiding this comment

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

Not sure if Integer.MIN_VALUE is the best here but I trust you tested this out ;-)

@cjelger
Copy link
Contributor Author

cjelger commented Sep 30, 2020

@mhaack I added a screenshot, the Integer.MIN_VALUE is used by other filters that don't need to be processed with any high priority, and yes it works fine for us.

@mhaack
Copy link
Contributor

mhaack commented Sep 30, 2020

and yes it works fine for us.

I was expecting this :-) I was just curious.

@herzog31 herzog31 removed their request for review October 1, 2020 07:01
@herzog31 herzog31 merged commit 0d8e81d into master Oct 1, 2020
@herzog31 herzog31 deleted the CIF-1653 branch October 1, 2020 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working verified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants