stefan-egli commented on code in PR #560:
URL: https://github.com/apache/jackrabbit-oak/pull/560#discussion_r873773082
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -114,10 +114,10 @@
import static
org.apache.jackrabbit.oak.plugins.document.NodeDocument.SD_MAX_REV_TIME_IN_SECS;
import static org.apache.jackrabbit.oak.plugins.document.NodeDocument.SD_TYPE;
import static
org.apache.jackrabbit.oak.plugins.document.UpdateOp.Condition.newEqualsCondition;
-import static
org.apache.jackrabbit.oak.plugins.document.mongo.MongoUtils.createIndex;
import static
org.apache.jackrabbit.oak.plugins.document.mongo.MongoUtils.createPartialIndex;
import static
org.apache.jackrabbit.oak.plugins.document.mongo.MongoUtils.getDocumentStoreExceptionTypeFor;
import static
org.apache.jackrabbit.oak.plugins.document.mongo.MongoUtils.hasIndex;
+import static
org.apache.jackrabbit.oak.plugins.document.mongo.MongoUtils.createIndex;
Review Comment:
This seems to have been at the right place where it was before, I'd move it
back to reduce number of changes
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/util/UtilsTest.java:
##########
@@ -75,6 +76,7 @@
public class UtilsTest {
private static final long TIME_DIFF_WARN_THRESHOLD_MILLIS = 2000L;
+ private static final String LONG_PATH =
"/foo/barbar/qwerty/asdfgh/zxcvbnm/bu~pcsdb_2_c_cm~2020_idea_pad_idea_centre_holiday_campaign_cv~all_the_right_answers_idea_centre_aio_amd_carousel_ct~fb_cs~1_x_1_lg~en_pr~idea_centre_aio_5_fs~amd_ot~_cr~the_woo_5_jpg/bu~pcsdb_2_c_cm~2020_idea_pad_idea_centre_holiday_campaign_cv~all_the_right_answers_idea_centre_aio_amd_carousel_ct~fb_cs~1_x_1_lg~en_pr~idea_centre_aio_5_fs~amd_ot~_cr~the_woo_5_jpg_1/bu~pcsdb_2_c_cm~2020_idea_pad_idea_centre_holiday_campaign_cv~all_the_right_answers_idea_centre_aio_amd_carousel_ct~fb_cs~1_x_1_lg~en_pr~idea_centre_aio_5_fs~amd_ot~_cr~the_woo_5_jpg_2";
Review Comment:
I would further obfuscate the path name. It still looks too similar to an
actual name used out in the field. What about making up the whole deep path
structure from foo bar bazes..?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoUtils.java:
##########
@@ -190,4 +195,15 @@ static Type getDocumentStoreExceptionTypeFor(Throwable t) {
}
return type;
}
+
+ /**
+ * Util method to get node size limit for current mongo version
+ *
+ * @param version version of current mongo db
+ * @return size limit based on mongo db version
+ */
+ static int getNodeNameLimit(final String version) {
+ final ServerVersion sv = new
ServerVersion(Arrays.stream(version.split("\\.")).map(Integer::new).collect(Collectors.toList()));
+ return sv.compareTo(new ServerVersion(Lists.newArrayList(4,0,0))) > 0
? Integer.MAX_VALUE : DocumentStore.NODE_NAME_LIMIT;
Review Comment:
I guess you could also do
```suggestion
return sv.compareTo(new ServerVersion(4, 0) > 0 ? Integer.MAX_VALUE
: DocumentStore.NODE_NAME_LIMIT;
```
but that's a detail
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java:
##########
@@ -92,11 +92,6 @@ public class Utils {
*/
public static final int PATH_LONG = Integer.getInteger("oak.pathLong",
350);
- /**
- * The maximum size a node name, in bytes. This is only a problem for long
path.
- */
- public static final int NODE_NAME_LIMIT =
Integer.getInteger("oak.nodeNameLimit", 150);
-
Review Comment:
Was thinking about this a bit. Currently you're moving it to `DocumentStore`
as that's where there's now a new `getNodeNameLimit`. But actually there's also
(test) code that still refers to the static field. So in light of reducing the
number of changes of this PR even further, but also not change if it's really
necessary, what do you think about leaving `NODE_NAME_LIMIT` in here in Utils -
and have `DocumentStore.getNodeNameLimit` refer to this one? It would reduce
the diff and separate one concern: keep the static field here as the 'hardcoded
default' vs allow subclasses of `DocumentStore` overwriting it (without
necessarily also keeping this field there)
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentSplitTest.java:
##########
@@ -732,21 +732,21 @@ public <T extends Document> T find(Collection<T>
collection,
ns.dispose();
}
-
+
Review Comment:
+1, this comment still applied, I'd too suggest to revert
##########
oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositoryUpgrade.java:
##########
@@ -1022,10 +1023,10 @@ void assertNoLongNames() throws RepositoryException {
}
private boolean nameMayBeTooLong(String name) {
- if (name.length() <= Utils.NODE_NAME_LIMIT / 3) {
+ if (name.length() <= DocumentStore.NODE_NAME_LIMIT / 3) {
return false;
}
- if (name.getBytes(Charsets.UTF_8).length <= Utils.NODE_NAME_LIMIT) {
+ if (name.getBytes(Charsets.UTF_8).length <=
DocumentStore.NODE_NAME_LIMIT) {
Review Comment:
(this class could remain unchanged if the static is kept in `Utils` - see
earlier comment )
##########
oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/nodestate/NameFilteringNodeState.java:
##########
@@ -77,10 +78,10 @@ protected PropertyState decorateProperty(@NotNull final
PropertyState delegatePr
private boolean isNameTooLong(@NotNull String name) {
// OAK-1589: maximum supported length of name for DocumentNodeStore
// is 150 bytes. Skip the sub tree if the the name is too long
- if (name.length() <= Utils.NODE_NAME_LIMIT / 3) {
+ if (name.length() <= DocumentStore.NODE_NAME_LIMIT / 3) {
return false;
}
- if (name.getBytes(Charsets.UTF_8).length <= Utils.NODE_NAME_LIMIT) {
+ if (name.getBytes(Charsets.UTF_8).length <=
DocumentStore.NODE_NAME_LIMIT) {
Review Comment:
(this class could remain unchanged if the static is kept in `Utils` - see
earlier comment )
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentMK.java:
##########
@@ -52,6 +52,9 @@ public class DocumentMK {
* The threshold where special handling for many child node starts.
*/
static final int MANY_CHILDREN_THRESHOLD =
DocumentNodeStoreBuilder.MANY_CHILDREN_THRESHOLD;
+
+ static final String LONG_PATH =
"/foo/barbar/qwerty/asdfgh/zxcvbnm/bu~pcsdb_2_c_cm~2020_idea_pad_idea_centre_holiday_campaign_cv~all_the_right_answers_idea_centre_aio_amd_carousel_ct~fb_cs~1_x_1_lg~en_pr~idea_centre_aio_5_fs~amd_ot~_cr~the_woo_5_jpg/bu~pcsdb_2_c_cm~2020_idea_pad_idea_centre_holiday_campaign_cv~all_the_right_answers_idea_centre_aio_amd_carousel_ct~fb_cs~1_x_1_lg~en_pr~idea_centre_aio_5_fs~amd_ot~_cr~the_woo_5_jpg_1/bu~pcsdb_2_c_cm~2020_idea_pad_idea_centre_holiday_campaign_cv~all_the_right_answers_idea_centre_aio_amd_carousel_ct~fb_cs~1_x_1_lg~en_pr~idea_centre_aio_5_fs~amd_ot~_cr~the_woo_5_jpg_2";
Review Comment:
I would further obfuscate the path name. It still looks too similar to an
actual name used out in the field. What about making up the whole deep path
structure from foo bar bazes..?
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/Sweep2TestHelper.java:
##########
@@ -85,7 +85,7 @@ static MemoryDocumentStore
getMemoryDocumentStore(DocumentNodeStore ns) {
* <li>if simulatePreFixUpgrade then simulate an upgrade to a version post
1.8 but without a fix for the branch commit problem</li>
* <li>upgrade to a fixed version</li>
* </ol>
- * @param ns
+ * @param memStore
Review Comment:
seems unrelated from this PR and might complicate backporting work
unnecessarily, even if just very slightly
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java:
##########
@@ -365,14 +361,17 @@ private static boolean isLongPath(String path) {
}
// check if the parent path is long
byte[] parent = PathUtils.getParentPath(path).getBytes(UTF_8);
- if (parent.length < PATH_LONG) {
- return false;
- }
- String name = PathUtils.getName(path);
- if (name.getBytes(UTF_8).length > NODE_NAME_LIMIT) {
- throw new IllegalArgumentException("Node name is too long: " +
path);
- }
- return true;
+ return parent.length >= PATH_LONG;
+ }
Review Comment:
can you explain why the node name check is obsolete here? I understand
`DocumentNodeState.asOperation` is now the place where the node name length
check is done. But is that (enough) justification to remove this name check
here?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java:
##########
@@ -338,6 +333,7 @@ public static StringBuilder encodeHexString(byte[] data,
StringBuilder sb) {
* <li>If id is for root path</li>
* <li>If id is for an invalid path</li>
* </ul>
+ *
Review Comment:
I'd remove this unrelated one line 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
