geonetwork / core-geonetwork

GeoNetwork is a catalog application to manage spatially referenced resources. It provides powerful metadata editing and search functions as well as an interactive web map viewer. It is currently used in numerous Spatial Data Infrastructure initiatives across the world.
http://geonetwork-opensource.org/
GNU General Public License v2.0
430 stars 489 forks source link

Batch editing - In XPATH replace mode don't try to add a node if doesn't exist #8350

Open josegar74 opened 2 months ago

josegar74 commented 2 months ago

The Batch Edit API uses the following setting to add an element in XPATH replace mode, if the XML node doesn't exist:

add-non-existing-element

This doesn't make much sense, since in replacement mode you want to update an element if it exists, otherwise do nothing.

This pull request updates the code to not use that setting and never try to add the element in XPATH replace mode.

Test case:

1) Create an ISO19139 metadata and add a dataset contact with the name John and role owner 2) In the batch edit configure a XPATH replacement:

batch-edit-form

The previous change match the element and changes the individual name to Mary

       <gmd:pointOfContact>
          <gmd:CI_ResponsibleParty>
            <gmd:individualName>
              <gco:CharacterString>Mary</gco:CharacterString>
            </gmd:individualName>

3) Edit XPATH expression to match another role: pointOfContact, that doesn't exist in the metadata:

/gmd:MD_Metadata/gmd:identificationInfo/*/gmd:pointOfContact[*/gmd:role/gmd:CI_RoleCode/@codeListValue='pointOfContact' and */gmd:individualName/*/text()='John']/*/gmd:individualName/gco:CharacterString


The pull request includes Sonarlint improvements.


For existing installations, a workaround to avoid this problem is to disable the settings listed at the beginning, unde r Admin console > Settings > CSW


Checklist

josegar74 commented 2 months ago

@fxprunayre this change breaks some unit tests, like BatchEditsServiceTest.testUpdateRecordAddAttribute:323 expected:<value> but was:<null>

The test is doing this replacement:

https://github.com/geonetwork/core-geonetwork/blob/ee23e5430ef839c90ab6d08890e1176481465741/services/src/test/java/org/fao/geonet/services/metadata/BatchEditsServiceTest.java#L296-L301

I would expect that in the following code the isCreate variable would be true, but that is not the case, so with this change, no longer enters in the condition as createXpathNodeIfNotExist is false and the test fails:

https://github.com/geonetwork/core-geonetwork/blob/ee23e5430ef839c90ab6d08890e1176481465741/core/src/main/java/org/fao/geonet/kernel/EditLib.java#L518-L519

isCreate is false as doesn't check for the tag gn_add, but gn_create.

https://github.com/geonetwork/core-geonetwork/blob/ee23e5430ef839c90ab6d08890e1176481465741/core/src/main/java/org/fao/geonet/kernel/EditLib.java#L505-L507

The difference seems a bit subtle in the documentation, but gn_add is kind of special case of gn_create:

https://github.com/geonetwork/core-geonetwork/blob/ee23e5430ef839c90ab6d08890e1176481465741/core/src/main/java/org/fao/geonet/kernel/EditLib.java#L1821-L1828

I'm afraid changing the code in https://github.com/geonetwork/core-geonetwork/blob/ee23e5430ef839c90ab6d08890e1176481465741/core/src/main/java/org/fao/geonet/kernel/EditLib.java#L505-L507 to check both probably will cause additional unwanted side effects?

fxprunayre commented 2 months ago

gn_add/create don't have effect on attribute which are always text. So a proper fix could be to change AddElemValue to extract that in a specific property... or we can also check if the target is an attribute and in this case enable creation mode.