From 343ae0f3f6e0ae51866acb93aea62eeedfbf5165 Mon Sep 17 00:00:00 2001 From: Victor Ramperez Date: Wed, 5 Aug 2026 14:36:32 +0200 Subject: [PATCH] fix(repository): re-escape reserved words on the read-merge-write update path The replaceOnUpdate path (needed for Scorpio 6.x, which appends instead of replacing on POST /entities/{id}/attrs) reads the existing entity back from the broker and writes it out again via batch upsert. That read goes through the EscapeCleaningParser of the mapping library, which strips the `tmfEscaped-` prefix from every reserved word it can safely unescape - i.e. all of them except the VO field collisions `id`, `type` and `value`. Nothing put the prefix back before the entity was written, so raw JSON-LD keywords ended up on the wire. Scorpio >= 6.0.0 drops `@id`/`@type`/`@value`/`@context` inside Property values, so any free-form value carrying expanded JSON-LD was silently mangled on the first update. Concretely: the ODRL policy of a product-offering-price lost its `@type`/`@id`, which made the EDC fail to map the offer and left contract negotiations hanging in REQUESTED. Re-apply the escape recursively (property values, sub-attributes and multi-attributes) before the merged entity goes out. escapeReservedWords is idempotent, so keys that kept their prefix during parsing are untouched. Also guard the merge against an update without additional properties, which used to NPE. Co-Authored-By: Claude Opus 5 (1M context) --- .../repository/NgsiLdBaseRepository.java | 94 ++++++++- .../NgsiLdBaseRepositoryUpdateTest.java | 184 ++++++++++++++++++ 2 files changed, 277 insertions(+), 1 deletion(-) create mode 100644 common/src/test/java/org/fiware/tmforum/common/repository/NgsiLdBaseRepositoryUpdateTest.java diff --git a/common/src/main/java/org/fiware/tmforum/common/repository/NgsiLdBaseRepository.java b/common/src/main/java/org/fiware/tmforum/common/repository/NgsiLdBaseRepository.java index f78418aa..58603612 100644 --- a/common/src/main/java/org/fiware/tmforum/common/repository/NgsiLdBaseRepository.java +++ b/common/src/main/java/org/fiware/tmforum/common/repository/NgsiLdBaseRepository.java @@ -2,6 +2,7 @@ import io.github.wistefan.mapping.EntityVOMapper; import io.github.wistefan.mapping.JavaObjectMapper; +import io.github.wistefan.mapping.ReservedWordHandler; import io.github.wistefan.mapping.annotations.MappingEnabled; import io.micronaut.cache.annotation.CacheInvalidate; import io.micronaut.cache.annotation.CachePut; @@ -12,9 +13,13 @@ import lombok.RequiredArgsConstructor; import org.fiware.ngsi.api.EntitiesApiClient; import org.fiware.ngsi.api.SubscriptionsApiClient; +import org.fiware.ngsi.model.AdditionalPropertyVO; import org.fiware.ngsi.model.EntityFragmentVO; import org.fiware.ngsi.model.EntityListVO; import org.fiware.ngsi.model.EntityVO; +import org.fiware.ngsi.model.GeoPropertyVO; +import org.fiware.ngsi.model.PropertyVO; +import org.fiware.ngsi.model.RelationshipVO; import org.fiware.ngsi.model.SubscriptionVO; import org.fiware.tmforum.common.CommonConstants; import org.fiware.tmforum.common.caching.EntityIdKeyGenerator; @@ -29,8 +34,11 @@ import java.net.URI; import java.util.Arrays; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import java.util.Optional; +import java.util.function.BiConsumer; import java.util.function.Function; import java.util.stream.Stream; @@ -186,7 +194,17 @@ public Mono updateDomainEntity(String id, T domainEntity) { } private EntityVO mergeForUpdate(EntityVO existing, EntityVO update) { - update.getAdditionalProperties().forEach(existing::setAdditionalProperties); + // `existing` was parsed by EscapeCleaningParser, which strips the tmfEscaped- prefix + // from reserved words it can safely unescape (everything except VO_FIELD_COLLISIONS). + // Writing that back verbatim would put raw JSON-LD keywords (@id/@type/@value/@context) + // on the wire; brokers are free to drop them (Scorpio >= 6.0.0 does), which silently + // destroys nested free-form values such as an expanded ODRL policy. Re-apply the escape + // before the entity goes out again. Only relevant on the replaceOnUpdate path, since + // PATCH /attrs sends the freshly mapped `update` only, which JavaObjectMapper escapes. + reEscapeReservedWords(existing); + if (update.getAdditionalProperties() != null) { + update.getAdditionalProperties().forEach(existing::setAdditionalProperties); + } if (update.getLocation() != null) existing.setLocation(update.getLocation()); if (update.getObservationSpace() != null) existing.setObservationSpace(update.getObservationSpace()); if (update.getOperationSpace() != null) existing.setOperationSpace(update.getOperationSpace()); @@ -195,6 +213,80 @@ private EntityVO mergeForUpdate(EntityVO existing, EntityVO update) { return existing; } + /** + * Re-apply the reserved-word escape to the additional properties of an entity that was read + * back from the broker, recursively. Counterpart of the {@code EscapeCleaningParser} of the + * mapping library, which removes the prefix while parsing. {@code escapeReservedWords} is + * idempotent, so keys that kept their prefix during parsing (the VO field collisions + * {@code id}/{@code type}/{@code value}) are left untouched. + * + * @param entityVO the entity to fix up in place + */ + private void reEscapeReservedWords(EntityVO entityVO) { + reEscapeProperties(entityVO.getAdditionalProperties(), entityVO::setAdditionalProperties); + } + + /** + * Rewrite the keys of an additional-properties map in place and recurse into its values. + * + * @param properties the (potentially null) map to rewrite + * @param setter the {@code setAdditionalProperties} of the owning VO, used to re-insert + */ + private void reEscapeProperties(Map properties, + BiConsumer setter) { + if (properties == null || properties.isEmpty()) { + return; + } + Map reEscaped = new LinkedHashMap<>(); + properties.forEach((key, value) -> reEscaped.put(ReservedWordHandler.escapeReservedWords(key), + reEscapeAdditionalProperty(value))); + properties.clear(); + reEscaped.forEach(setter); + } + + private AdditionalPropertyVO reEscapeAdditionalProperty(AdditionalPropertyVO additionalPropertyVO) { + // the generated VOs do not share an accessor for the additional properties, so every + // concrete type has to be handled explicitly. + if (additionalPropertyVO instanceof PropertyVO propertyVO) { + propertyVO.setValue(reEscapeValue(propertyVO.getValue())); + reEscapeProperties(propertyVO.getAdditionalProperties(), propertyVO::setAdditionalProperties); + } else if (additionalPropertyVO instanceof RelationshipVO relationshipVO) { + reEscapeProperties(relationshipVO.getAdditionalProperties(), relationshipVO::setAdditionalProperties); + } else if (additionalPropertyVO instanceof GeoPropertyVO geoPropertyVO) { + reEscapeProperties(geoPropertyVO.getAdditionalProperties(), geoPropertyVO::setAdditionalProperties); + } else if (additionalPropertyVO instanceof List multiAttribute) { + // multi-attributes: PropertyListVO, RelationshipListVO and GeoPropertyListVO are lists + // of the types handled above. + multiAttribute.stream() + .filter(AdditionalPropertyVO.class::isInstance) + .map(AdditionalPropertyVO.class::cast) + .forEach(this::reEscapeAdditionalProperty); + } + return additionalPropertyVO; + } + + /** + * Free-form property values are parsed into plain maps and lists, with their keys cleaned by + * the same parser, so they need the escape re-applied too. This is what carries e.g. the + * expanded ODRL policy of a product-offering-price and the reason the entity has to be fixed + * up at all. + * + * @param value the value to fix up + * @return the value with all reserved keys escaped again + */ + private Object reEscapeValue(Object value) { + if (value instanceof Map valueMap) { + Map reEscaped = new LinkedHashMap<>(); + valueMap.forEach((key, nestedValue) -> reEscaped + .put(ReservedWordHandler.escapeReservedWords(String.valueOf(key)), reEscapeValue(nestedValue))); + return reEscaped; + } + if (value instanceof List valueList) { + return valueList.stream().map(this::reEscapeValue).toList(); + } + return value; + } + /** * Delete a domain entity * diff --git a/common/src/test/java/org/fiware/tmforum/common/repository/NgsiLdBaseRepositoryUpdateTest.java b/common/src/test/java/org/fiware/tmforum/common/repository/NgsiLdBaseRepositoryUpdateTest.java new file mode 100644 index 00000000..e358e29f --- /dev/null +++ b/common/src/test/java/org/fiware/tmforum/common/repository/NgsiLdBaseRepositoryUpdateTest.java @@ -0,0 +1,184 @@ +package org.fiware.tmforum.common.repository; + +import io.github.wistefan.mapping.JavaObjectMapper; +import io.micronaut.http.HttpResponse; +import org.fiware.ngsi.api.EntitiesApiClient; +import org.fiware.ngsi.model.BatchOperationResultVO; +import org.fiware.ngsi.model.EntityListVO; +import org.fiware.ngsi.model.EntityVO; +import org.fiware.ngsi.model.PropertyListVO; +import org.fiware.ngsi.model.PropertyVO; +import org.fiware.tmforum.common.configuration.GeneralProperties; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import reactor.core.publisher.Mono; + +import java.net.URI; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Tests for the read-merge-write update path ({@code replaceOnUpdate}, required for Scorpio 6.x). + */ +class NgsiLdBaseRepositoryUpdateTest { + + private static final String ENTITY_ID = "urn:ngsi-ld:product-offering-price:8de1a1f4-2e56-4e3e-8d5b-8a3fa4a0e6d1"; + + private GeneralProperties properties; + private EntitiesApiClient entitiesApi; + private JavaObjectMapper javaObjectMapper; + private TmForumRepository repository; + + @BeforeEach + public void setUp() { + properties = new GeneralProperties(); + properties.setReplaceOnUpdate(true); + entitiesApi = mock(EntitiesApiClient.class); + javaObjectMapper = mock(JavaObjectMapper.class); + repository = new TmForumRepository(properties, entitiesApi, null, null, null, javaObjectMapper); + when(entitiesApi.batchEntityUpsert(any(), any())) + .thenReturn(Mono.just(HttpResponse.ok(new BatchOperationResultVO()))); + } + + @Test + public void reservedWordsAreEscapedAgainBeforeTheEntityIsWrittenBack() { + // The EscapeCleaningParser of the mapping library strips the tmfEscaped- prefix from + // reserved words while the existing entity is read from the broker. Writing that back + // verbatim puts raw JSON-LD keywords on the wire, which Scorpio >= 6.0.0 drops - and with + // them the expanded ODRL policy of the offering price, which then fails to map in the EDC. + EntityVO existing = anEntity(); + PropertyVO policy = aProperty(Map.of( + "@type", List.of("http://www.w3.org/ns/odrl/2/Offer"), + "@id", "urn:uuid:1b0f3b8a-6c2a-4b6b-9d0a-2f0f6a1a3c4d")); + existing.setAdditionalProperties("policy", policy); + mockRetrieval(existing); + + repository.updateDomainEntity(ENTITY_ID, new Object()).block(); + + Map writtenPolicy = writtenValueOf("policy"); + assertEquals(Map.of( + "tmfEscaped-@type", List.of("http://www.w3.org/ns/odrl/2/Offer"), + "tmfEscaped-@id", "urn:uuid:1b0f3b8a-6c2a-4b6b-9d0a-2f0f6a1a3c4d"), + writtenPolicy, + "Keywords the parser unescaped on read have to be escaped again on write."); + } + + @Test + public void escapingIsAppliedRecursivelyAndIsIdempotent() { + // Nested objects, lists and the keys the parser deliberately leaves escaped + // (id/type/value collide with VO fields) all have to end up correct. + EntityVO existing = anEntity(); + existing.setAdditionalProperties("policy", aProperty(Map.of( + "@type", "Offer", + "tmfEscaped-id", "already-escaped", + "permission", List.of(Map.of( + "@id", "urn:uuid:permission", + "constraint", Map.of("@value", "5")))))); + mockRetrieval(existing); + + repository.updateDomainEntity(ENTITY_ID, new Object()).block(); + + Map written = writtenValueOf("policy"); + assertEquals("Offer", written.get("tmfEscaped-@type")); + assertEquals("already-escaped", written.get("tmfEscaped-id"), + "Keys that kept their prefix during parsing must not be escaped twice."); + Map permission = (Map) ((List) written.get("permission")).get(0); + assertEquals("urn:uuid:permission", permission.get("tmfEscaped-@id")); + assertEquals(Map.of("tmfEscaped-@value", "5"), permission.get("constraint")); + } + + @Test + public void escapingCoversSubAttributesAndMultiAttributes() { + EntityVO existing = anEntity(); + PropertyVO first = aProperty("first"); + first.setAdditionalProperties("@type", aProperty("sub-attribute")); + PropertyVO second = aProperty(Map.of("@id", "urn:uuid:second")); + PropertyListVO multiAttribute = new PropertyListVO(); + multiAttribute.add(first); + multiAttribute.add(second); + existing.setAdditionalProperties("relatedParty", multiAttribute); + mockRetrieval(existing); + + repository.updateDomainEntity(ENTITY_ID, new Object()).block(); + + PropertyListVO written = (PropertyListVO) writtenEntity().getAdditionalProperties().get("relatedParty"); + assertTrue(written.get(0).getAdditionalProperties().containsKey("tmfEscaped-@type"), + "Sub-attribute names are escaped as well."); + assertFalse(written.get(0).getAdditionalProperties().containsKey("@type")); + assertEquals(Map.of("tmfEscaped-@id", "urn:uuid:second"), written.get(1).getValue()); + } + + @Test + public void anUpdateWithoutAdditionalPropertiesDoesNotFail() { + // e.g. an update that only touches a structural field. The merge used to NPE here. + EntityVO existing = anEntity(); + existing.setAdditionalProperties("name", aProperty("an offering price")); + when(javaObjectMapper.toEntityVO(any())).thenReturn(new EntityVO()); + when(entitiesApi.retrieveEntityById(eq(URI.create(ENTITY_ID)), any(), any(), any(), any(), any())) + .thenReturn(Mono.just(HttpResponse.ok(existing))); + + repository.updateDomainEntity(ENTITY_ID, new Object()).block(); + + assertEquals("an offering price", + ((PropertyVO) writtenEntity().getAdditionalProperties().get("name")).getValue()); + } + + @Test + public void plainAttributesAreLeftAlone() { + EntityVO existing = anEntity(); + existing.setAdditionalProperties("name", aProperty("an offering price")); + mockRetrieval(existing); + + repository.updateDomainEntity(ENTITY_ID, new Object()).block(); + + EntityVO written = writtenEntity(); + assertEquals("an offering price", ((PropertyVO) written.getAdditionalProperties().get("name")).getValue()); + assertEquals("2026-08-05T10:15:30Z", + ((PropertyVO) written.getAdditionalProperties().get("lastUpdate")).getValue(), + "The update still has to be merged on top of the existing entity."); + } + + private EntityVO anEntity() { + EntityVO entityVO = new EntityVO(); + entityVO.setId(URI.create(ENTITY_ID)); + entityVO.setType("product-offering-price"); + return entityVO; + } + + private PropertyVO aProperty(Object value) { + PropertyVO propertyVO = new PropertyVO(); + // the mapping library parses free-form values into mutable maps, mirror that + propertyVO.setValue(value instanceof Map map ? new LinkedHashMap<>(map) : value); + return propertyVO; + } + + private void mockRetrieval(EntityVO existing) { + // what the domain object to be updated maps to - a partial entity carrying just the change + EntityVO update = new EntityVO(); + update.setAdditionalProperties("lastUpdate", aProperty("2026-08-05T10:15:30Z")); + when(javaObjectMapper.toEntityVO(any())).thenReturn(update); + when(entitiesApi.retrieveEntityById(eq(URI.create(ENTITY_ID)), any(), any(), any(), any(), any())) + .thenReturn(Mono.just(HttpResponse.ok(existing))); + } + + private EntityVO writtenEntity() { + ArgumentCaptor captor = ArgumentCaptor.forClass(EntityListVO.class); + verify(entitiesApi).batchEntityUpsert(captor.capture(), eq("replace")); + return captor.getValue().get(0); + } + + private Map writtenValueOf(String attribute) { + return (Map) ((PropertyVO) writtenEntity().getAdditionalProperties().get(attribute)).getValue(); + } +}