[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: |
|
| 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): Steps to reproduce (visually, just quoting philipp here, please ask him in case of problems reproducing): 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! |