rishabhdaim commented on code in PR #880:
URL: https://github.com/apache/jackrabbit-oak/pull/880#discussion_r1153464833
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/migration/NodeStateCopier.java:
##########
@@ -145,29 +150,73 @@ public static boolean copyNodeStore(@NotNull final
NodeStore source, @NotNull fi
*
* @param source The NodeState to copy from.
* @param target The NodeBuilder to copy to.
+ * @param preserveOnTarget boolean to indicate no changes on target except
additions
+ * @param path current path
* @return Whether changes were made or not.
*/
- public static boolean copyProperties(NodeState source, NodeBuilder target)
{
+ public static boolean copyProperties(NodeState source, NodeBuilder target,
boolean preserveOnTarget, String path) {
boolean hasChanges = false;
// remove removed properties
- for (final PropertyState property : target.getProperties()) {
- final String name = property.getName();
- if (!source.hasProperty(name)) {
- target.removeProperty(name);
- hasChanges = true;
+ if (!preserveOnTarget) {
+ for (final PropertyState property : target.getProperties()) {
+ final String name = property.getName();
+ if (!source.hasProperty(name)) {
+ target.removeProperty(name);
+ hasChanges = true;
+ }
}
}
// add new properties and change changed properties
for (PropertyState property : source.getProperties()) {
if (!property.equals(target.getProperty(property.getName()))) {
- target.setProperty(property);
- hasChanges = true;
+ if (!isVersionPropertyEmpty(source, property,
preserveOnTarget, path)) {
+ target.setProperty(property);
+ hasChanges = true;
+ }
}
}
return hasChanges;
}
+
+ private static Set<String> getValues(NodeState nodeState, String prop) {
+ if (nodeState.hasProperty(prop)) {
+ Iterable<String> values =
nodeState.getProperty(prop).getValue(Type.STRINGS);
+ return StreamSupport.stream(values.spliterator(), false).filter(s
-> !s.isEmpty()).collect(Collectors.toSet());
+ }
+ return new HashSet<>();
Review Comment:
Implementation of this method only uses the size of `set` returned by this
method. So instead of creating an `set` of non empty properties, we should
simply return a boolean indicating whether it is empty or not.
```suggestion
private static boolean isVersionEmpty(NodeState nodeState, String prop) {
if (nodeState.hasProperty(prop)) {
Iterable<String> values =
nodeState.getProperty(prop).getValue(Type.STRINGS);
return StreamSupport.stream(values.spliterator(),
false).allMatch(String::isEmpty);
}
return true;
```
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/migration/NodeStateCopier.java:
##########
@@ -145,29 +150,73 @@ public static boolean copyNodeStore(@NotNull final
NodeStore source, @NotNull fi
*
* @param source The NodeState to copy from.
* @param target The NodeBuilder to copy to.
+ * @param preserveOnTarget boolean to indicate no changes on target except
additions
+ * @param path current path
* @return Whether changes were made or not.
*/
- public static boolean copyProperties(NodeState source, NodeBuilder target)
{
+ public static boolean copyProperties(NodeState source, NodeBuilder target,
boolean preserveOnTarget, String path) {
boolean hasChanges = false;
// remove removed properties
- for (final PropertyState property : target.getProperties()) {
- final String name = property.getName();
- if (!source.hasProperty(name)) {
- target.removeProperty(name);
- hasChanges = true;
+ if (!preserveOnTarget) {
+ for (final PropertyState property : target.getProperties()) {
+ final String name = property.getName();
+ if (!source.hasProperty(name)) {
+ target.removeProperty(name);
+ hasChanges = true;
+ }
}
}
// add new properties and change changed properties
for (PropertyState property : source.getProperties()) {
if (!property.equals(target.getProperty(property.getName()))) {
- target.setProperty(property);
- hasChanges = true;
+ if (!isVersionPropertyEmpty(source, property,
preserveOnTarget, path)) {
+ target.setProperty(property);
+ hasChanges = true;
+ }
}
}
return hasChanges;
}
+
+ private static Set<String> getValues(NodeState nodeState, String prop) {
+ if (nodeState.hasProperty(prop)) {
+ Iterable<String> values =
nodeState.getProperty(prop).getValue(Type.STRINGS);
+ return StreamSupport.stream(values.spliterator(), false).filter(s
-> !s.isEmpty()).collect(Collectors.toSet());
+ }
+ return new HashSet<>();
+ }
+
+ private static boolean isVersionPropertyEmpty(NodeState source,
PropertyState property, boolean preserveOnTarget,
+ String path) {
+ if (preserveOnTarget) {
+ if (property.getName().equals(JcrConstants.JCR_UUID) ||
+ property.getName().equals(JcrConstants.JCR_SUCCESSORS) ||
+ property.getName().equals(JcrConstants.JCR_PREDECESSORS)) {
+ boolean versionPropertyEmpty = getValues(source,
property.getName()).isEmpty();
Review Comment:
```suggestion
boolean versionPropertyEmpty = isVersionEmpty(source,
property.getName());
```
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/migration/version/VersionCopier.java:
##########
@@ -111,6 +125,72 @@ public boolean copyVersionHistory(String versionableUuid,
Calendar minDate, bool
return false;
}
+ private boolean hasNoConflicts(String versionHistoryPath, String
versionableUuid, boolean preserveOnTarget, NodeState sourceVersionHistory) {
+ // if preserveOnTarget is true then check no conflicts which means
version history has moved forward only
+ if (preserveOnTarget) {
Review Comment:
same as above, return if `preserveOnTarget` is false.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/migration/NodeStateCopier.java:
##########
@@ -145,29 +150,73 @@ public static boolean copyNodeStore(@NotNull final
NodeStore source, @NotNull fi
*
* @param source The NodeState to copy from.
* @param target The NodeBuilder to copy to.
+ * @param preserveOnTarget boolean to indicate no changes on target except
additions
+ * @param path current path
* @return Whether changes were made or not.
*/
- public static boolean copyProperties(NodeState source, NodeBuilder target)
{
+ public static boolean copyProperties(NodeState source, NodeBuilder target,
boolean preserveOnTarget, String path) {
boolean hasChanges = false;
// remove removed properties
- for (final PropertyState property : target.getProperties()) {
- final String name = property.getName();
- if (!source.hasProperty(name)) {
- target.removeProperty(name);
- hasChanges = true;
+ if (!preserveOnTarget) {
+ for (final PropertyState property : target.getProperties()) {
+ final String name = property.getName();
+ if (!source.hasProperty(name)) {
+ target.removeProperty(name);
+ hasChanges = true;
+ }
}
}
// add new properties and change changed properties
for (PropertyState property : source.getProperties()) {
if (!property.equals(target.getProperty(property.getName()))) {
- target.setProperty(property);
- hasChanges = true;
+ if (!isVersionPropertyEmpty(source, property,
preserveOnTarget, path)) {
+ target.setProperty(property);
+ hasChanges = true;
+ }
}
}
return hasChanges;
}
+
+ private static Set<String> getValues(NodeState nodeState, String prop) {
+ if (nodeState.hasProperty(prop)) {
+ Iterable<String> values =
nodeState.getProperty(prop).getValue(Type.STRINGS);
+ return StreamSupport.stream(values.spliterator(), false).filter(s
-> !s.isEmpty()).collect(Collectors.toSet());
+ }
+ return new HashSet<>();
+ }
+
+ private static boolean isVersionPropertyEmpty(NodeState source,
PropertyState property, boolean preserveOnTarget,
+ String path) {
+ if (preserveOnTarget) {
Review Comment:
I would simply return if `preserveOnTarget` is false.
```suggestion
if (!preserveOnTarget) {
return false;
```
then to go inside if condition.
As a general coding practice, I found:
`if not condition:
do little stuff
return
// now...
do lots of stuff.`
increases the code readability and reduce cyclomatic complexity.
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/migration/version/VersionCopierTest.java:
##########
@@ -44,18 +50,15 @@
import org.junit.Test;
import static java.util.Collections.singletonList;
-import static org.apache.jackrabbit.JcrConstants.JCR_FROZENNODE;
-import static org.apache.jackrabbit.JcrConstants.JCR_MIXINTYPES;
-import static org.apache.jackrabbit.JcrConstants.JCR_PRIMARYTYPE;
-import static org.apache.jackrabbit.JcrConstants.JCR_UUID;
-import static org.apache.jackrabbit.JcrConstants.MIX_VERSIONABLE;
-import static org.apache.jackrabbit.JcrConstants.NT_UNSTRUCTURED;
+import static org.apache.jackrabbit.JcrConstants.*;
Review Comment:
Please import them individually.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/migration/version/VersionCopier.java:
##########
@@ -111,6 +125,72 @@ public boolean copyVersionHistory(String versionableUuid,
Calendar minDate, bool
return false;
}
+ private boolean hasNoConflicts(String versionHistoryPath, String
versionableUuid, boolean preserveOnTarget, NodeState sourceVersionHistory) {
+ // if preserveOnTarget is true then check no conflicts which means
version history has moved forward only
+ if (preserveOnTarget) {
+ NodeBuilder targetVersionHistory =
getVersionHistoryBuilder(targetVersionStorage, versionableUuid);
+ if (targetVersionHistory.exists()) {
+ VersionComparator versionComparator = new VersionComparator();
+
+ // version history id not equal
+ boolean conflictingVersionHistory =
!targetVersionHistory.getString(JCR_UUID).equals(sourceVersionHistory.getString(JCR_UUID));
+ if (conflictingVersionHistory) {
+ logger.info("Skipping version history for {}: Conflicting
version history found",
+ versionHistoryPath);
+ return false;
+ }
+
+ // Get the version names except jcr:rootVersion
+ List<String> targetVersions =
+
StreamSupport.stream(targetVersionHistory.getChildNodeNames().spliterator(),
false).filter(s -> !s.equals(JCR_ROOTVERSION) && !s.equals(JCR_VERSIONLABELS))
+
.sorted(versionComparator).collect(Collectors.toList());
+ List<String> sourceVersions =
+
StreamSupport.stream(sourceVersionHistory.getChildNodeNames().spliterator(),
false).filter(s -> !s.equals(JCR_ROOTVERSION) && !s.equals(JCR_VERSIONLABELS))
+
.sorted(versionComparator).collect(Collectors.toList());
+ // source version only has a rootVersion which means nothing
to update
+ boolean noUpdate = sourceVersions.isEmpty() ||
targetVersions.containsAll(sourceVersions);
+ if (noUpdate) {
+ logger.info("Skipping version history for {}: No update
required", versionHistoryPath);
+ return false;
+ }
+
+ // highest source version does not exist on target or
+ // all source versions already exist on target (diverged or no
diff)
+ boolean diverged =
!targetVersions.contains(sourceVersions.get(0));
+ if (diverged) {
+ logger.info("Skipping version history for {}: Versions
diverged", versionHistoryPath);
+ return false;
+ }
+
+ // highest source version UUID does not match the
corresponding version on target (diverged)
+ boolean conflictingHighestVersion =
+
!sourceVersionHistory.getChildNode(sourceVersions.get(0)).getString(JCR_UUID).equals(targetVersionHistory.getChildNode(sourceVersions.get(0)).getString(JCR_UUID));
Review Comment:
Please use `Objects.equal`
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/migration/version/VersionCopier.java:
##########
@@ -111,6 +125,72 @@ public boolean copyVersionHistory(String versionableUuid,
Calendar minDate, bool
return false;
}
+ private boolean hasNoConflicts(String versionHistoryPath, String
versionableUuid, boolean preserveOnTarget, NodeState sourceVersionHistory) {
+ // if preserveOnTarget is true then check no conflicts which means
version history has moved forward only
+ if (preserveOnTarget) {
+ NodeBuilder targetVersionHistory =
getVersionHistoryBuilder(targetVersionStorage, versionableUuid);
+ if (targetVersionHistory.exists()) {
+ VersionComparator versionComparator = new VersionComparator();
+
+ // version history id not equal
+ boolean conflictingVersionHistory =
!targetVersionHistory.getString(JCR_UUID).equals(sourceVersionHistory.getString(JCR_UUID));
+ if (conflictingVersionHistory) {
+ logger.info("Skipping version history for {}: Conflicting
version history found",
+ versionHistoryPath);
+ return false;
+ }
+
+ // Get the version names except jcr:rootVersion
+ List<String> targetVersions =
+
StreamSupport.stream(targetVersionHistory.getChildNodeNames().spliterator(),
false).filter(s -> !s.equals(JCR_ROOTVERSION) && !s.equals(JCR_VERSIONLABELS))
+
.sorted(versionComparator).collect(Collectors.toList());
+ List<String> sourceVersions =
+
StreamSupport.stream(sourceVersionHistory.getChildNodeNames().spliterator(),
false).filter(s -> !s.equals(JCR_ROOTVERSION) && !s.equals(JCR_VERSIONLABELS))
+
.sorted(versionComparator).collect(Collectors.toList());
+ // source version only has a rootVersion which means nothing
to update
+ boolean noUpdate = sourceVersions.isEmpty() ||
targetVersions.containsAll(sourceVersions);
+ if (noUpdate) {
+ logger.info("Skipping version history for {}: No update
required", versionHistoryPath);
+ return false;
+ }
+
+ // highest source version does not exist on target or
+ // all source versions already exist on target (diverged or no
diff)
+ boolean diverged =
!targetVersions.contains(sourceVersions.get(0));
+ if (diverged) {
+ logger.info("Skipping version history for {}: Versions
diverged", versionHistoryPath);
+ return false;
+ }
+
+ // highest source version UUID does not match the
corresponding version on target (diverged)
+ boolean conflictingHighestVersion =
+
!sourceVersionHistory.getChildNode(sourceVersions.get(0)).getString(JCR_UUID).equals(targetVersionHistory.getChildNode(sourceVersions.get(0)).getString(JCR_UUID));
+ if (conflictingHighestVersion) {
+ logger.info("Skipping version history for {}: Old base
version id changed", versionHistoryPath);
+ return false;
+ }
+ }
+ }
+ return true;
+ }
+
+ static class VersionComparator implements Comparator<String> {
Review Comment:
We are making a reverse comparison here.
Shouldn't we make a simple comparison and then where we are using we should
explicitly call `comparator.reversed()` to highlight that we are using a
reverse comparison?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/migration/version/VersionCopier.java:
##########
@@ -111,6 +125,72 @@ public boolean copyVersionHistory(String versionableUuid,
Calendar minDate, bool
return false;
}
+ private boolean hasNoConflicts(String versionHistoryPath, String
versionableUuid, boolean preserveOnTarget, NodeState sourceVersionHistory) {
+ // if preserveOnTarget is true then check no conflicts which means
version history has moved forward only
+ if (preserveOnTarget) {
+ NodeBuilder targetVersionHistory =
getVersionHistoryBuilder(targetVersionStorage, versionableUuid);
+ if (targetVersionHistory.exists()) {
+ VersionComparator versionComparator = new VersionComparator();
+
+ // version history id not equal
+ boolean conflictingVersionHistory =
!targetVersionHistory.getString(JCR_UUID).equals(sourceVersionHistory.getString(JCR_UUID));
Review Comment:
Since `NodeBuilder.getString()` can return null, I would compare them using
`Objects.equal`
--
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]
