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

Handle ApiVersion in CachingOperationInvoker #18961

Closed
wants to merge 1 commit into from

Conversation

dreis2211
Copy link
Contributor

@dreis2211 dreis2211 commented Nov 12, 2019

Hi,

this is a proposal for fixing #18959.

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.

Let me know what you think.
Cheers,
Christoph

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
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 12, 2019
@snicoll snicoll added type: regression A regression from a previous release 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
@dreis2211
Copy link
Contributor Author

Btw. I'm having trouble to see the concourse builds at the moment (it redirects me to a blank page). Might be related to the build-pipeline rename, but I thought I mention it at least ;-)

@wilkinsona
Copy link
Member

Thanks, @dreis2211. Could you try again now please?

@dreis2211
Copy link
Contributor Author

Works like a charm, @wilkinsona . Thanks 👍

@snicoll snicoll self-assigned this Nov 21, 2019
snicoll pushed a commit that referenced this pull request Nov 21, 2019
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.

See gh-18961
@snicoll snicoll closed this in 0b4dcf9 Nov 21, 2019
@snicoll snicoll modified the milestones: 2.2.x, 2.2.2 Nov 21, 2019
@snicoll
Copy link
Member

snicoll commented Nov 21, 2019

Thanks again Christoph!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants