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 2021/06/02 16:44:50 UTC
[GitHub] [sling-org-apache-sling-resourceresolver] henrykuijpers opened a new pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
henrykuijpers opened a new pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46
--
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-resourceresolver] jsedding commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
jsedding commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r644618310
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1032,34 +1070,33 @@ private boolean addEntry(final Map<String, List<MapEntry>> entryMap, final Strin
/*
* Update alias query based on configured alias locations
*/
- private String updateAliasQuery(){
+ private String createAliasQuery(){
final Set<String> allowedLocations = this.factory.getAllowedAliasLocations();
- StringBuilder baseQuery = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
- baseQuery.append(" ").append("WHERE");
+ StringBuilder query = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
+ query.append(" ").append("WHERE");
if(allowedLocations.isEmpty()){
- String jcrSystemPath = StringUtils.removeEnd(JCR_SYSTEM_PREFIX, "/");
- baseQuery.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
- .append("\"").append(jcrSystemPath).append("\"").append("))");
+ query.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
+ .append("\"").append(JCR_SYSTEM_PATH).append("\"").append("))");
Review comment:
It would be nice to reformat this query to use single quotes instead of escaped double quotes. Then both queries share the same style, and it is more readable.
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1255,6 +1279,33 @@ private boolean loadVanityPath(final Resource resource, final Map<String, List<M
return hasVanityPath;
}
+ private String createVanityPathQuery() {
+ final String query =
+ VANITY_PATH_BASE_QUERY_DEFAULT +
+ " WHERE sling:vanityPath IS NOT NULL" +
+ Stream
+ .of(true, false)
+ .map(this::createVanityPathQueryPathRestriction)
Review comment:
After reading the code generating the restrictions several times, I believe it is actually correct. But it is not very readable or intuitive.
Maybe it would become more readable if you separated out all "include" restrictions and all "exclude" restrictions, each into their own collection, and then join them with the appropriate "AND" or "OR" operators?
Pseudo code:
```
includesExcludes = Stream.of(new VanityPathConfig(JCR_SYSTEM_PATH, true), factory.getVanityPathConfig().stream())
.collect(Collectors.groupingBy(VanityPathConfig::isExclude)
excludes = includesExclude.get(Boolean.TRUE)
includes = includesExclude.get(Boolean.FALSE)
excludesPart =excludes.stream().map("NOT ISDESCENDANTNODE(page, '%s')").join(" AND ")
includesPart = includes.stream().map("ISDESCENDANTNODE(page, '%s')").join(" OR ")
queryRestrictions = String.format("(%s) AND (%s)", excludesPart, includesPart)
```
Beware, I may have gotten the "OR"s and "AND"s wrong ;)
PS: maybe also move `new VanityPathConfig(JCR_SYSTEM_PATH, true)` into `factory.getVanityPathConfig()`?
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java
##########
@@ -112,11 +112,6 @@
"the ResourceResolver mapping. The default value is /etc/map.")
String resource_resolver_map_location() default MapEntries.DEFAULT_MAP_ROOT;
- @AttributeDefinition(name = "Mapping Observation",
Review comment:
I don't understand why you are removing this configuration option. Is this actually related to improving the vanity path query? Or is this a different change?
--
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-resourceresolver] sonarcloud[bot] removed a comment on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-853214408
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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY)
[<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT)
[<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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_coverage&view=list) [94.1% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list)
--
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-resourceresolver] rombert commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
rombert commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r649981504
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
I would be a bit cautious with pulling in too much into this test module. Bring in Sling Mocks in low-level modules has in my experience lead to various classpath clashes and overriding of depedencies.
I think validating the query is better done at the integration test level. Or maybe, if we want to be sure, we might want to try importing a query parser from Oak and checking that the query is valid. But I think that running the full query is too much at this level.
--
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-resourceresolver] joerghoh commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
joerghoh commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r649964644
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
You could use SlingContext and back it for the test with a Oak Repo.
--
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-resourceresolver] bdelacretaz commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r649806943
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
Although you are checking that the query is what you expect, unless I missed something there's no test that validates the query syntax? I understand this is similar to the existing tests but I think it would be better to run that query against a content repository provided by Sling JCR Mocks and validate the results.
I don't know how much time you have for that - this way of testing probably works for now but IMHO running the query would create a test that's easier to maintain and understand. Maybe factor out the query generation to its own package-local helper class to avoid issues with private methods.
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
If other reviewers agree to merge this with the tests as is I will not object. Maybe just add a comment to the test explaining that actually running the query would be better?
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
The [jcr/repoinit/ExecutionOrderTest](https://github.com/apache/sling-org-apache-sling-jcr-repoinit/blob/master/src/test/java/org/apache/sling/jcr/repoinit/ExecutionOrderTest.java) is an example using SlingContext. I agree with Robert that such things might be better tested in integration tests, and as mentioned I won't object to committing the code without a query validation test.
--
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-resourceresolver] cziegeler commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r691402791
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -707,7 +748,7 @@ public void onChange(final List<ResourceChange> changes) {
log.debug("onChange, type={}, path={}", rc.getType(), path);
// don't care for system area
- if (path.startsWith(JCR_SYSTEM_PREFIX)) {
+ if (path.startsWith(JCR_SYSTEM_PATH + '/')) {
Review comment:
while this is not a big deal, why did you change this from a constant to an expression that is executed each time?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
rombert commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r650787725
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java
##########
@@ -112,11 +112,6 @@
"the ResourceResolver mapping. The default value is /etc/map.")
String resource_resolver_map_location() default MapEntries.DEFAULT_MAP_ROOT;
- @AttributeDefinition(name = "Mapping Observation",
Review comment:
Might also be worth logging a warning if a non-default setting is found.
--
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-resourceresolver] sonarcloud[bot] removed a comment on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-853227172
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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY)
[<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT)
[<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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_coverage&view=list) [94.1% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list)
--
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-resourceresolver] henrykuijpers commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r645514016
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1255,6 +1279,33 @@ private boolean loadVanityPath(final Resource resource, final Map<String, List<M
return hasVanityPath;
}
+ private String createVanityPathQuery() {
+ final String query =
+ VANITY_PATH_BASE_QUERY_DEFAULT +
+ " WHERE sling:vanityPath IS NOT NULL" +
+ Stream
+ .of(true, false)
+ .map(this::createVanityPathQueryPathRestriction)
Review comment:
I just had another go at it, @jsedding , does this already look a bit better?
--
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-resourceresolver] henrykuijpers commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r649823690
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
As far as I know, the execution of querys isn't implemented anywhere in the testing code that we have a available. IIUC, we need an actual instance to be able to execute querys and thus validating the query syntax and also the proper execution of the query.
What I've done is indeed making sure that the expected query is being executed.
I think factoring out query code etc etc might be one mile too far for what we're trying to achieve in this ticket? For us, given our big version storage, it's quite important that those paths will not be considered anymore.
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
Just added that comment anyway. :)
@bdelacretaz Do you know of a way to test querys with a real query engine, without booting a Sling instance?
--
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-resourceresolver] henrykuijpers commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r645513718
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1032,34 +1070,33 @@ private boolean addEntry(final Map<String, List<MapEntry>> entryMap, final Strin
/*
* Update alias query based on configured alias locations
*/
- private String updateAliasQuery(){
+ private String createAliasQuery(){
final Set<String> allowedLocations = this.factory.getAllowedAliasLocations();
- StringBuilder baseQuery = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
- baseQuery.append(" ").append("WHERE");
+ StringBuilder query = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
+ query.append(" ").append("WHERE");
if(allowedLocations.isEmpty()){
- String jcrSystemPath = StringUtils.removeEnd(JCR_SYSTEM_PREFIX, "/");
- baseQuery.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
- .append("\"").append(jcrSystemPath).append("\"").append("))");
+ query.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
+ .append("\"").append(JCR_SYSTEM_PATH).append("\"").append("))");
Review comment:
I just changed this to use single quotes.
--
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-resourceresolver] cziegeler commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r691402791
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -707,7 +748,7 @@ public void onChange(final List<ResourceChange> changes) {
log.debug("onChange, type={}, path={}", rc.getType(), path);
// don't care for system area
- if (path.startsWith(JCR_SYSTEM_PREFIX)) {
+ if (path.startsWith(JCR_SYSTEM_PATH + '/')) {
Review comment:
while this is not a big deal, why did you change this from a constant to an expression that is executed each time?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
rombert commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r650787399
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java
##########
@@ -112,11 +112,6 @@
"the ResourceResolver mapping. The default value is /etc/map.")
String resource_resolver_map_location() default MapEntries.DEFAULT_MAP_ROOT;
- @AttributeDefinition(name = "Mapping Observation",
Review comment:
I think it may be confusing to remove this setting. Perhaps mark it as deprecated and indicate that it is actually auto-computed?
--
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-resourceresolver] henrykuijpers commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r691306477
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
Indeed, it looks like `ResourceResolverType.JCR_OAK` is a solution that can actually parse and execute a query. Very cool! I've been using it in the project I'm working on and that's really nice. It's also nice that I was able to get com.day.cq.search working in the unit test, to test the predicate search that we're executing. But that's something outside Sling, of course.
I've created https://issues.apache.org/jira/browse/SLING-10742 to tackle this.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-resourceresolver] henrykuijpers commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r649250780
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java
##########
@@ -516,6 +494,16 @@ public ResourceProviderTracker getResourceProviderTracker() {
return authenticationInfo;
}
+ @Override
Review comment:
Updated
--
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-resourceresolver] sonarcloud[bot] commented on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-853265748
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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY)
[<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT)
[<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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.9%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_coverage&view=list) [94.9% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list)
--
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-resourceresolver] sonarcloud[bot] removed a comment on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-854653413
SonarCloud Quality Gate failed.
[<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-resourceresolver&pullRequest=46&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/B.png' alt='B' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=BUG) [2 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY)
[<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT)
[<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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='95.2%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_coverage&view=list) [95.2% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list)
--
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-resourceresolver] jsedding commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
jsedding commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r644618310
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1032,34 +1070,33 @@ private boolean addEntry(final Map<String, List<MapEntry>> entryMap, final Strin
/*
* Update alias query based on configured alias locations
*/
- private String updateAliasQuery(){
+ private String createAliasQuery(){
final Set<String> allowedLocations = this.factory.getAllowedAliasLocations();
- StringBuilder baseQuery = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
- baseQuery.append(" ").append("WHERE");
+ StringBuilder query = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
+ query.append(" ").append("WHERE");
if(allowedLocations.isEmpty()){
- String jcrSystemPath = StringUtils.removeEnd(JCR_SYSTEM_PREFIX, "/");
- baseQuery.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
- .append("\"").append(jcrSystemPath).append("\"").append("))");
+ query.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
+ .append("\"").append(JCR_SYSTEM_PATH).append("\"").append("))");
Review comment:
It would be nice to reformat this query to use single quotes instead of escaped double quotes. Then both queries share the same style, and it is more readable.
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1255,6 +1279,33 @@ private boolean loadVanityPath(final Resource resource, final Map<String, List<M
return hasVanityPath;
}
+ private String createVanityPathQuery() {
+ final String query =
+ VANITY_PATH_BASE_QUERY_DEFAULT +
+ " WHERE sling:vanityPath IS NOT NULL" +
+ Stream
+ .of(true, false)
+ .map(this::createVanityPathQueryPathRestriction)
Review comment:
After reading the code generating the restrictions several times, I believe it is actually correct. But it is not very readable or intuitive.
Maybe it would become more readable if you separated out all "include" restrictions and all "exclude" restrictions, each into their own collection, and then join them with the appropriate "AND" or "OR" operators?
Pseudo code:
```
includesExcludes = Stream.of(new VanityPathConfig(JCR_SYSTEM_PATH, true), factory.getVanityPathConfig().stream())
.collect(Collectors.groupingBy(VanityPathConfig::isExclude)
excludes = includesExclude.get(Boolean.TRUE)
includes = includesExclude.get(Boolean.FALSE)
excludesPart =excludes.stream().map("NOT ISDESCENDANTNODE(page, '%s')").join(" AND ")
includesPart = includes.stream().map("ISDESCENDANTNODE(page, '%s')").join(" OR ")
queryRestrictions = String.format("(%s) AND (%s)", excludesPart, includesPart)
```
Beware, I may have gotten the "OR"s and "AND"s wrong ;)
PS: maybe also move `new VanityPathConfig(JCR_SYSTEM_PATH, true)` into `factory.getVanityPathConfig()`?
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java
##########
@@ -112,11 +112,6 @@
"the ResourceResolver mapping. The default value is /etc/map.")
String resource_resolver_map_location() default MapEntries.DEFAULT_MAP_ROOT;
- @AttributeDefinition(name = "Mapping Observation",
Review comment:
I don't understand why you are removing this configuration option. Is this actually related to improving the vanity path query? Or is this a different change?
--
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-resourceresolver] rombert commented on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
rombert commented on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-853613164
Thanks for the PR @henrykuijpers ! I'll not merge it immediately as @kwin has some concerns on whether this is the right thing to do.
If we end up going this way, we will need the branch to be rebased as it has conflicts.
--
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-resourceresolver] sonarcloud[bot] commented on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-858680632
SonarCloud Quality Gate failed.
[<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-resourceresolver&pullRequest=46&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/B.png' alt='B' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=BUG) [2 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY)
[<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT)
[<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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='95.3%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_coverage&view=list) [95.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list)
--
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-resourceresolver] henrykuijpers commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r691306477
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
Indeed, it looks like `ResourceResolverType.JCR_OAK` is a solution that can actually parse and execute a query. Very cool! I've been using it in the project I'm working on and that's really nice. It's also nice that I was able to get com.day.cq.search working in the unit test, to test the predicate search that we're executing. But that's something outside Sling, of course.
I've created https://issues.apache.org/jira/browse/SLING-10742 to tackle this.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-resourceresolver] henrykuijpers commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r691913953
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -707,7 +748,7 @@ public void onChange(final List<ResourceChange> changes) {
log.debug("onChange, type={}, path={}", rc.getType(), path);
// don't care for system area
- if (path.startsWith(JCR_SYSTEM_PREFIX)) {
+ if (path.startsWith(JCR_SYSTEM_PATH + '/')) {
Review comment:
That is an oversight: I think I inlined a few values, I'll change it back to a constant.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-resourceresolver] sonarcloud[bot] commented on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-853214408
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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY)
[<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT)
[<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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_coverage&view=list) [94.1% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list)
--
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-resourceresolver] henrykuijpers commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r644635245
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java
##########
@@ -112,11 +112,6 @@
"the ResourceResolver mapping. The default value is /etc/map.")
String resource_resolver_map_location() default MapEntries.DEFAULT_MAP_ROOT;
- @AttributeDefinition(name = "Mapping Observation",
Review comment:
The mapping observation configuration setting is used to register the `ResourceChangeListener`, so that the `MapEntries` will be notified of changes that it is interested in. In AEM, it is set to `/`.
However, given the various configurations that exist, it is easy to compute the right paths to use. (That is also changed in the part where the listener is registered.)
There are also some fallbacks in there to listen to `/` anyway, if that is required.
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1255,6 +1279,33 @@ private boolean loadVanityPath(final Resource resource, final Map<String, List<M
return hasVanityPath;
}
+ private String createVanityPathQuery() {
+ final String query =
+ VANITY_PATH_BASE_QUERY_DEFAULT +
+ " WHERE sling:vanityPath IS NOT NULL" +
+ Stream
+ .of(true, false)
+ .map(this::createVanityPathQueryPathRestriction)
Review comment:
Good points! I will have another look at this. Also, a good point about moving the `/jcr:system`-exclude to the factory.
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1032,34 +1070,33 @@ private boolean addEntry(final Map<String, List<MapEntry>> entryMap, final Strin
/*
* Update alias query based on configured alias locations
*/
- private String updateAliasQuery(){
+ private String createAliasQuery(){
final Set<String> allowedLocations = this.factory.getAllowedAliasLocations();
- StringBuilder baseQuery = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
- baseQuery.append(" ").append("WHERE");
+ StringBuilder query = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
+ query.append(" ").append("WHERE");
if(allowedLocations.isEmpty()){
- String jcrSystemPath = StringUtils.removeEnd(JCR_SYSTEM_PREFIX, "/");
- baseQuery.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
- .append("\"").append(jcrSystemPath).append("\"").append("))");
+ query.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
+ .append("\"").append(JCR_SYSTEM_PATH).append("\"").append("))");
Review comment:
Agree, I'll do that. I tried to touch as less as possible, but I very much agree with this.
--
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-resourceresolver] sonarcloud[bot] removed a comment on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-853265748
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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY)
[<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT)
[<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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.9%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_coverage&view=list) [94.9% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list)
--
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-resourceresolver] sonarcloud[bot] commented on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-854653413
SonarCloud Quality Gate failed.
[<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-resourceresolver&pullRequest=46&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/B.png' alt='B' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=BUG) [2 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY)
[<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT)
[<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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='95.2%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_coverage&view=list) [95.2% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list)
--
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-resourceresolver] henrykuijpers commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r644637033
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1032,34 +1070,33 @@ private boolean addEntry(final Map<String, List<MapEntry>> entryMap, final Strin
/*
* Update alias query based on configured alias locations
*/
- private String updateAliasQuery(){
+ private String createAliasQuery(){
final Set<String> allowedLocations = this.factory.getAllowedAliasLocations();
- StringBuilder baseQuery = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
- baseQuery.append(" ").append("WHERE");
+ StringBuilder query = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
+ query.append(" ").append("WHERE");
if(allowedLocations.isEmpty()){
- String jcrSystemPath = StringUtils.removeEnd(JCR_SYSTEM_PREFIX, "/");
- baseQuery.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
- .append("\"").append(jcrSystemPath).append("\"").append("))");
+ query.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
+ .append("\"").append(JCR_SYSTEM_PATH).append("\"").append("))");
Review comment:
Agree, I'll do that. I tried to touch as less as possible, but I very much agree with this.
--
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-resourceresolver] rombert commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
rombert commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r650747694
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
For validating queries we can probably wrap the logic from Oak's QueryEngineImpl#parseQuery ( https://github.com/apache/jackrabbit-oak/blob/fa54797c66e262d07e6e139526855ae8cb800bd1/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineImpl.java#L144-L230 ), which basically delegates to the `SQL2Parser` and `XPathToSQL2Converter` classes. Those classes look pretty self-contained to me and we should be able to stub any dependencies they have - they don't seem used.
To me, this looks like a good idea for a separate module or bundle, so we can reuse it in other places where it may be desirable to do so.
--
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-resourceresolver] sonarcloud[bot] removed a comment on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-858680632
--
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-resourceresolver] cziegeler commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r649177296
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java
##########
@@ -516,6 +494,16 @@ public ResourceProviderTracker getResourceProviderTracker() {
return authenticationInfo;
}
+ @Override
Review comment:
Can we maybe use more inclusive language here like allow/deny list?
--
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-resourceresolver] rombert commented on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
rombert commented on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-853613164
Thanks for the PR @henrykuijpers ! I'll not merge it immediately as @kwin has some concerns on whether this is the right thing to do.
If we end up going this way, we will need the branch to be rebased as it has conflicts.
--
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-resourceresolver] henrykuijpers commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r644635245
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java
##########
@@ -112,11 +112,6 @@
"the ResourceResolver mapping. The default value is /etc/map.")
String resource_resolver_map_location() default MapEntries.DEFAULT_MAP_ROOT;
- @AttributeDefinition(name = "Mapping Observation",
Review comment:
The mapping observation configuration setting is used to register the `ResourceChangeListener`, so that the `MapEntries` will be notified of changes that it is interested in. In AEM, it is set to `/`.
However, given the various configurations that exist, it is easy to compute the right paths to use. (That is also changed in the part where the listener is registered.)
There are also some fallbacks in there to listen to `/` anyway, if that is required.
--
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-resourceresolver] sonarcloud[bot] commented on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-901871477
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_coverage&view=list) [95.4% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-resourceresolver] sonarcloud[bot] commented on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-859378239
--
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-resourceresolver] henrykuijpers commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r644636666
##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1255,6 +1279,33 @@ private boolean loadVanityPath(final Resource resource, final Map<String, List<M
return hasVanityPath;
}
+ private String createVanityPathQuery() {
+ final String query =
+ VANITY_PATH_BASE_QUERY_DEFAULT +
+ " WHERE sling:vanityPath IS NOT NULL" +
+ Stream
+ .of(true, false)
+ .map(this::createVanityPathQueryPathRestriction)
Review comment:
Good points! I will have another look at this. Also, a good point about moving the `/jcr:system`-exclude to the factory.
--
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-resourceresolver] sonarcloud[bot] commented on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-853227172
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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=VULNERABILITY)
[<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [<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/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=SECURITY_HOTSPOT)
[<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-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='94.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_coverage&view=list) [94.1% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&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-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver&pullRequest=46&metric=new_duplicated_lines_density&view=list)
--
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-resourceresolver] henrykuijpers commented on pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
Posted by GitBox <gi...@apache.org>.
henrykuijpers commented on pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#issuecomment-854651064
Just did a commit with some fixes, @jsedding . :) Can you have another look?
--
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