[GEOT-7559] App-Schema throws an error when mappings have duplicate names, even when they come from includes #4730

turingtestfail · 2024-04-24T15:59:07Z

deep equality trimmed down

fixed equality compare and started on test

Cleanup

more cleanup

moved equal to local

Checklist

I have read the contribution guidelines.
I have sent a Contribution Licence Agreement (not required for small changes, e.g., fixing typos in documentation).
First PR targets the main branch (backports managed later; ignore for branch specific issues).
Avoid Java 9+ split packages.
All the build checks are green (see automated QA checks).

For core and extension modules:

New unit tests have been added covering the changes.
Documentation has been updated (if change is visible to end users).
There is an issue in GeoTools Jira (except for changes not visible to end users).
Commit message(s) must be in the form [GEOT-XYZW] Title of the Jira ticket.
Bug fixes and small new features are presented as a single commit.
The commit targets a single objective (if multiple focuses cannot be avoided, each one is in its own commit, and has a separate ticket describing it).

nmco

Thx @turingtestfail, I add some review comments.

nmco · 2024-05-03T22:31:02Z

...p-schema/src/main/java/org/geotools/data/complex/config/AppSchemaDataAccessConfigurator.java

@@ -121,6 +121,9 @@ public class AppSchemaDataAccessConfigurator {

    public static final String PROPERTY_REPLACE_OR_UNION = "app-schema.orUnionReplace";

+    /** Whether the mapping is for an include. */
+    private final Boolean isInclude;


Wondering, do we have a reason to make the isInclude optional? I guess by default we could assume false.

There were a lot of tests that have calls to the method so to avoid a lot of rewrites I overloaded the method with this as an argument. The latest push switched it to have an Optional wrapper to make it more explicit. Does that work?

Switched to plain boolean primitive type with default of false.

nmco · 2024-05-03T22:50:07Z

...nsion/app-schema/app-schema/src/main/java/org/geotools/data/complex/AppSchemaDataAccess.java

@@ -169,6 +179,57 @@ public AppSchemaDataAccess(Set<FeatureTypeMapping> mappings, boolean hidden)
        register();
    }

