#1609 - Exclude seed property and facets subtree from ensure oak index comparison

[davidjgonzalez]

EnsureOakIndex excludes property seed, and sub-tree [oak:QueryIndexDefinition]/facets/jcr:content, by way up an updated to ChecksumGeneratorImpl that allows specific excludedNodeNames and excludedSubTrees.

Glossary

• Oak Index Definition = OID
• Ensure Oak Index Definition = EOID

Handling the "seed" property.

The seed property ("seed") is set as an Mandatory Ignored Property, meaning it's existence and value are ignored for both:

1. Comparing the OID and the EOID
2. During the update of the OID from the EOID.

Ensure Oak Index will completely ignore this property on the OID entirely and must be managed directly on the OID itself (EOID cannot add/remove/update this property, and does not understand that it even exists).

Handling the "facets" subtree

The following subtrees are set as Mandatory Excluded SubTrees meaning they are excluded while determining if the OID and EOID are different and need updating.

• [oak:QueryIndexDefinition]/facets/jcr:content <- for the OID
• [oak:Unstructured]/facets/jcr:content <- for the EOID

EOID should never define facets/jcr:content but rather only facets. (Requires documentation)

The following will indicate a difference between the OID and EOID:

• The existence mismatch of the facets node in either the OID/EOID
• A mismatch in ANY property on the facets node between the OID/EOID

When the OID is updated based on the EOID, the OID's [oak:QueryIndexDefinition]/facets will be entirely replaced with the EOID's [oak:Unstructured]/facets/jcr:content

[davidjgonzalez]

KelvinXu can you review the mandatory excludes?

[kelvinxzf]

Shouldn't these two lines also be added to oakIndexOptions? Also notice this.excludeSubTrees is based on NT_OAK_QUERY_INDEX_DEFINITION, which is meant for oakIndexOptions if I am not mistaken. Essentially we may have to have two excludeSubTrees based on NT_OAK_UNSTRUCTURED and NT_OAK_QUERY_INDEX_DEFINITION and added to the two options respectively?

[davidjgonzalez]

right you are! and thats why @kelvinxzf should always review my PR's! :)

[kelvinxzf]

Maybe you need the following as well?
oakIndexOptions.addExcludedSubTrees(this.excludeSubTrees); oakIndexOptions.addExcludedNodeNames(this.excludeNodeNames);

[davidjgonzalez]

Have to update EOI side of excludes

[davidjgonzalez]

@kelvinxzf @catholicon - Updated the Code and the Issue description to match the the current state.
I think the "big" thing is the EOID will "dumbly" copy all its facets content's into the OID's facets node, however nothing on or under facets/jcr:content will TRIGGER an update.

WDYT?

[catholicon]

@davidjgonzalez, I wonder if it might be interesting if EOID could be defined such that it has facets/jcr:content@mandatoryExcluded=true. The obvious issue with the suggestion is that there's no reason to treat mandatoryExcluded specially. Also, that each customer would've to define it. The only reason I mention this is that the nodes that can theoretically spawn under facets may not always be jcr:content - those structures are relative path to a faceted multi valued property. Admittedly though, in almost all practical AEM cases it would indeed be jcr:content.

Barring this theoretical suggestion, the idea seems correct to me (I just read the comments - not the code though)

[davidjgonzalez]

@catholicon

1. That requires non-trivial updates to the ChecksumGenerator code to detect changes (which is... oof!)
2. That requires changes to the EnsureOakIndex's update code (probably a re-write of how that piece)

Perhaps how the copy happens needs to get re-architected for these flexible use cases... which would likely result in not needing the ChecksumGenerator (since a more advance tree walk could compute that)

We could look at it, but we'd probably scrap this PR entirely and rethink this.

[catholicon]

Oh... Then don't worry about it. I was imagining in terms of like diff where it often is easy to skip traversing further into the tree if some condition turns up.

[davidjgonzalez]

@catholicon im totally fine w scrapping a PR if its the right move :) Im not sure what the timing around this is ... or if we want to use this as a Temp fix and define an updated delta-detection/update strategy.

[kelvinxzf]

@davidjgonzalez if it is just a matter of populating excludeSubTrees and excludeNodeNames, we can potentially use the same way as how properties.ignore is passed (via EnsureOakIndexManagerImpl). Ultimately coming from,

<jcr:root xmlns:sling="http://sling.apache.org/jcr/sling/1.0" xmlns:cq="http://www.day.com/jcr/cq/1.0"
xmlns:jcr="http://www.jcp.org/jcr/1.0" xmlns:nt="http://www.jcp.org/jcr/nt/1.0"
jcr:primaryType="sling:OsgiConfig"
properties.ignore="[myDescription,ignoreMe]"
nodeNames.ignore="[node1,node2]"
subTrees.ignore="[damAssetLucene/facets/jcr:content]"
/>

For the simplification purpose, can we also assume everything defined above would be ignored on both sides?

[kelvinxzf]

Yeah timing wise, the configurable way can be coming later...

[catholicon]

@davidjgonzalez, no I don't think the current implementation is incorrect step. I just meant to say that it'd be nice to have some configuration to say which nodes to avoid under facet node. In that regard, I like @kelvinxzf's idea too. The default config could have facet/jcr:content but can be overriden if required.

[kelvinxzf]

@davidjgonzalez @catholicon I quickly tested this branch and it seems working as expected...seed and facets\jcr:content are successfully ignored during diff calculation, in turn update or override is not triggered. Just to confirm, when the overriding really happens due to any legitimate changes outside of seed or facets\jcr:content, EOI would do a "dumb" copy as David stated before (in such case facets\jcr:content would be wiped out). For the timing being, if I understand correctly, this is how this feature would be delivered as of now?

[davidjgonzalez]

@kelvinxzf that is my understanding of the correct and expected behavior, but @catholicon is the super-authority!

Thanks for taking the time to work through this - your help has been very appreciated.

[coveralls]

Pull Request Test Coverage Report for Build 3557

• 70 of 91 (76.92%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 42.332%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
bundle/src/main/java/com/adobe/acs/commons/analysis/jcrchecksum/ChecksumGeneratorOptions.java	0	2	0.0%
bundle/src/main/java/com/adobe/acs/commons/oak/impl/EnsureOakIndexJobHandler.java	7	11	63.64%
bundle/src/main/java/com/adobe/acs/commons/analysis/jcrchecksum/impl/options/CustomChecksumGeneratorOptions.java	4	10	40.0%
bundle/src/main/java/com/adobe/acs/commons/analysis/jcrchecksum/impl/ChecksumGeneratorImpl.java	47	56	83.93%

Totals
Change from base Build 3556:	0.0%
Covered Lines:	9688
Relevant Lines:	22886

---

💛 - Coveralls