You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2020/10/01 09:38:36 UTC
[GitHub] [sling-org-apache-sling-models-impl] cjelger opened a new pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
cjelger opened a new pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20
- use original sling request when caching models adapted from a wrapped request
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] raducotescu commented on pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
raducotescu commented on pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20#issuecomment-702223806
I was thinking that resources can also be wrapped and potentially other objects.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] sonarcloud[bot] commented on pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20#issuecomment-702020607
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&resolved=false&types=SEC
URITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100.png' alt='100.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-models-impl&pullRequest=20&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] cjelger commented on pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
cjelger commented on pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20#issuecomment-703574960
@justinedelson Thank you for the merge. It would be great if you could also release a new version since this issue pretty much prevents caching from working with HTL scripts? Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] cjelger commented on pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
cjelger commented on pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20#issuecomment-702017054
@justinedelson @raducotescu Can you please have a look at the PR? Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] cjelger edited a comment on pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
cjelger edited a comment on pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20#issuecomment-703574960
@justinedelson Thank you for the merge. It would be great if you could also release a new version since this issue pretty much prevents caching from working with HTL scripts. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] raducotescu commented on pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
raducotescu commented on pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20#issuecomment-702709764
@justinedelson, can you take over the PR and an eventual release?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] justinedelson merged pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
justinedelson merged pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] cjelger commented on pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
cjelger commented on pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20#issuecomment-702568534
> at least could not say whether it would be expected that for a wrapped resource if the cache should be keyed on the wrapper or the base.
I agree with @justinedelson, in the case of a wrapped resource it's not obvious what the expected caching behaviour should be, I also honestly cannot think of a "standard" use case that would involve wrapped resources and would clearly demonstrate that the cache key should always be the "base" resource.
In contrast and IMO, the issue with wrapped requests is straightforward: it is the consequence of the wrapping done by the scripting engine, and pretty much prevents caching from being used in an advanced scenario where multiple scripts are involved. I also don't see why anyone would expect the caching to be based on wrapped requests that are being added "behind the scene" without any control/visibility on what happens ... in this case and IMHO, it's clear that we want to use the base request for the cache key.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] raducotescu commented on pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
raducotescu commented on pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20#issuecomment-702043527
The changes look good to me for this particular instance of the issue, but I think that a similar problem could happen for any `adaptable` where the actual object is an implementation of a certain `Adaptable` interface.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] cjelger edited a comment on pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
cjelger edited a comment on pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20#issuecomment-702568534
> I at least could not say whether it would be expected that for a wrapped resource if the cache should be keyed on the wrapper or the base.
I agree with @justinedelson, in the case of a wrapped resource it's not obvious what the expected caching behaviour should be, I also honestly cannot think of a "standard" use case that would involve wrapped resources and would clearly demonstrate that the cache key should always be the "base" resource.
In contrast and IMO, the issue with wrapped requests is straightforward: it is the consequence of the wrapping done by the scripting engine, and pretty much prevents caching from being used in an advanced scenario where multiple scripts are involved. I also don't see why anyone would expect the caching to be based on wrapped requests that are being added "behind the scene" without any control/visibility on what happens ... in this case and IMHO, it's clear that we want to use the base request for the cache key.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] justinedelson commented on pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
justinedelson commented on pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20#issuecomment-702289460
bq. I was thinking that resources can also be wrapped and potentially other objects.
True, there is a `ResourceWrapper` but I at least could not say whether it would be expected that for a wrapped resource if the cache should be keyed on the wrapper or the base.
I'm also not sure that needs to be dealt with in this PR -- adding support for unwrapping resources could be dealt with at a later time should the desired behavior be clearly defined.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-models-impl] justinedelson commented on pull request #20: SLING-9781 - [Sling Models] Caching doesn't work with Wrapped requests
Posted by GitBox <gi...@apache.org>.
justinedelson commented on pull request #20:
URL: https://github.com/apache/sling-org-apache-sling-models-impl/pull/20#issuecomment-702062578
@raducotescu I think this is a specific case where request objects are very commonly wrapped by the scripting engine. Am I misunderstanding your concern?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org