+    /**
+     * Checks if the mapping is an include and is identical to the previous one.


I would suggest adding more details to this method, along the lines of:

If the provided feature type mapping does not result from an include directive, we return false immediately. We then check if a previous mapping has already been registered for the provided feature type name. If that's the case, we verify if the new provided mapping is deeply equal to the already registered one. If this comparison holds true, we return true.

The objective here is to ensure that if a feature type mapping has already included this specific feature type mapping, it is indeed the same mapping definition. The intention is to prevent different mappings for the same feature type from coexisting.

Comments added

nmco · 2024-05-03T22:59:40Z

...nsion/app-schema/app-schema/src/main/java/org/geotools/data/complex/AppSchemaDataAccess.java

+        }
+        FeatureTypeMapping compareMapping = null;
+        // Get the mapping to compare with
+        if (mapping.getMappingName() != null) {


If my memory serves me well, in App-Schema, a feature type mapping is identified by the target element's qualified name, or optionally by the mapping name. The typical scenario involves defining a mapping for a target element, and that's usually sufficient. However, there are certain scenarios where we might require different mappings for the same target element. In such cases, we can provide a mapping name to disambiguate between them.

The current logic appears to rely solely on the presence of a mapping name, which is optional. This implies that include types must mandatorily have a mapping name, which is not the case. Therefore, I would suggest reviewing the logic to search for corresponding mappings by using the provided mapping name if available, and if not, by the target element.

I assume that currently, if two different mappings include the same mapping without defining a mapping name, it would raise a complaint about a duplicate mapping for the same target element. This PR aims to rectify such issues, assuming my understanding is correct.

Added logic to check of target element.

nmco · 2024-05-03T23:14:24Z

...nsion/app-schema/app-schema/src/main/java/org/geotools/data/complex/AppSchemaDataAccess.java

+                                    + " while trying to test for duplicates");
+                    return false;
+                }
+            }


I'm uncertain about this aspect, but if my understanding is accurate, the objective is to prevent App-Schema from raising errors about duplicate mappings when the exact same feature type mapping is encountered multiple times due to being included by different mappings.

It appears that we're conducting a search in two different places:

Firstly, in the mappings hashmap of this AppSchemaDataAccess instance. I presume that an AppSchemaDataAccess instance is associated with a root mapping in App-Schema.

Secondly, in the DataAccessRegistry registry, which encompasses all the root mappings registered in App-Schema.

The search for duplicates encompasses both:

if (this.mappings.containsKey(name) || DataAccessRegistry.hasName(name)) {

I would suggest adding comments in the code to elucidate the process. It seems that the initial check is performed within the current mappings, as if the mapping is found there, it implies it has already been validated against the common registry.

Comments added.

nmco · 2024-05-03T23:19:42Z

...nsion/app-schema/app-schema/src/main/java/org/geotools/data/complex/AppSchemaDataAccess.java

+            FeatureTypeMapping mapping, FeatureTypeMapping compareMapping) {
+        if (mapping == compareMapping) return true;
+        return Objects.equals(mapping.getAttributeMappings(), compareMapping.getAttributeMappings())
+                && Objects.equals(mapping.getMappingName(), compareMapping.getMappingName());


The same comment applies here as above. So, it's not only the mapping name, which is optional, but we also need to check the target element name.

That said, should we also check for other properties of the mappings? Such as data sources, normalization, etc.? Would it make more sense to implement this equality check in the FeatureTypeMapping class itself?

Target check added. Data source id and normalization comparisons added. Previously this was the equals method in FeatureTypeMapping but the accompanying hash method was causing test failures so I moved it here.

Actually I just pushed a version with the logic moved to the equals method in FeatureTypeMapping and a corrected hash method and the tests are passing.

…ames, even when they come from includes deep equality trimmed down fixed equality compare and started on test Cleanup more cleanup moved equal to local PR responses moved to equals switch to primitive boolean

sikeoka · 2024-05-20T16:20:28Z

This PR may be failing GeoServer builds due to an API change in AppSchemaDataAccessConfigurator:
Error: 8,713 [ERROR] /home/runner/work/geoserver/geoserver/src/community/smart-data-loader/src/main/java/org/geoserver/smartdataloader/data/SmartDataLoaderDataAccessFactory.java:[205,74] incompatible types: org.geotools.data.complex.config.DataAccessMap cannot be converted to boolean

turingtestfail · 2024-05-20T17:18:42Z

Yes, was made aware of that today. This PR provides a fix on the GeoServer side: geoserver/geoserver#7633

…

On Mon, May 20, 2024 at 12:20 PM Steve Ikeoka ***@***.***> wrote: This PR may be failing GeoServer builds due to an API change in AppSchemaDataAccessConfigurator: Error: 8,713 [ERROR] /home/runner/work/geoserver/geoserver/src/community/smart-data-loader/src/main/java/org/geoserver/smartdataloader/data/SmartDataLoaderDataAccessFactory.java:[205,74] incompatible types: org.geotools.data.complex.config.DataAccessMap cannot be converted to boolean — Reply to this email directly, view it on GitHub <#4730 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHRJZ4XLT2X2GPPOTEG24YTZDIPGHAVCNFSM6AAAAABGXGR3WSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRQG43TSMJVHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

turingtestfail requested review from nmco and aaime April 24, 2024 15:59

nmco reviewed May 3, 2024

View reviewed changes

turingtestfail force-pushed the GEOT-7559-App-Schema-duplicate-mappings-includes branch 2 times, most recently from 054a5a5 to 172088e Compare May 8, 2024 14:34

[GEOT-7559] App-Schema throws an error when mappings have duplicate n…

ca597fb

…ames, even when they come from includes deep equality trimmed down fixed equality compare and started on test Cleanup more cleanup moved equal to local PR responses moved to equals switch to primitive boolean

turingtestfail force-pushed the GEOT-7559-App-Schema-duplicate-mappings-includes branch from 172088e to ca597fb Compare May 13, 2024 14:38

aaime mentioned this pull request May 14, 2024

[GEOS-11397] App-Schema Includes fix Integration Tests geoserver/geoserver#7633

Merged

11 tasks

nmco merged commit 2e298e1 into geotools:main May 17, 2024
21 checks passed

turingtestfail added backport 30.x Backport bot will backport to 30.x on merge backport 31.x Automatic backport to 31.x branch labels May 20, 2024

This was referenced May 20, 2024

[GEOS-11407] Upgrade jackson from 2.15.2 to 2.17.1 geoserver/geoserver#7664

Merged

Fix smart-data-loader compilation error geoserver/geoserver#7665

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GEOT-7559] App-Schema throws an error when mappings have duplicate names, even when they come from includes #4730

[GEOT-7559] App-Schema throws an error when mappings have duplicate names, even when they come from includes #4730

turingtestfail commented Apr 24, 2024 •

edited

nmco left a comment

nmco May 3, 2024

turingtestfail May 7, 2024

turingtestfail May 13, 2024

nmco May 3, 2024 •

edited

turingtestfail May 7, 2024

nmco May 3, 2024

turingtestfail May 7, 2024

nmco May 3, 2024

turingtestfail May 7, 2024

nmco May 3, 2024

turingtestfail May 7, 2024

turingtestfail May 8, 2024

sikeoka commented May 20, 2024

turingtestfail commented May 20, 2024 via email

[GEOT-7559] App-Schema throws an error when mappings have duplicate names, even when they come from includes #4730

[GEOT-7559] App-Schema throws an error when mappings have duplicate names, even when they come from includes #4730

Conversation

turingtestfail commented Apr 24, 2024 • edited

Checklist

nmco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmco May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sikeoka commented May 20, 2024

turingtestfail commented May 20, 2024 via email

turingtestfail commented Apr 24, 2024 •

edited

nmco May 3, 2024 •

edited