You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "mbaedke (via GitHub)" <gi...@apache.org> on 2023/08/07 14:20:19 UTC
[GitHub] [jackrabbit-oak] mbaedke opened a new pull request, #1051: OAK-10374: Checked-out state of non-versionable nodes is not handled …
mbaedke opened a new pull request, #1051:
URL: https://github.com/apache/jackrabbit-oak/pull/1051
…correctly
Added versionability check.
--
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
[GitHub] [jackrabbit-oak] mbaedke closed pull request #1051: OAK-10377: Checked-out state of non-versionable nodes is not handled …
Posted by "mbaedke (via GitHub)" <gi...@apache.org>.
mbaedke closed pull request #1051: OAK-10377: Checked-out state of non-versionable nodes is not handled …
URL: https://github.com/apache/jackrabbit-oak/pull/1051
--
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
[GitHub] [jackrabbit-oak] mreutegg commented on a diff in pull request #1051: OAK-10377: Checked-out state of non-versionable nodes is not handled …
Posted by "mreutegg (via GitHub)" <gi...@apache.org>.
mreutegg commented on code in PR #1051:
URL: https://github.com/apache/jackrabbit-oak/pull/1051#discussion_r1328398490
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/VersionEditor.java:
##########
@@ -212,7 +212,7 @@ private boolean isLockProperty(PropertyState state) {
private boolean wasCheckedIn() {
PropertyState prop = before.getProperty(JCR_ISCHECKEDOUT);
if (prop != null) {
- return !prop.getValue(Type.BOOLEAN);
+ return !prop.getValue(Type.BOOLEAN) && isVersionable;
Review Comment:
Why not use `isVersionable()` instead? `isVersionable` can be null and needs to be computed first. I see calls to wasCheckedIn() are currently always preceded with isVersionable(), but I'd rather not introduce this implicit dependency.
--
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
[GitHub] [jackrabbit-oak] sonarcloud[bot] commented on pull request #1051: OAK-10377: Checked-out state of non-versionable nodes is not handled …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1051:
URL: https://github.com/apache/jackrabbit-oak/pull/1051#issuecomment-1668092150
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&metric=new_coverage&view=list) [80.0% Coverage](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&metric=new_duplicated_lines_density&view=list)
 The version of Java (11.0.19) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more [here](https://docs.sonarcloud.io/appendices/scanner-environment/)
--
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
[GitHub] [jackrabbit-oak] mreutegg commented on a diff in pull request #1051: OAK-10377: Checked-out state of non-versionable nodes is not handled …
Posted by "mreutegg (via GitHub)" <gi...@apache.org>.
mreutegg commented on code in PR #1051:
URL: https://github.com/apache/jackrabbit-oak/pull/1051#discussion_r1328414603
##########
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/version/VersionableTest.java:
##########
@@ -362,6 +363,68 @@ public void testIsCheckedOutOnNonAccessibleParent() throws Exception {
assertFalse(guest.getNode(nodePath).isCheckedOut());
}
+ public void testNonVersionableCheckedOut() throws Exception {
+ Node node = testRootNode.addNode(nodeName1, "nt:unstructured");
+ superuser.save();
+
+ assertTrue(node.isCheckedOut());
+
+ node.setProperty("jcr:isCheckedOut", false);
+ superuser.save();
+
+ assertTrue(node.isCheckedOut());
+ }
+
+ public void testModifyNonVersionableNodeWithCheckedOutProperty() throws Exception {
+ Node node = testRootNode.addNode(nodeName1, "nt:unstructured");
+ superuser.save();
+
+ assertTrue(node.isCheckedOut());
+
+ node.setProperty("jcr:isCheckedOut", false);
+ superuser.save();
+
+ node.setProperty("test", true);
+ superuser.save();
+
+ assertTrue(node.getProperty("test").getBoolean());
+
+ node.setProperty("test", false);
+ superuser.save();
+
+ assertFalse(node.getProperty("test").getBoolean());
+
+ node.getProperty("test").remove();
+ superuser.save();
+ assertFalse(node.hasProperty("test"));
+
+ node.addNode(nodeName2, "nt:unstructured");
+ superuser.save();
+
+ assertTrue(node.hasNode(nodeName2));
+
+ node.getNode(nodeName2).remove();
+ superuser.save();
+
+ assertFalse(node.hasNode(nodeName2));
+ }
+
+ public void testAddRemoveMixinVersionable() throws Exception {
+ Node node = testRootNode.addNode(nodeName1, "nt:unstructured");
+ node.addMixin(mixVersionable);
+ superuser.save();
+ node.checkin();
+ superuser.save();
+ assertFalse(node.isCheckedOut());
+ node.checkout();
+ superuser.save();
+ assertTrue(node.isCheckedOut());
+ node.removeMixin(mixVersionable);
+ superuser.save();
+ assertTrue(node.isCheckedOut());
+ assertFalse(node.hasProperty(jcrIsCheckedOut));
+ }
Review Comment:
I think it would be good to extend this test a bit and add mix:versionable again at the end, then verify version semantics are still correct. WDYT?
--
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
[GitHub] [jackrabbit-oak] mbaedke commented on a diff in pull request #1051: OAK-10377: Checked-out state of non-versionable nodes is not handled …
Posted by "mbaedke (via GitHub)" <gi...@apache.org>.
mbaedke commented on code in PR #1051:
URL: https://github.com/apache/jackrabbit-oak/pull/1051#discussion_r1328738094
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/VersionEditor.java:
##########
@@ -212,7 +212,7 @@ private boolean isLockProperty(PropertyState state) {
private boolean wasCheckedIn() {
PropertyState prop = before.getProperty(JCR_ISCHECKEDOUT);
if (prop != null) {
- return !prop.getValue(Type.BOOLEAN);
+ return !prop.getValue(Type.BOOLEAN) && isVersionable;
Review Comment:
Makes sense.
--
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
[GitHub] [jackrabbit-oak] sonarcloud[bot] commented on pull request #1051: OAK-10377: Checked-out state of non-versionable nodes is not handled …
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1051:
URL: https://github.com/apache/jackrabbit-oak/pull/1051#issuecomment-1725272897
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&metric=new_coverage&view=list) [80.0% Coverage](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=1051&metric=new_duplicated_lines_density&view=list)
 The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more [here](https://docs.sonarcloud.io/appendices/scanner-environment/)
--
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
[GitHub] [jackrabbit-oak] mbaedke commented on a diff in pull request #1051: OAK-10377: Checked-out state of non-versionable nodes is not handled …
Posted by "mbaedke (via GitHub)" <gi...@apache.org>.
mbaedke commented on code in PR #1051:
URL: https://github.com/apache/jackrabbit-oak/pull/1051#discussion_r1328738960
##########
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/version/VersionableTest.java:
##########
@@ -362,6 +363,68 @@ public void testIsCheckedOutOnNonAccessibleParent() throws Exception {
assertFalse(guest.getNode(nodePath).isCheckedOut());
}
+ public void testNonVersionableCheckedOut() throws Exception {
+ Node node = testRootNode.addNode(nodeName1, "nt:unstructured");
+ superuser.save();
+
+ assertTrue(node.isCheckedOut());
+
+ node.setProperty("jcr:isCheckedOut", false);
+ superuser.save();
+
+ assertTrue(node.isCheckedOut());
+ }
+
+ public void testModifyNonVersionableNodeWithCheckedOutProperty() throws Exception {
+ Node node = testRootNode.addNode(nodeName1, "nt:unstructured");
+ superuser.save();
+
+ assertTrue(node.isCheckedOut());
+
+ node.setProperty("jcr:isCheckedOut", false);
+ superuser.save();
+
+ node.setProperty("test", true);
+ superuser.save();
+
+ assertTrue(node.getProperty("test").getBoolean());
+
+ node.setProperty("test", false);
+ superuser.save();
+
+ assertFalse(node.getProperty("test").getBoolean());
+
+ node.getProperty("test").remove();
+ superuser.save();
+ assertFalse(node.hasProperty("test"));
+
+ node.addNode(nodeName2, "nt:unstructured");
+ superuser.save();
+
+ assertTrue(node.hasNode(nodeName2));
+
+ node.getNode(nodeName2).remove();
+ superuser.save();
+
+ assertFalse(node.hasNode(nodeName2));
+ }
+
+ public void testAddRemoveMixinVersionable() throws Exception {
+ Node node = testRootNode.addNode(nodeName1, "nt:unstructured");
+ node.addMixin(mixVersionable);
+ superuser.save();
+ node.checkin();
+ superuser.save();
+ assertFalse(node.isCheckedOut());
+ node.checkout();
+ superuser.save();
+ assertTrue(node.isCheckedOut());
+ node.removeMixin(mixVersionable);
+ superuser.save();
+ assertTrue(node.isCheckedOut());
+ assertFalse(node.hasProperty(jcrIsCheckedOut));
+ }
Review Comment:
Also makes sense.
--
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