...while JIRA is down...
We currently loose index information when converting from JCR to OAK
paths (signaling an error on index != 1, and dropping the others).
This isn't sufficient; there are API signatures where even a "[1]" on
the final path segment is forbidden, such as when creating new nodes.
We can either hack these restrictions into NodeImpl, SessionImpl and
friends, or extend the path mapper to optionally check. To do the latter
properly, I'd like to change the NamePathMapperImpl so that it collects
PathSegments instead of Strings (see attached w-i-p patch).
I realize that this deviates from the use-Strings-when-possible
approach; but I don't see a clean way to get things working properly
without otherwise.
So feedback appreciated; I plan to finish this tomorrow.
Best regards, Julian
Index:
oak-core/src/main/java/org/apache/jackrabbit/oak/namepath/NamePathMapperImpl.java
===================================================================
---
oak-core/src/main/java/org/apache/jackrabbit/oak/namepath/NamePathMapperImpl.java
(revision 1338797)
+++
oak-core/src/main/java/org/apache/jackrabbit/oak/namepath/NamePathMapperImpl.java
(working copy)
@@ -21,6 +21,7 @@
import java.util.ArrayList;
import java.util.List;
+import java.util.Stack;
/**
* NamePathMapperImpl...
@@ -52,7 +53,7 @@
//---------------------------------------------------------< PathMapper
>---
@Override
public String getOakPath(String jcrPath) {
- final List<String> elements = new ArrayList<String>();
+ final Stack<PathSegment> segments = new Stack<PathSegment>();
final StringBuilder parseErrors = new StringBuilder();
if ("/".equals(jcrPath)) {
@@ -64,21 +65,19 @@
@Override
public boolean root() {
- if (!elements.isEmpty()) {
- parseErrors.append("/ on non-empty path");
- return false;
+ if (!segments.isEmpty()) {
+ throw new IllegalArgumentException("/ on non-empty path");
}
- elements.add("");
+ segments.add(new PathSegment(""));
return true;
}
@Override
public boolean identifier(String identifier) {
- if (!elements.isEmpty()) {
- parseErrors.append("[identifier] on non-empty path");
- return false;
+ if (!segments.isEmpty()) {
+ throw new IllegalArgumentException("[identifier] on
non-empty path");
}
- elements.add(identifier); // todo resolve identifier
+ segments.add(new PathSegment(identifier)); // todo resolve
identifier
// todo seal elements
return true;
}
@@ -91,21 +90,23 @@
@Override
public boolean parent() {
- if (elements.isEmpty() ||
"..".equals(elements.get(elements.size() - 1))) {
- elements.add("..");
- return true;
+ if (segments.isEmpty() ||
"..".equals(segments.peek().getName())) {
+ segments.add(new PathSegment(".."));
+ } else {
+ segments.pop();
}
- elements.remove(elements.size() - 1);
return true;
}
@Override
public boolean index(int index) {
if (index > 1) {
- parseErrors.append("index > 1");
+ throw new IllegalArgumentException("index > 1");
+ } else if (segments.isEmpty()) {
return false;
+ } else {
+ return segments.peek().addIndex(index);
}
- return true;
}
@Override
@@ -116,11 +117,7 @@
@Override
public boolean name(String name) {
String p = nameMapper.getOakName(name);
- if (p == null) {
- parseErrors.append("Invalid name: ").append(name);
- return false;
- }
- elements.add(p);
+ segments.add(new PathSegment(p));
return true;
}
};
@@ -132,22 +129,29 @@
}
// Empty path maps to ""
- if (elements.isEmpty()) {
+ if (segments.isEmpty()) {
return "";
}
StringBuilder oakPath = new StringBuilder();
- for (String element : elements) {
- if (element.isEmpty()) {
+ PathSegment lastSegment = null;
+ for (PathSegment segment: segments) {
+ if (segment.getName().isEmpty()) {
// root
oakPath.append('/');
}
else {
- oakPath.append(element);
+ oakPath.append(segment.getName());
oakPath.append('/');
}
+ lastSegment = segment;
}
+ if (lastSegment != null && lastSegment.hasIndex()) {
+ // it's not allowed to specify an index on the last path segments
for paths to be created
+ return null;
+ }
+
// root path is special-cased early on so it does not need to
// be considered here
oakPath.deleteCharAt(oakPath.length() - 1);
@@ -156,7 +160,7 @@
@Override
public String getJcrPath(String oakPath) {
- final List<String> elements = new ArrayList<String>();
+ final Stack<PathSegment> segments = new Stack<PathSegment>();
if ("/".equals(oakPath)) {
// avoid the need to special case the root path later on
@@ -166,19 +170,19 @@
JcrPathParser.Listener listener = new JcrPathParser.Listener() {
@Override
public boolean root() {
- if (!elements.isEmpty()) {
+ if (!segments.isEmpty()) {
throw new IllegalArgumentException("/ on non-empty path");
}
- elements.add("");
+ segments.add(new PathSegment(""));
return true;
}
@Override
public boolean identifier(String identifier) {
- if (!elements.isEmpty()) {
+ if (!segments.isEmpty()) {
throw new IllegalArgumentException("[identifier] on
non-empty path");
}
- elements.add(identifier); // todo resolve identifier
+ segments.add(new PathSegment(identifier)); // todo resolve
identifier
// todo seal elements
return true;
}
@@ -191,11 +195,11 @@
@Override
public boolean parent() {
- if (elements.isEmpty() ||
"..".equals(elements.get(elements.size() - 1))) {
- elements.add("..");
- return true;
+ if (segments.isEmpty() ||
"..".equals(segments.peek().getName())) {
+ segments.add(new PathSegment(".."));
+ } else {
+ segments.pop();
}
- elements.remove(elements.size() - 1);
return true;
}
@@ -203,8 +207,11 @@
public boolean index(int index) {
if (index > 1) {
throw new IllegalArgumentException("index > 1");
+ } else if (segments.isEmpty()) {
+ return false;
+ } else {
+ return segments.peek().addIndex(index);
}
- return true;
}
@Override
@@ -215,7 +222,7 @@
@Override
public boolean name(String name) {
String p = nameMapper.getJcrName(name);
- elements.add(p);
+ segments.add(new PathSegment(p));
return true;
}
};
@@ -223,23 +230,65 @@
JcrPathParser.parse(oakPath, listener);
// empty path: map to "."
- if (elements.isEmpty()) {
+ if (segments.isEmpty()) {
return ".";
}
StringBuilder jcrPath = new StringBuilder();
- for (String element : elements) {
- if (element.isEmpty()) {
+ PathSegment lastSegment = null;
+ for (PathSegment segment: segments) {
+ if (segment.getName().isEmpty()) {
// root
jcrPath.append('/');
}
else {
- jcrPath.append(element);
+ jcrPath.append(segment.getName());
jcrPath.append('/');
}
+ lastSegment = segment;
}
+ if (lastSegment != null && lastSegment.hasIndex()) {
+ // it's not allowed to specify an index on the last path segments
for paths to be created
+ return null;
+ }
+
jcrPath.deleteCharAt(jcrPath.length() - 1);
return jcrPath.toString();
}
+
+
+ /**
+ * Minimal information needed to keep track of JCR path segments
+ */
+ private class PathSegment {
+
+ private final String name;
+ private int index = -1; // not set
+
+ public PathSegment(String name) {
+ this.name = name;
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public boolean hasIndex() {
+ return index >= 0;
+ }
+
+ public boolean addIndex(int index) {
+ if (this.index != -1) {
+ // already set
+ return false;
+ } else if ("".equals(name) || "..".equals(name)) {
+ // not allowed on this segment
+ return false;
+ } else {
+ this.index = index;
+ return true;
+ }
+ }
+ }
}
Index:
oak-core/src/main/java/org/apache/jackrabbit/oak/namepath/JcrPathParser.java
===================================================================
---
oak-core/src/main/java/org/apache/jackrabbit/oak/namepath/JcrPathParser.java
(revision 1338797)
+++
oak-core/src/main/java/org/apache/jackrabbit/oak/namepath/JcrPathParser.java
(working copy)
@@ -115,7 +115,7 @@
}
JcrNameParser.parse(name, listener);
- if (!listener.index(index)) {
+ if (index != 0 && !listener.index(index)) {
return;
}
state = STATE_PREFIX_START;
@@ -218,7 +218,7 @@
jcrPath.substring(lastPos, pos - 1));
return;
}
- if (index < 0) {
+ if (index <= 0) {
listener.error('\'' + jcrPath + "' is not a valid
path. " +
"Index number invalid: " + index);
return;
