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

[FIX JENKINS-43848] - Lack of cache-invalidation headers results in stale item list #2973

Merged
merged 6 commits into from
Aug 12, 2017
Merged

[FIX JENKINS-43848] - Lack of cache-invalidation headers results in stale item list #2973

merged 6 commits into from
Aug 12, 2017

Conversation

josiahhaswell
Copy link
Contributor

@josiahhaswell josiahhaswell commented Aug 7, 2017 •
edited
Loading

See JENKINS-43848.

Proposed changelog entries

  • Fix for JENKINS-43848: Browser caching response results in stale list
  • ...

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention
@reviewbybees

@ghost
Copy link

ghost commented Aug 7, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@josiahhaswell
Copy link
Contributor Author

josiahhaswell commented Aug 7, 2017 •
edited
Loading

There's some sort of whack white-space issue. Those are not showing up in my local diff. My Git settings are checkin with Unix line-feeds, checkout with local OS.

@oleg-nenashev
Copy link
Member

@josiahhaswell Which IDE you use just in case?

@daniel-beck
Copy link
Member

daniel-beck commented Aug 8, 2017 •
edited
Loading

@josiahhaswell It's not line endings, your IDE reformats empty (whitespace only) lines.

oleg-nenashev
oleg-nenashev previously requested changes Aug 8, 2017
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Although whitespace fixes are correct, they should not be fixed in the bugfix since they complicate backporting. Likely you use IDEA and hence need to configure it to be less smart :(

@kzantow
Copy link
Contributor

kzantow commented Aug 8, 2017

Also side note: if you use format: [FIX JENKINS-######] in the title, the issue tracker will automatically update things for you nicely

@oleg-nenashev oleg-nenashev changed the title Jenkins 43848: Lack of cache-invalidation headers results in stale item list [JENKINS-43848] - Lack of cache-invalidation headers results in stale item list Aug 8, 2017
@oleg-nenashev
Copy link
Member

@kzantow Yes, you are right. But we can always fix it by squash :)

@josiahhaswell
Copy link
Contributor Author

Ok. I had a version of IntelliJ with a bug wherein it would remove trailing whitespace no matter what was selected. Upgraded and fixed.

@josiahhaswell josiahhaswell changed the title [JENKINS-43848] - Lack of cache-invalidation headers results in stale item list [FIX JENKINS-43848] - Lack of cache-invalidation headers results in stale item list Aug 8, 2017
jglick
jglick approved these changes Aug 8, 2017
View reviewed changes
test/src/test/java/hudson/model/ViewTest.java Outdated
String resp = values.get("Cache-Control");
assertThat(resp, is("no cache, no store, must revalidate"));
assertThat(values.get("Expires"), is("0"));
assertThat(values.get("Pragma"), is("no-cache"));
Copy link
Member

Choose a reason for hiding this comment

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

FWIW easier to write:

WebResponse rsp = j.createWebClient().goTo("view/all/itemCategories", "application/json").getWebResponse();
assertEquals("no cache, no store, must revalidate", rsp.getResponseHeaderValue("Cache-Control"));
assertEquals("0", rsp.getResponseHeaderValue("Expires"));
assertEquals("no cache", rsp.getResponseHeaderValue("Pragma"));

@jglick jglick dismissed oleg-nenashev’s stale review August 8, 2017 21:03

requested changes made

oleg-nenashev
oleg-nenashev approved these changes Aug 11, 2017
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

🐝 and @reviewbybees done

oleg-nenashev
oleg-nenashev requested changes Aug 11, 2017
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Opps, test needs to be fixed, hence 🐛

Error
Expected: is "no cache, no store, must revalidate"
     but: was "no-cache, no-store, must-revalidate"
Stacktrace
java.lang.AssertionError: 
Expected: is "no cache, no store, must revalidate"
     but: was "no-cache, no-store, must-revalidate"
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.junit.Assert.assertThat(Assert.java:956)
	at org.junit.Assert.assertThat(Assert.java:923)
	at hudson.model.ViewTest.testNoCacheHeadersAreSet(ViewTest.java:108)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.jvnet.hudson.test.JenkinsRule$2.evaluate(JenkinsRule.java:547)
	at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:365)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:272)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:236)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:159)
	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:386)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:323)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:143)
Standard Output
=== Starting testNoCacheHeadersAreSet(hudson.model.ViewTest)
Standard Error
Aug 08, 2017 7:59:09 PM org.jvnet.hudson.test.JenkinsRule createWebServer
INFO: Running on http://localhost:41088/jenkins/
Aug 08, 2017 7:59:09 PM jenkins.InitReactorRunner$1 onAttained
INFO: Started initialization
Aug 08, 2017 7:59:09 PM jenkins.InitReactorRunner$1 onAttained
INFO: Listed all plugins
Aug 08, 2017 7:59:09 PM jenkins.InitReactorRunner$1 onAttained
INFO: Prepared all plugins
Aug 08, 2017 7:59:09 PM jenkins.InitReactorRunner$1 onAttained
INFO: Started all plugins
Aug 08, 2017 7:59:09 PM jenkins.InitReactorRunner$1 onAttained
INFO: Augmented all extensions
Aug 08, 2017 7:59:10 PM jenkins.InitReactorRunner$1 onAttained
INFO: Loaded all jobs
Aug 08, 2017 7:59:10 PM jenkins.InitReactorRunner$1 onAttained
INFO: Completed initialization
Aug 08, 2017 7:59:10 PM jenkins.model.Jenkins cleanUp
INFO: Stopping Jenkins
Aug 08, 2017 7:59:11 PM jenkins.model.Jenkins cleanUp
INFO: Jenkins stopped

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Aug 12, 2017
@oleg-nenashev oleg-nenashev merged commit 34bf393 into jenkinsci:master Aug 12, 2017
@josiahhaswell josiahhaswell deleted the JENKINS-43848 branch August 12, 2017 23:57
olivergondza pushed a commit that referenced this pull request Aug 24, 2017
@josiahhaswell @olivergondza
[FIX JENKINS-43848] - Lack of cache-invalidation headers results in s…
9a8fdb9 
…tale item list (#2973)

* Saving progress for review

* Adding licenses

* Adding integration test for headers

* Removing proposal for refactoring to method objects

* Removing whitespace changeswq

* Fixing test

(cherry picked from commit 34bf393)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick

@oleg-nenashev oleg-nenashev

Assignees
No one assigned
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@josiahhaswell @oleg-nenashev @daniel-beck @kzantow @jglick