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

Feature/httpcache responseheader exclusion #1905

Merged
merged 20 commits into from
May 28, 2019
Merged

Feature/httpcache responseheader exclusion #1905

merged 20 commits into from
May 28, 2019

Conversation

niekraaijmakers
Copy link
Contributor

Make it possible to exclude certain response headers (like login cookies etc) .

Throttle the storing of responses (todo item)

Cleaned up the HttpCacheEngine a little.

@niekraaijmakers niekraaijmakers requested review from justinedelson and davidjgonzalez and removed request for justinedelson May 23, 2019 07:07
@update-changelog
Copy link

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update the CHANGELOG.md file with this pull request.

@coveralls
Copy link

coveralls commented May 23, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 4503

  • 81 of 148 (54.73%) changed or added relevant lines in 6 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 52.864%
Changes Missing Coverage Covered Lines Changed/Added Lines %
bundle/src/main/java/com/adobe/acs/commons/httpcache/config/HttpCacheConfig.java 1 2 50.0%
bundle/src/main/java/com/adobe/acs/commons/httpcache/engine/CacheContent.java 5 9 55.56%
bundle/src/main/java/com/adobe/acs/commons/httpcache/engine/impl/HttpCacheEngineImpl.java 39 52 75.0%
bundle/src/main/java/com/adobe/acs/commons/httpcache/engine/impl/delegate/HttpCacheEngineBindingsDelegate.java 33 53 62.26%
bundle/src/main/java/com/adobe/acs/commons/httpcache/engine/impl/delegate/HttpCacheEngineMBeanDelegate.java 1 30 3.33%
Files with Coverage Reduction New Missed Lines %
bundle/src/main/java/com/adobe/acs/commons/mcp/impl/ProcessErrorReportExcelServlet.java 1 66.22%
bundle/src/main/java/com/adobe/acs/commons/httpcache/engine/impl/HttpCacheEngineImpl.java 1 52.98%
Totals Coverage Status
Change from base Build 4468: 0.03%
Covered Lines: 13482
Relevant Lines: 25503
💛 - Coveralls

@niekraaijmakers
Copy link
Contributor Author

1 out of 5 builds on travis push failed :/

@badvision
Copy link
Contributor

Can you open an issue for the random build fails? I think it's one of the fam tests having a race condition.

@badvision
Copy link
Contributor

In the meantime I restarted the build.

davidjgonzalez
davidjgonzalez requested changes May 23, 2019
View reviewed changes
Copy link
Contributor

@davidjgonzalez davidjgonzalez left a comment

Choose a reason for hiding this comment

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

few minors - i assume delegates are copy/paste without any real changes

bundle/src/main/java/com/adobe/acs/commons/httpcache/config/HttpCacheConfig.java Outdated Show resolved Hide resolved
bundle/src/main/java/com/adobe/acs/commons/httpcache/config/impl/HttpCacheConfigImpl.java Outdated Show resolved Hide resolved
bundle/src/main/java/com/adobe/acs/commons/httpcache/config/package-info.java Show resolved Hide resolved
bundle/src/main/java/com/adobe/acs/commons/httpcache/engine/CacheContent.java Outdated Show resolved Hide resolved
bundle/src/main/java/com/adobe/acs/commons/httpcache/engine/impl/HttpCacheEngineImpl.java Show resolved Hide resolved
bundle/src/main/java/com/adobe/acs/commons/httpcache/engine/impl/HttpCacheEngineImpl.java Outdated Show resolved Hide resolved
bundle/src/main/java/com/adobe/acs/commons/httpcache/engine/impl/HttpCacheEngineImpl.java Outdated Show resolved Hide resolved
@niekraaijmakers
Copy link
Contributor Author

I implemented the review comments (thanks!), please revisit :)

davidjgonzalez
davidjgonzalez reviewed May 24, 2019
View reviewed changes
Copy link
Contributor

@davidjgonzalez davidjgonzalez left a comment

Choose a reason for hiding this comment

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

One question about scope reduction - not sure that its needed.

Added a few code snippet suggestions in.

bundle/src/main/java/com/adobe/acs/commons/httpcache/engine/CacheContent.java Outdated Show resolved Hide resolved
bundle/src/main/java/com/adobe/acs/commons/httpcache/config/HttpCacheConfig.java Outdated Show resolved Hide resolved
bundle/src/main/java/com/adobe/acs/commons/httpcache/config/HttpCacheConfig.java Outdated Show resolved Hide resolved
bundle/src/main/java/com/adobe/acs/commons/httpcache/engine/CacheContent.java Outdated Show resolved Hide resolved
bundle/src/main/java/com/adobe/acs/commons/httpcache/config/HttpCacheConfig.java Show resolved Hide resolved
@niekraaijmakers
Copy link
Contributor Author

updated.. :) please review again

@davidjgonzalez davidjgonzalez merged commit 9a84c94 into master May 28, 2019
@niekraaijmakers niekraaijmakers deleted the feature/httpcache-responseheader-exclusion branch May 29, 2019 08:16
id-keenan added a commit to id-keenan/acs-aem-commons that referenced this pull request Jun 7, 2019
@id-keenan
Merge branch 'master' into feature/templated-dialogs
a56017a 
* master:
  Feature/dialog resource provider Adobe-Consulting-Services#1904 (Adobe-Consulting-Services#1922)
  Bugfix/cleanup filter xml (Adobe-Consulting-Services#1910)
  Update CHANGELOG.md (Adobe-Consulting-Services#1923)
  add javax.annotation dependency to pom as it was removed in java 11 (Adobe-Consulting-Services#1893)
  Feature/httpcache responseheader exclusion (Adobe-Consulting-Services#1905)
  Respect page inheritance in Named Transform Image Servlet for cq:Page (Adobe-Consulting-Services#1899)
  Allow uniform download links in JCR Compare (Adobe-Consulting-Services#1902)
  Feature/bugfix 1868 (Adobe-Consulting-Services#1906)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@badvision badvision badvision left review comments

@davidjgonzalez davidjgonzalez davidjgonzalez approved these changes

@justinedelson justinedelson Awaiting requested review from justinedelson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@niekraaijmakers @coveralls @badvision @davidjgonzalez