Re: [PR] SLING-12124 - Inconsistent handling of empty selectors [sling-org-apache-sling-engine]
cziegeler merged PR #40: URL: https://github.com/apache/sling-org-apache-sling-engine/pull/40 -- 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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12124 - Inconsistent handling of empty selectors [sling-org-apache-sling-engine]
cziegeler commented on code in PR #40:
URL:
https://github.com/apache/sling-org-apache-sling-engine/pull/40#discussion_r1387629421
##
src/main/java/org/apache/sling/engine/impl/request/RequestData.java:
##
@@ -507,7 +507,12 @@ public static void service(SlingHttpServletRequest request,
SlingHttpServletResponse response) throws IOException,
ServletException {
-if (!isValidRequest(request.getRequestPathInfo().getResourcePath(),
request.getRequestPathInfo().getSelectors())) {
+final String selectorString =
request.getRequestPathInfo().getSelectorString();
+String[] selectors = selectorString == null ?
+
getRawSelectors(request.getResource().getResourceMetadata().getResolutionPathInfo())
+: request.getRequestPathInfo().getSelectors();
+
+if (!isValidRequest(request.getRequestPathInfo().getResourcePath(),
selectors)) {
Review Comment:
This looks good. How about moving this code into isValidRequest and only
pass the RequestPathInfo into that method (I think you can replace
request.getResource()...getResolutionPathInfo() with
RequestPathInfo.getResourcePath()) ?
--
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-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] SLING-12124 - Inconsistent handling of empty selectors [sling-org-apache-sling-engine]
sagarmiglani commented on PR #40: URL: https://github.com/apache/sling-org-apache-sling-engine/pull/40#issuecomment-1801517879 @cziegeler Updated the code, can you please have a look. Now it does not change the computation of selectors and selector string. -- 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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12124 - Inconsistent handling of empty selectors [sling-org-apache-sling-engine]
sonarcloud[bot] commented on PR #40: URL: https://github.com/apache/sling-org-apache-sling-engine/pull/40#issuecomment-1798053942 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-engine=40) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-engine=40=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-engine=40=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-engine=40=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_coverage=list) [88.5% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_duplicated_lines_density=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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12124 - Inconsistent handling of empty selectors [sling-org-apache-sling-engine]
sonarcloud[bot] commented on PR #40: URL: https://github.com/apache/sling-org-apache-sling-engine/pull/40#issuecomment-1798008733 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-engine=40) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-engine=40=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-engine=40=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-engine=40=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_coverage=list) [88.5% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_duplicated_lines_density=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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12124 - Inconsistent handling of empty selectors [sling-org-apache-sling-engine]
cziegeler commented on PR #40: URL: https://github.com/apache/sling-org-apache-sling-engine/pull/40#issuecomment-1790153094 I think when the check method is called you have access to the request object which should give you the full original path. You also have the resource path. So maybe something like if the request path contains more than one dot directly after the resource path. (only in the case of null selector string) -- 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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12124 - Inconsistent handling of empty selectors [sling-org-apache-sling-engine]
sagarmiglani commented on PR #40: URL: https://github.com/apache/sling-org-apache-sling-engine/pull/40#issuecomment-1790104527 > Maybe it is safer to do the change where we check for invalid requests (if selector array is null but there are pairs of dots) @cziegeler For empty selectors (e.g: "html"), the resulting selector string is null and selector array is empty. How would you recommend verifying this condition (only pair of dots?) while assessing the validity of a request? (at that point, we will have null selector string and empty selector array) -- 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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12124 - Inconsistent handling of empty selectors [sling-org-apache-sling-engine]
sagarmiglani commented on PR #40: URL: https://github.com/apache/sling-org-apache-sling-engine/pull/40#issuecomment-1788573092 @cziegeler Thank you for having a look at this. This is what i thought at first, but there were some edge cases which were still making the flow inconsistent. e.g: In case `..html` generated selector string is an empty string which will again pass the validation. Allow me some more time, i will try to come up with a different and safer solution. -- 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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12124 - Inconsistent handling of empty selectors [sling-org-apache-sling-engine]
cziegeler commented on PR #40: URL: https://github.com/apache/sling-org-apache-sling-engine/pull/40#issuecomment-1788475384 Thanks @sagarmiglani for the patch. I think you change the behaviour of SlingRequestPathInfo for this case which might lead to problems (previously null was returned for the selectors and now an array with an empty string). Maybe it is safer to do the change where we check for invalid requests (if selector array is null but there are pairs of dots). -- 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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12124 - Inconsistent handling of empty selectors [sling-org-apache-sling-engine]
sonarcloud[bot] commented on PR #40: URL: https://github.com/apache/sling-org-apache-sling-engine/pull/40#issuecomment-1788397096 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-engine=40) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-engine=40=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-engine=40=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-engine=40=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-engine=40=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_coverage=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-engine=40=new_duplicated_lines_density=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-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
