jackrabbit-oak-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Mueller (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (OAK-5260) Incorrect handling of subpaths with leading left curly bracket
Date Fri, 16 Dec 2016 07:36:58 GMT

    [ https://issues.apache.org/jira/browse/OAK-5260?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15753717#comment-15753717
] 

Thomas Mueller edited comment on OAK-5260 at 12/16/16 7:36 AM:
---------------------------------------------------------------

Thinking about this some more, I think there is a risk of applying the patch. The patch makes
the following test pass:

{noformat}
+        String[] paths = {
+                "/parent/sub/{childB7",
+                "/parent/{childA1",
+                "/parent/{{childA2"
+        };
+
+        for (String path : paths) {
+            assertEquals(path, npMapper.getOakPath(path));
+        }
{noformat}

(The new test contains more cases, but those were working before as well.)

Assuming it is possible that such a nodes can exists, then the following paths become ambiguous:

{noformat}
                "/parent/{childA1/childChildB1}",
                "/parent/{{childA2/}childChildB2}"
{noformat}

Is the slash now the delimiter, or is the "\{..\}" pair the delimiter?

The existing JcrPathParser code searches for a "\}" after finding a "\{", then can't find
one, and so truncates from "/parent/{childA1" to "/parent/", which is then further truncated
to "/parent".

Maybe it would be better to return null, meaning "invalid". The Javadoc of that method says:

{quote}
Returns the Oak path for the given JCR path, or _null_ if no
such mapping exists because the given JCR path contains a name element
with an unknown namespace URI or prefix, or is otherwise invalid.
{quote}

There is also a problem with getOakName: the javadoc for _that_ method says:

{quote}
Returns the Oak name for the specified JCR name. In contrast to
getOakNameOrNull(String) this method will throw a RepositoryException
if the JCR name is invalid and cannot be resolved.
{quote}

This is not in consistent with getOakPath, which never throws, but may return null. That's
inconsistent. Then, the name "{test" is not considered invalid. So maybe you can create such
nodes, which would be a problem, because if you create a child node "\}test2", then the full
path of that node would be "/\{test/\}test2", which is ambiguous.

I think it would be much easier if names such as "\{test" are invalid, because the "\}" is
missing. Instead, they need to be escaped using "\{\}": "\{\}\{test".


was (Author: tmueller):
Thinking about this some more, I think there is a risk of applying the patch. The patch makes
the following test pass:

{noformat}
+        String[] paths = {
+                "/parent/sub/{childB7",
+                "/parent/{childA1",
+                "/parent/{{childA2"
+        };
+
+        for (String path : paths) {
+            assertEquals(path, npMapper.getOakPath(path));
+        }
{noformat}

(The new test contains more cases, but those were working before as well.)

Assuming it is possible that such a nodes can exists, then the following paths become ambiguous:

{noformat}
                "/parent/{childA1/childChildB1}",
                "/parent/{{childA2/}childChildB2}"
{noformat}

Is the slash now the delimiter, or is the "\{..\}" pair the delimiter?

The existing JcrPathParser code searches for a "\}" after finding a "\{", then can't find
one, and so truncates from "/parent/{childA1" to "/parent/", which is then further truncated
to "/parent".

Maybe it would be better to return null, meaning "invalid". The Javadoc of that method says:

{quote}
Returns the Oak path for the given JCR path, or _null_ if no
such mapping exists because the given JCR path contains a name element
with an unknown namespace URI or prefix, or is otherwise invalid.
{quote}

There is also a problem with getOakName: the javadoc for _that_ method says:

{quote}
Returns the Oak name for the specified JCR name. In contrast to
getOakNameOrNull(String) this method will throw a RepositoryException
if the JCR name is invalid and cannot be resolved.
{quote}

This is not in consistent with getOakPath, which never throws, but may return null. That's
inconsistent. Then, the name "{test" is not considered invalid. So maybe you can create such
nodes, which would be a problem, because if you create a child node "}test2", then the full
path of that node would be "/{test/}test2", which is ambiguous.

I think it would be much easier if names such as "\{test" are invalid, because the "\}" is
missing. Instead, they need to be escaped using "\{\}": "\{\}\{test".

> Incorrect handling of subpaths with leading left curly bracket
> --------------------------------------------------------------
>
>                 Key: OAK-5260
>                 URL: https://issues.apache.org/jira/browse/OAK-5260
>             Project: Jackrabbit Oak
>          Issue Type: Bug
>          Components: jcr
>            Reporter: Bertrand Delacretaz
>            Assignee: Julian Sedding
>             Fix For: 1.6
>
>         Attachments: OAK-5260-jsedding.patch, OAK-5260.patch
>
>
> As per SLING-6383 it looks like the Oak name mapping causes for example getItem("/libs/{sub")
(with a left curly bracket in the path) to return the /libs node if that exists but the supplied
path does not.
> I'll attach a patch with a test that demonstrates this. 
> [~fmeschbe] mentions in that Sling issue that the parsing of the CR 2 section 3.2.5.1
Expanded Form could be involved, treating the left curly bracket in a special way that's not
appropriate here.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message