reschke commented on code in PR #1993:
URL: https://github.com/apache/jackrabbit-oak/pull/1993#discussion_r1938313155
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/namepath/NameMapper.java:
##########
@@ -63,16 +63,35 @@ public interface NameMapper {
Map<String, String> getSessionLocalMappings();
/**
- * Returns the JCR name for the given Oak name. The given name is
+ * Returns the JCR name in qualified form for the given Oak name. The
given name is
* expected to have come from a valid Oak repository that contains
* only valid names with proper namespace mappings. If that's not
* the case, either a programming error or a repository corruption
* has occurred and an appropriate unchecked exception gets thrown.
*
* @param oakName Oak name
- * @return JCR name
+ * @return JCR name in qualified form
+ *
+ * @see <a
href="https://s.apache.org/jcr-2.0-spec/3_Repository_Model.html#3.2.5.2%20Qualified%20Form";>JCR
2.0, 3.2.5.2 Qualifed Form</a>
*/
@NotNull
String getJcrName(@NotNull String oakName);
+ /**
+ * Returns the JCR name in expanded form for the given Oak name. The given
name is
+ * expected to have come from a valid Oak repository that contains
+ * only valid names with proper namespace mappings. If that's not
+ * the case, either a programming error or a repository corruption
+ * has occurred and an appropriate unchecked exception gets thrown.
+ *
+ * @param oakName Oak name
+ * @return JCR name in expanded form
+ * @since Oak 1.76.0
+ * @throws IllegalStateException in case the namespace URI for the given
Oak name cannot be resolved
Review Comment:
"resolved" in the context fof URIs can be confusing. Maybe "determined"?
##########
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/namepath/NameMapper.java:
##########
@@ -63,16 +63,35 @@ public interface NameMapper {
Map<String, String> getSessionLocalMappings();
/**
- * Returns the JCR name for the given Oak name. The given name is
+ * Returns the JCR name in qualified form for the given Oak name. The
given name is
* expected to have come from a valid Oak repository that contains
* only valid names with proper namespace mappings. If that's not
* the case, either a programming error or a repository corruption
* has occurred and an appropriate unchecked exception gets thrown.
*
* @param oakName Oak name
- * @return JCR name
+ * @return JCR name in qualified form
+ *
+ * @see <a
href="https://s.apache.org/jcr-2.0-spec/3_Repository_Model.html#3.2.5.2%20Qualified%20Form";>JCR
2.0, 3.2.5.2 Qualifed Form</a>
*/
@NotNull
String getJcrName(@NotNull String oakName);
+ /**
+ * Returns the JCR name in expanded form for the given Oak name. The given
name is
Review Comment:
There's the unfortunate exception of the "internal" namespace. That's an
invalid namespace URI, but there we are.
What is the current behavior for that namespace when we try to map it to an
expanded name? AFAIR, we special-case that, but I would need to check.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/namepath/impl/GlobalNameMapper.java:
##########
@@ -67,10 +67,15 @@ protected static boolean isHiddenName(String name) {
protected static boolean isExpandedName(String name) {
if (name.startsWith("{")) {
int brace = name.indexOf('}', 1);
- return brace != -1 && name.substring(1, brace).indexOf(':') != -1;
- } else {
- return false;
+ if (brace != -1) {
+ String namespace = name.substring(1, brace);
+ // the empty namespace is valid as well, otherwise it always
contains a colon (as it is a URI)
Review Comment:
If there's a bug here, we should fix it independently; we may also want to
backport that fix to 1.22.
Could you please open a Jira issue? Do we lack test coverage?
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/namepath/impl/NamePathMapperImplTest.java:
##########
@@ -135,6 +135,26 @@ public void testOakToJcr() {
}
}
+ @Test
+ public void testOakToExpandedJcr() {
+ assertEquals("/{http://www.example.com/foo}bar";,
npMapper.getExpandedJcrPath("/oak-foo:bar"));
+
assertEquals("/{http://www.example.com/foo}bar/{http://www.example.com/quu}qux";,
npMapper.getExpandedJcrPath("/oak-foo:bar/oak-quu:qux"));
+ assertEquals("{http://www.example.com/foo}bar";,
npMapper.getExpandedJcrPath("oak-foo:bar"));
+ assertEquals(".", npMapper.getExpandedJcrPath(""));
+
+ try {
+
npMapper.getExpandedJcrPath("{http://www.jcp.org/jcr/nt/1.0}unstructured";);
Review Comment:
Not sure whether this is good. As a user of this API, I would expect that I
can apply it to *any* valid JCR name (in both forms). Otherwise I may have to
check before.
Proposal: allow both forms (with one being a NO-OP) and include that case in
the Javadoc.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/namepath/impl/GlobalNameMapper.java:
##########
@@ -67,10 +67,15 @@ protected static boolean isHiddenName(String name) {
protected static boolean isExpandedName(String name) {
if (name.startsWith("{")) {
int brace = name.indexOf('}', 1);
- return brace != -1 && name.substring(1, brace).indexOf(':') != -1;
- } else {
- return false;
+ if (brace != -1) {
+ String namespace = name.substring(1, brace);
+ // the empty namespace is valid as well, otherwise it always
contains a colon (as it is a URI)
Review Comment:
Also, we need to check that the "internal" namespace is handled consistently
throughout and has sufficient test coverage.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
