[jira] [Commented] (JCRVLT-767) vlt: potential incorrect identifier comparison
[
https://issues.apache.org/jira/browse/JCRVLT-767?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17872243#comment-17872243
]
Julian Reschke commented on JCRVLT-767:
---
For the case of Jackrabbit classic, I've now added a somewhat clumsy workaround
(checking the repository's name descriptor first)
> vlt: potential incorrect identifier comparison
> --
>
> Key: JCRVLT-767
> URL: https://issues.apache.org/jira/browse/JCRVLT-767
> Project: Jackrabbit FileVault
> Issue Type: Bug
> Components: vlt
>Reporter: Julian Reschke
>Priority: Minor
>
> In
> https://github.com/apache/jackrabbit-filevault/blob/931ceef98513167af3218b773d9213e123a2f52d/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L993-L1007
> {noformat}
> if (identifier.isPresent() &&
> !node.getIdentifier().equals(identifier.get()) &&
> !"rep:root".equals(ni.getPrimaryType().orElse(""))) {
> long startTime = System.currentTimeMillis();
> String previousIdentifier = node.getIdentifier();
> log.debug("Node stashing for {} starting, existing identifier:
> {}, new identifier: {}, import mode: {}",
> node.getPath(), previousIdentifier, identifier.get(),
> importMode);
> {noformat}
> However, Node.getIdentifer() will always be non-null - even for nodes without
> mix:referenceable.
> This means that we go into stashing although we don't have to.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (JCRVLT-767) vlt: potential incorrect identifier comparison
[
https://issues.apache.org/jira/browse/JCRVLT-767?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17872063#comment-17872063
]
Julian Reschke commented on JCRVLT-767:
---
Yes, stashing indeed happens too often.
In the else-case, we actually use {{setPrimaryNodeType}} and then add mixins
one-by-one. We can set the {{jcr:uuid}} before adding
{{{}mixin:referenceable{}}}.
What's bad is that Jackrabbit Classic doesn't allow us to do so, and I haven't
figured out yet how to deal with the exception properly.
> vlt: potential incorrect identifier comparison
> --
>
> Key: JCRVLT-767
> URL: https://issues.apache.org/jira/browse/JCRVLT-767
> Project: Jackrabbit FileVault
> Issue Type: Bug
> Components: vlt
>Reporter: Julian Reschke
>Priority: Minor
>
> In
> https://github.com/apache/jackrabbit-filevault/blob/931ceef98513167af3218b773d9213e123a2f52d/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L993-L1007
> {noformat}
> if (identifier.isPresent() &&
> !node.getIdentifier().equals(identifier.get()) &&
> !"rep:root".equals(ni.getPrimaryType().orElse(""))) {
> long startTime = System.currentTimeMillis();
> String previousIdentifier = node.getIdentifier();
> log.debug("Node stashing for {} starting, existing identifier:
> {}, new identifier: {}, import mode: {}",
> node.getPath(), previousIdentifier, identifier.get(),
> importMode);
> {noformat}
> However, Node.getIdentifer() will always be non-null - even for nodes without
> mix:referenceable.
> This means that we go into stashing although we don't have to.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (JCRVLT-767) vlt: potential incorrect identifier comparison
[
https://issues.apache.org/jira/browse/JCRVLT-767?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17872062#comment-17872062
]
Konrad Windszus commented on JCRVLT-767:
Even if {{Node.getIdentifer()}} is always non-null (and returns their path for
non-referencable nodes) I don't see how the overall condition on the if would
be wrong? Are you suggesting that stashing happens too often? I was assuming
that stashing is necessary whenever we set the UUID (as we don't do tricks yet
like removing the mixin to make the property unprotected).
> vlt: potential incorrect identifier comparison
> --
>
> Key: JCRVLT-767
> URL: https://issues.apache.org/jira/browse/JCRVLT-767
> Project: Jackrabbit FileVault
> Issue Type: Bug
> Components: vlt
>Reporter: Julian Reschke
>Priority: Minor
>
> In
> https://github.com/apache/jackrabbit-filevault/blob/931ceef98513167af3218b773d9213e123a2f52d/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L993-L1007
> {noformat}
> if (identifier.isPresent() &&
> !node.getIdentifier().equals(identifier.get()) &&
> !"rep:root".equals(ni.getPrimaryType().orElse(""))) {
> long startTime = System.currentTimeMillis();
> String previousIdentifier = node.getIdentifier();
> log.debug("Node stashing for {} starting, existing identifier:
> {}, new identifier: {}, import mode: {}",
> node.getPath(), previousIdentifier, identifier.get(),
> importMode);
> {noformat}
> However, Node.getIdentifer() will always be non-null - even for nodes without
> mix:referenceable.
> This means that we go into stashing although we don't have to.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (JCRVLT-767) vlt: potential incorrect identifier comparison
[
https://issues.apache.org/jira/browse/JCRVLT-767?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17872019#comment-17872019
]
Julian Reschke commented on JCRVLT-767:
---
This is a bit tricky to change: in the "other" case ("else"), we set the new
primary type and then re-add mixins. I tried setting jcr:uuid (when present)
before adding mix:referenceable. That works for Oak, but fails for Jackrabbit.
> vlt: potential incorrect identifier comparison
> --
>
> Key: JCRVLT-767
> URL: https://issues.apache.org/jira/browse/JCRVLT-767
> Project: Jackrabbit FileVault
> Issue Type: Bug
> Components: vlt
>Reporter: Julian Reschke
>Priority: Minor
>
> In
> https://github.com/apache/jackrabbit-filevault/blob/931ceef98513167af3218b773d9213e123a2f52d/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L993-L1007
> {noformat}
> if (identifier.isPresent() &&
> !node.getIdentifier().equals(identifier.get()) &&
> !"rep:root".equals(ni.getPrimaryType().orElse(""))) {
> long startTime = System.currentTimeMillis();
> String previousIdentifier = node.getIdentifier();
> log.debug("Node stashing for {} starting, existing identifier:
> {}, new identifier: {}, import mode: {}",
> node.getPath(), previousIdentifier, identifier.get(),
> importMode);
> {noformat}
> However, Node.getIdentifer() will always be non-null - even for nodes without
> mix:referenceable.
> This means that we go into stashing although we don't have to.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (JCRVLT-767) vlt: potential incorrect identifier comparison
[
https://issues.apache.org/jira/browse/JCRVLT-767?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17871981#comment-17871981
]
Julian Reschke commented on JCRVLT-767:
---
FWIW, I just confirmed that on a server with actual customer data (node to be
overwritten not refenceable, not any ancestor, Oak returns on "getIdentifier()"
the node's path, which of course is different from the identifier to be
imported).
> vlt: potential incorrect identifier comparison
> --
>
> Key: JCRVLT-767
> URL: https://issues.apache.org/jira/browse/JCRVLT-767
> Project: Jackrabbit FileVault
> Issue Type: Bug
> Components: vlt
>Reporter: Julian Reschke
>Priority: Minor
>
> In
> https://github.com/apache/jackrabbit-filevault/blob/931ceef98513167af3218b773d9213e123a2f52d/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L993-L1007
> {noformat}
> if (identifier.isPresent() &&
> !node.getIdentifier().equals(identifier.get()) &&
> !"rep:root".equals(ni.getPrimaryType().orElse(""))) {
> long startTime = System.currentTimeMillis();
> String previousIdentifier = node.getIdentifier();
> log.debug("Node stashing for {} starting, existing identifier:
> {}, new identifier: {}, import mode: {}",
> node.getPath(), previousIdentifier, identifier.get(),
> importMode);
> {noformat}
> However, Node.getIdentifer() will always be non-null - even for nodes without
> mix:referenceable.
> This means that we go into stashing although we don't have to.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (JCRVLT-767) vlt: potential incorrect identifier comparison
[
https://issues.apache.org/jira/browse/JCRVLT-767?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17871883#comment-17871883
]
Julian Reschke commented on JCRVLT-767:
---
This
{noformat}
diff --git
a/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java
b/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java
index 3572276b..a6e27742 100644
---
a/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java
+++
b/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java
@@ -989,8 +989,16 @@ public class DocViewImporter implements
DocViewParserHandler {
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional identifier = ni.getIdentifier();
+
+String nodeUUID = null;
+try {
+nodeUUID = node.getUUID();
+} catch (RepositoryException ex) {
+// node not referenceable
+}
+
// try to set uuid via sysview import if it differs from existing one
-if (identifier.isPresent() &&
!node.getIdentifier().equals(identifier.get()) &&
!"rep:root".equals(ni.getPrimaryType().orElse(""))) {
+if (identifier.isPresent() && !identifier.get().equals(nodeUUID) &&
!"rep:root".equals(ni.getPrimaryType().orElse(""))) {
long startTime = System.currentTimeMillis();
String previousIdentifier = node.getIdentifier();
log.debug("Node stashing for {} starting, existing identifier: {},
new identifier: {}, import mode: {}",
{noformat}
would fix is and still passes tests.
> vlt: potential incorrect identifier comparison
> --
>
> Key: JCRVLT-767
> URL: https://issues.apache.org/jira/browse/JCRVLT-767
> Project: Jackrabbit FileVault
> Issue Type: Bug
> Components: vlt
>Reporter: Julian Reschke
>Priority: Minor
>
> In
> https://github.com/apache/jackrabbit-filevault/blob/931ceef98513167af3218b773d9213e123a2f52d/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java#L993-L1007
> {noformat}
> if (identifier.isPresent() &&
> !node.getIdentifier().equals(identifier.get()) &&
> !"rep:root".equals(ni.getPrimaryType().orElse(""))) {
> long startTime = System.currentTimeMillis();
> String previousIdentifier = node.getIdentifier();
> log.debug("Node stashing for {} starting, existing identifier:
> {}, new identifier: {}, import mode: {}",
> node.getPath(), previousIdentifier, identifier.get(),
> importMode);
> {noformat}
> However, Node.getIdentifer() will always be non-null - even for nodes without
> mix:referenceable.
> This means that we go into stashing although we don't have to.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
