Merging of the maps results in incomplete final map, having incorrectly determined entries about aem-classification HOT 8 CLOSED

@henrykuijpers Thanks for the report. Which one do you mean by

latest aem-content-classifications jar

Can you give the concrete version? At least the source maps look all correct to me:

• for 6.5.3.0:

  aem-classification/aem-classification-maps/aem-classification-map-repo-annotations/src/main/resources/biz/netcentric/filevault/validator/maps/aem-classification-map-repo-annotations.map

  Line 1074 in 48390c0

  /libs/granite/ui/components/coral/foundation/form/field,PUBLIC

• for 2021.6.5586.20210628T210726Z:

  aem-classification/aem-classification-maps/aem-classification-map-repo-annotations/src/main/resources/biz/netcentric/filevault/validator/maps/aem-classification-map-repo-annotations.map

  Line 808 in 05e2b59

  /libs/granite/ui/components/coral/foundation/form/field,PUBLIC

from aem-classification.

Hi @kwin , we further debugged the issue. There seems to be an issue with the merging of the maps, and also something with determining whether the parent is more restrictive or not. We're using the following configuration:

<plugin>
                    <groupId>org.apache.jackrabbit</groupId>
                    <artifactId>filevault-package-maven-plugin</artifactId>
                    <configuration>
                        ...
                        <validatorsSettings>
                            <netcentric-aem-classification>
                                <options>
                                    <maps>tccl:biz/netcentric/filevault/validator/maps/aem-classification-map-deprecations/graniteuideprecations.map,tccl:biz/netcentric/filevault/validator/maps/aem-classification-map-repo-annotations.map</maps>
                                </options>
                            </netcentric-aem-classification>
                        </validatorsSettings>
                    </configuration>
                    <dependencies>
                        <dependency>
                            <groupId>biz.netcentric.filevault.validator</groupId>
                            <artifactId>aem-classification-validator</artifactId>
                            <version>1.0.1</version>
                        </dependency>
                        <!-- the dependency containing the actual classification map -->
                        <dependency>
                            <groupId>biz.netcentric.filevault.validator.maps</groupId>
                            <artifactId>aem-classification-map-repo-annotations</artifactId>
                            <version>6.5.3.0</version>
                        </dependency>
                        <dependency>
                            <groupId>biz.netcentric.filevault.validator.maps</groupId>
                            <artifactId>aem-classification-map-deprecations</artifactId>
                            <version>6.5.0.0</version>
                        </dependency>
                    </dependencies>

There seems to be an issue on this line:
https://github.com/Netcentric/aem-classification/blob/master/aem-classification-validator/src/main/java/biz/netcentric/filevault/validator/aem/classification/ContentClassificationMapperImpl.java#L244

The 2 ordinal values that are compared are the same, which causes the item to not be added. This is because the map by default contains 4 entries, including this one:

When the tool checks for the restriction on /libs/granite/ui/components/coral/foundation/form/field, it does not find a match. It should probably return null there. However, instead of doing that, it checks if there is a parent restriction. It does so by continuing to check the parents until it is at the root node. The root node, however, is defined and is PUBLIC. Since the field-node is also PUBLIC, the ordinals are indeed the same. This causes the item to never be added to the map.

Later on, checks are done and the field-node is being checked. It is however not there, due to the merging issue. Then, the parent of the field-node is checked and unfortunately the parent of the field node (the form-node) is FINAL. This in the end causes the validator to report the error.

from aem-classification.

@henrykuijpers Thanks for the detailed analysis. Indeed the merge does not behave correctly. I think we actually need to keep all maps as is (and don't try to do any premature optimization) and do the check of the actual content classification for each map individually. Then the most restrictive one wins (no matter how long the matched resource type prefix). I will therefore introduce a new CompositeContentClassificationMap which implements such a logic.

from aem-classification.

That was incredibly fast! Thank you so much for fixing it! I was thinking of having a go at making a fix and providing a PR, but it would have taken me so much longer. Any idea when the new version can be released? We'd like to use it. :)

We're still wondering how volatile the classifications are, and therefore wondering if we should create a new map with every service pack install.
Or are the classifications maintained in such a way that the latest version reflects the latest AEM product's updates?

from aem-classification.

@henrykuijpers
Can you test the version 1.1.0-SNAPSHOT of aem-classification-validator from the OSSRH Snapshot repository at https://oss.sonatype.org/content/repositories/snapshots?

Regarding

if we should create a new map with every service pack install.

Usually those are very stable and won't change in SP updates. But I will soon cross-check for AEM 6.5.13 whether an update is necessary.

from aem-classification.

@kwin Tested with @henrykuijpers, your fix works. Thank you for the quick responses!

from aem-classification.

@henrykuijpers I now released updated versions of the maps for 6.5.13 and the newest SDK (https://repo1.maven.org/maven2/biz/netcentric/filevault/validator/maps/aem-classification-map-repo-annotations/) because there were indeed some changes from the previous maps.

from aem-classification.

@kwin I smoke tested the 6.5.13 list a bit, it looks good to me! I noticed there was 1 node that was removed from AEM 6.5.13 and thus is not in the list (was on the list as INTERNAL). It seems logical to me that once the node is removed from AEM, it will also be removed from the list.

Thank you so much for all the updates!

from aem-classification.