[MAGNOLIA-3288] Retrieving a binary from versioned content fails Created: 02/Sep/10  Updated: 13/Dec/11  Resolved: 13/Sep/10

Status: Closed
Project: Magnolia
Component/s: core
Affects Version/s: 4.3.1, 4.3.6
Fix Version/s: 4.3.7, 4.5

Type: Bug Priority: Critical
Reporter: Vivian Steller Assignee: Ondrej Chytil
Resolution: Fixed Votes: 0
Labels: versioning
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Text File MAGNOLIA-3288-ContentVersionTest.java     Text File MAGNOLIA-3288-magnolia-core.patch    
Template:
Patch included:
Yes
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Testcase included:
Yes

 Description   

Technically spoken it looks as if ContentVersion doesn't return any node data of type binary.

Steps to reproduce (code):
1. create a mgnl:contentNode called 'X'
2. create a binary node data within X called 'B'
3. call X.addVersion(rule) with a rule that accepts 'mgnl:resource' nodes
-> version gets created correctly, also containing the binary (use JCR query tool to visualize the version node that you get when debugging X.getContentVersion("1.0").getPath())
4. call X.getContentVersion("1.0").getNodeData('B').getValue()
-> returns null, expected was returning the binary value of node data B

Steps to reproduce (visually, just quoting philipp here, please ask him in case of problems reproducing):
1. login to a fresh mgnl 4.3.6 instance, go to DMS
2. upload a document (binary, e.g. image), activate and evtl. accept the activation in the inbox
-> creates a version of that document
3. go to the version history of the newly created document and select the previously created version
-> no data is displayed, expected was seeing the image you just versioned

Philipp mentioned there could be a bug in ContentVersion aka. the ContentVersion$FixParentContentWrapper aka. ContentWrapper that fails to retrieve the binary from a node in the version workspace.



 Comments   
Comment by Vivian Steller [ 03/Sep/10 ]

added test case that can be used to verify the correct behavior

Comment by Vivian Steller [ 03/Sep/10 ]

added patch to fix the issue.

The code overwritten methods in ContentVersion are just dump copies from the superclass DefaultContent, replacing the snippet:

(1) this.node.getNode(name).isNodeType(ItemType.NT_RESOURCE)

with

(2) this.node.getNode(name).getProperty("jcr:frozenPrimaryType").getValue().getString().equals(ItemType.NT_RESOURCE)

In (1) the node type is checked directly on the node. However, when it is a versioned content it is not sufficient to check the node type, since this one returns "jcr:frozenNode" for the versioned binary node data. The original node type is stored within the "jcr:frozenPrimaryType" node property, which correctly returns "mgnl:resource".

Comment by Vivian Steller [ 03/Sep/10 ]

adding another method to the test that verifies that retrieving a binary from the restored version of a node works.

Comment by Vivian Steller [ 08/Sep/10 ]

just found another related issue: the patch also needs to be applied to DefaultContent not only to ContentVersion. This is because (strangely) the ContenVersion returns wrapped DefaultContent objects for its children. IMO ContentVersion should also return other wrapped ContentVersions instead of DefaultContent.

so either 1) let contentversion return wrapped ContentVersion objects for its children or 2) also apply the patch to DefaultContent as well (I'd prefer 1. since DefaultContent should not be concerned with version specific behavior at all)

> Jan: add 1) ... I tried this in the past but it causes other issues

Comment by Vivian Steller [ 09/Sep/10 ]

updated test case and patch

Comment by Vivian Steller [ 09/Sep/10 ]

Okay, I just updated the test case and patch.

Applying the test case to magnolia-core-4.3.6 now shows 2 failing tests.

For the patch I chose option 2) since Jan suspected option 1) to work. Now, when applying the patch to magnolia 4.3 branch (along with the testcase) all existing tests and the 2 new ones succeed.

If you'd take a look at the patches and drop me a message I could just commit those changes into mgnl 4.3 branch.

Note: we could possibly avoid patching DefaultContent by changing the FixedParentContentWrapper contained in ContentVersion. I'll try to do this next (as discussed with Philipp) and let you know the results.

Comment by Vivian Steller [ 09/Sep/10 ]

Rewriting ContentVersion$FixedParentContentWrapper doesn't seem to help a lot since we would have to copy a bunch of methods from DefaultContent into the Wrapper, which would just be dump code duplication. For now I'd leave it as it is, fixing DefaultContent to deal with FrozenNodes. However, as a suggestion, you might want to consider externalizing the protected method DefaultContent.determineNodeDataType(String name) (plus some more snippets in DefaultContent where the NodeData's type is determined) into another, reusable utility class.

Comment by Vivian Steller [ 13/Sep/10 ]

thanks very much for the quick integration of the patch!

Generated at Mon Feb 12 03:45:02 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.