You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "reschke (via GitHub)" <gi...@apache.org> on 2024/08/08 14:35:35 UTC
[PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
reschke opened a new pull request, #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348
(work in progress)
This appears to work, except in Jackrabbit, which is unhappy about the sequence "setProperty( ... uid ...)" followed by "addMixing(mix:ref)".
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1709824889
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -1047,8 +1056,38 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
}
// add remaining mixins (for all import modes)
for (String mixin : newMixins) {
+ Property uuidProp = null;
vs.ensureCheckedOut();
- node.addMixin(mixin);
+
+ if (mixin.equals("mix:referenceable") && identifier.isPresent()) {
+ // if this is mix:ref and ni specifies an identifier,
+ // try to set it
+ try {
+ uuidProp = node.setProperty(Property.JCR_UUID, identifier.get());
Review Comment:
I thought we get here in case we skipped that...?
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1709767267
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -1047,8 +1056,38 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
}
// add remaining mixins (for all import modes)
for (String mixin : newMixins) {
+ Property uuidProp = null;
vs.ensureCheckedOut();
- node.addMixin(mixin);
+
+ if (mixin.equals("mix:referenceable") && identifier.isPresent()) {
+ // if this is mix:ref and ni specifies an identifier,
+ // try to set it
+ try {
+ uuidProp = node.setProperty(Property.JCR_UUID, identifier.get());
Review Comment:
isn't the uuid already set through the sysview import?
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,17 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ String nodeUUID = null;
+ try {
+ nodeUUID = node.getUUID();
Review Comment:
> Deprecated. As of JCR 2.0, [getIdentifier()](https://developer.adobe.com/experience-manager/reference-materials/spec/javax.jcr/javadocs/jcr-2.0/javax/jcr/Node.html#getIdentifier()) should be used instead.
(https://developer.adobe.com/experience-manager/reference-materials/spec/javax.jcr/javadocs/jcr-2.0/javax/jcr/Node.html#getUUID())
I am not understanding how that makes a diference because this is all only relevant if the node supposed to be update/replaced has a uuid and IMHO the return value should be the same for getUUID and getIdentifier in case the node is referenceable right?
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1709823732
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,17 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ String nodeUUID = null;
+ try {
+ nodeUUID = node.getUUID();
Review Comment:
Yes, but this is the simplest way to find out whether the node is referenceable. The other things I tried all require a lot of work with checking node types.
FWIW, the deprecation of `getUUID()` IMHO was incorrect, because these two have different semantics for non-referenceable nodes.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1709761323
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,17 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ String nodeUUID = null;
+ try {
+ nodeUUID = node.getUUID();
Review Comment:
> Deprecated. As of JCR 2.0, [getIdentifier()](https://developer.adobe.com/experience-manager/reference-materials/spec/javax.jcr/javadocs/jcr-2.0/javax/jcr/Node.html#getIdentifier()) should be used instead.
(https://developer.adobe.com/experience-manager/reference-materials/spec/javax.jcr/javadocs/jcr-2.0/javax/jcr/Node.html#getUUID())
I am not understanding how that makes a diference because this is all only relevant if the node supposed to be update/replaced has a uuid and IMHO the return value should be the same for `getUUID()` and `getIdentifier()` in case the node is referenceable right?
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1711246254
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,19 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ boolean isUuidProtected = false;
+ if (node.hasProperty(Property.JCR_UUID)) {
+ isUuidProtected = node.getProperty(Property.JCR_UUID).getDefinition().isProtected();
+ } else if (! session.getRepository().getDescriptor(Repository.REP_NAME_DESC).contains("Oak")) {
Review Comment:
might have to do with https://jackrabbit.apache.org/oak/docs/differences.html#Identifiers - but code is unclear and needs fixing
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1711233713
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,19 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ boolean isUuidProtected = false;
+ if (node.hasProperty(Property.JCR_UUID)) {
+ isUuidProtected = node.getProperty(Property.JCR_UUID).getDefinition().isProtected();
+ } else if (! session.getRepository().getDescriptor(Repository.REP_NAME_DESC).contains("Oak")) {
Review Comment:
good question; tests passed with Jackrabbit; will have another look
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1711274675
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,19 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ boolean isUuidProtected = false;
+ if (node.hasProperty(Property.JCR_UUID)) {
+ isUuidProtected = node.getProperty(Property.JCR_UUID).getDefinition().isProtected();
+ } else if (! session.getRepository().getDescriptor(Repository.REP_NAME_DESC).contains("Oak")) {
Review Comment:
Should better wirh https://github.com/apache/jackrabbit-filevault/pull/348/commits/223bb00051fb67b41e2a01dd2d5f70a7f5ca7a2b
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1711246254
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,19 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ boolean isUuidProtected = false;
+ if (node.hasProperty(Property.JCR_UUID)) {
+ isUuidProtected = node.getProperty(Property.JCR_UUID).getDefinition().isProtected();
+ } else if (! session.getRepository().getDescriptor(Repository.REP_NAME_DESC).contains("Oak")) {
Review Comment:
mifgt have to do with https://jackrabbit.apache.org/oak/docs/differences.html#Identifiers - but code is unclear and needs fixing
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1709945702
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,17 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ String nodeUUID = null;
+ try {
+ nodeUUID = node.getUUID();
Review Comment:
so
hasProperty -> getProperty -> getDefinition -> isProtected
?
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1711338418
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,19 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ boolean isUuidProtected = false;
+ if (! session.getRepository().getDescriptor(Repository.REP_NAME_DESC).contains("Oak")) {
+ // workaround for non-Oak repos that may not be able to add
+ // mixin:referencable *after* setting jcr:uuid
+ isUuidProtected = true;
Review Comment:
The original idea was to encapsulate impl specific differences in https://github.com/apache/jackrabbit-filevault/blob/master/vault-core/src/main/java/org/apache/jackrabbit/vault/fs/spi/ServiceProvider.java.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1711153105
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,19 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ boolean isUuidProtected = false;
+ if (node.hasProperty(Property.JCR_UUID)) {
+ isUuidProtected = node.getProperty(Property.JCR_UUID).getDefinition().isProtected();
+ } else if (! session.getRepository().getDescriptor(Repository.REP_NAME_DESC).contains("Oak")) {
Review Comment:
why else if? Shouldn't this also overwrite isUuidProtected from line 995 potentially?
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1711341846
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,19 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ boolean isUuidProtected = false;
+ if (! session.getRepository().getDescriptor(Repository.REP_NAME_DESC).contains("Oak")) {
+ // workaround for non-Oak repos that may not be able to add
+ // mixin:referencable *after* setting jcr:uuid
+ isUuidProtected = true;
Review Comment:
Thanks for the pointer; let's do more research on the "setting of jcr:uuid" first, and the outcome of that would inform what information we would want to add there.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1709885484
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,17 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ String nodeUUID = null;
+ try {
+ nodeUUID = node.getUUID();
Review Comment:
checking if the property jcr:uuid is protected should be pretty straightforward
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #348:
URL: https://github.com/apache/jackrabbit-filevault/pull/348#discussion_r1709887725
##########
vault-core/src/main/java/org/apache/jackrabbit/vault/fs/impl/io/DocViewImporter.java:
##########
@@ -989,8 +989,17 @@ private void removeReferences(@NotNull Node node) throws ReferentialIntegrityExc
VersioningState vs = new VersioningState(stack, node);
Node updatedNode = null;
Optional<String> identifier = ni.getIdentifier();
+
+ String nodeUUID = null;
+ try {
+ nodeUUID = node.getUUID();
Review Comment:
That sounds good, I'll give that a try.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org