Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] JCRVLT-767: vlt: potential incorrect identifier comparison [jackrabbit-filevault]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
