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

Setting health endpoint cache time-to-live does not actually use CachingOperationInvoker #18959

Closed
temesoft opened this issue Nov 11, 2019 · 5 comments
Closed

Setting health endpoint cache time-to-live does not actually use CachingOperationInvoker #18959

temesoft opened this issue Nov 11, 2019 · 5 comments
Labels
type: regression A regression from a previous release

Comments

@temesoft
Copy link

When setting management.endpoint.health.cache.time-to-live=60s configuration to specify caching of health endpoint response - it seems not to take an effect in 2.2.0 release version. When requesting health endpoint ReflectiveOperationInvoker is being called instead of CachingOperationInvoker.

Attached, is a sample spring-boot application with a JUnit test that shows this issue.

my-app.zip

P.S. When switching to use an older version (for example 2.0.3) the health endpoint response is cached correctly.

Thank you
-Dmitriy
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 11, 2019
@dreis2211
Copy link
Contributor

Hi @temesoft . I think I'm on to something. Not a team member, but give me until this morning to try out my idea.

@dreis2211
Copy link
Contributor

dreis2211 commented Nov 12, 2019 •
edited

So here's what I found so far. The problem comes from the fact that ApiVersion is treated as a mandatory parameter in CachingOperationInvokerAdvisor and thus breaks the caching. One could simply handle ApiVersion the same way SecurityContext is handled, but that would probably return wrong results if request A has ApiVersion.V3 specified and request B has ApiVersion.V2.

This was likely introduced with #17929.

@snicoll snicoll added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 12, 2019
@snicoll snicoll added this to the 2.2.x milestone Nov 12, 2019
@snicoll
Copy link
Member

snicoll commented Nov 12, 2019

Thank you both. We'll have a look to handle ApiVersion so that it doesn't break caching.

@wilkinsona wilkinsona added type: regression A regression from a previous release and removed type: bug A general bug labels Nov 12, 2019
dreis2211 added a commit to dreis2211/spring-boot that referenced this issue Nov 12, 2019
@dreis2211
Handle ApiVersion in CachingOperationInvoker
e9ae765 
Prior to this commit, ApiVersion was treated as a mandatory parameter in
CachingOperationInvokerAdvisor and thus prevented the
CachingOperationInvoker to kick in. By skipping ApiVersion in the same
way we're skipping SecurityContext we can avoid this.

In order to not return the same cached response, this commit also
changes the cache handling in CachingOperationInvoker to account for
different ApiVersions being passed.

Closes spring-projectsgh-18959
@dreis2211 dreis2211 mentioned this issue Nov 12, 2019
Closed
@dreis2211
Copy link
Contributor

Made a proposal fix in #18961

@snicoll
Copy link
Member

snicoll commented Nov 12, 2019

Closing in favour or PR #18961

@snicoll snicoll closed this as completed Nov 12, 2019
@snicoll snicoll removed this from the 2.2.x milestone Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: regression A regression from a previous release
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@snicoll @wilkinsona @temesoft @dreis2211 @spring-projects-issues