OAK-11000: Test coverage for common JCR operations related to importing content by reschke · Pull Request #1622 · apache/jackrabbit-oak

reschke · 2024-08-02T10:23:57Z

No description provided.

…ng content (work-in-progress)

kwin · 2024-08-02T15:14:10Z

oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/ImportOperationsTest.java

+ * specification, but to observe the actual behavior of the implementation
+ * (which may be hard to change).
+ */
+public class ImportOperationsTest extends AbstractRepositoryTest {


The class name is confusing to me. What about ProtectedPropertyTest because IIUC this is mainly about jcr:uuid and jcr:created for cases when both become protected.

Maybe even split up general protected properties from jcr:uuid into dedicated test classes make sense because the latter imposes additional restrictions.

Done the remaining; let's discuss splitting later on.

oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/ImportOperationsTest.java

kwin · 2024-08-02T15:24:16Z

oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/ImportOperationsTest.java

+            session.save();
+            fail("Setting jcr:uuid after adding mixin:referenceable should fail");
+        } catch (ConstraintViolationException ex) {
+            // expected


This is IMHO mandated by the JCR spec as mix:referenceable defines jcr:uuid as protected (so this test rather belongs to the JCR TCK)

If it's there we could drop it.

kwin · 2024-08-02T15:24:42Z

oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/ImportOperationsTest.java

+    }
+
+    @Test
+    public void jcrMixinReferenceableOnNtUnstructuredAfterRemovingMixinButDanglingReference() throws RepositoryException {


Maybe; if you can find it. For now I'll leave it here, because having related tests in different places doesn't seem to be helpful.

…erited (such as from nt:resource)

… when another referenceable node already has that UUID

stefan-egli · 2024-08-05T14:00:13Z

Just to mention one further, somewhat brutal, option : Equivalent to temporarily removing the mix:referenceable (with the goal to allow changing the jcr:uuid) would be to temporarily change the primaryType to an unprotected one that would allow changing the jcr:uuid. That could be achieved either by using a hard-coded raw type (eg nt:unstructured, oak:Unstructured depending on orderable or not) - or by temporarily adding an node type that extends the existing one, and in the temporary one make jcr:uuid not protected, eg:

            NodeTypeTemplate unprotectedNTT = ntMgr.createNodeTypeTemplate();
            unprotectedNTT.setName(tmpNodeTypeName);
            unprotectedNTT.setDeclaredSuperTypeNames(new String[] {primaryType.getName()});
            PropertyDefinitionTemplate pdt = ntMgr.createPropertyDefinitionTemplate();
            pdt.setName("jcr:uuid");
            pdt.setProtected(false);
            unprotectedNTT.getPropertyDefinitionTemplates().add(pdt);
            session.getWorkspace().getNodeTypeManager().registerNodeType(unprotectedNTT, true);
            target.setPrimaryType(unprotectedNTT.getName());
            target.setProperty("jcr:uuid", newUUID);
            target.setPrimaryType(primaryType.getName());
            session.getWorkspace().getNodeTypeManager().unregisterNodeType(unprotectedNTT.getName());
            session.save();

reschke · 2024-08-05T14:02:25Z

The first two options would be problematic if the parent node requires a certain type (ack @kwin :-). The latter would work if extending a type would indeed allow "unprotecting" a property...

stefan-egli · 2024-08-05T14:53:21Z

unprotecting does work - I tested with the snipped added previously - it's just a bit ... brutal

…tured (ack mbaedke)

…ed jcr:uuid (ack stefan-egli), also minor refactoring

mbaedke · 2024-08-08T13:05:54Z

oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/ProtectedPropertyTest.java

+            // JCR spec
+            // (https://developer.adobe.com/experience-manager/reference-materials/spec/jcr/2.0/3_Repository_Model.html#3.8%20Referenceable%20Nodes)
+            // requests an "auto-created" property, so it might be a surprise
+            // that Oak actually keeps the application-assigned previous value.


The algorithm used to create the property value is free to use the existing one and I think that is also the most sane approach, so is it really that surprising?

the spec says "autocreated" :-)

mbaedke · 2024-08-08T13:08:16Z

oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/ProtectedPropertyTest.java

+        Node test = testNode.addNode(TEST_NODE_NAME, NodeType.NT_UNSTRUCTURED);
+        test.addMixin(NodeType.MIX_REFERENCEABLE);
+        session.save();
+        Node ref = testNode.addNode(TEST_NODE_NAME_REF, NodeType.NT_UNSTRUCTURED);


Nitpick: Earlier the name ref was used for the referenced node, not the referencing one. So we might want to change that to avoid confusion.

…without mixin:ref

OAK-11000: Test coverage for common JCR operations related to importi…

50c1fd1

…ng content (work-in-progress)

reschke marked this pull request as draft August 2, 2024 10:24

reschke requested review from kwin, mbaedke and stefan-egli August 2, 2024 10:24

OAK-11000: Test coverage for common JCR operations related to importi…

4f57156

…ng content (work-in-progress)

kwin reviewed Aug 2, 2024

View reviewed changes

reschke added 4 commits August 5, 2024 10:16

OAK-11000:rename to ProtectedPropertyTest

737408f

OAK-11000: fix comment about adding mixin:created behavior (ack kwin)

0025cf4

OAK-11000: confirm that removeMixin(mix:referenceable) fails when inh…

0517fd4

…erited (such as from nt:resource)

OAK-11000: confirm that setting jcr:uuid as custom property will fail…

9b7814a

… when another referenceable node already has that UUID

reschke and others added 3 commits August 6, 2024 14:27

OAK-11000: check that jcr:uuid is gone after removing from nt:unstruc…

3173b5c

…tured (ack mbaedke)

OAK-11000: add variant that uses a temporary node type with unprotect…

9401366

…ed jcr:uuid (ack stefan-egli), also minor refactoring

OAK-11000: use constants from java.jcr.Property

a4cf43e

reschke marked this pull request as ready for review August 7, 2024 14:00

reschke requested a review from kwin August 7, 2024 14:00

mbaedke reviewed Aug 8, 2024

View reviewed changes

mbaedke approved these changes Aug 8, 2024

View reviewed changes

reschke added 4 commits August 12, 2024 11:17

OAK-11000: test overlapping updates with same UUID using two sessions

1399ce1

OAK-11000: add tests for getNodeByIdentifier

c802642

OAK-11000: adjust expection for exceptions for DOCUMENT_NS

f4fc2a2

OAK-11000: add tests for just setting conflicting jcr:uuids on nodes …

fbc3bf1

…without mixin:ref

Conversation

reschke commented Aug 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefan-egli commented Aug 5, 2024

Uh oh!

reschke commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefan-egli commented Aug 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

reschke commented Aug 5, 2024 •

edited

Loading