From 9e281d0ee511f3cb2589f1a882ddd301b798301d Mon Sep 17 00:00:00 2001
From: sonika-shah <58761340+sonika-shah@users.noreply.github.com>
Date: Wed, 18 Jun 2025 14:03:45 +0530
Subject: [PATCH] fix #21394 Custom properties bug update with existing wrong
 values (#21825)

* fix #21394 allow only updated extension fields to be validated

* fix #21394 add tests

* fix #21394 add tests
---
 .../service/jdbi3/EntityRepository.java       |  90 +++++++----
 .../glossary/GlossaryTermResourceTest.java    | 152 ++++++++++++++++++
 2 files changed, 211 insertions(+), 31 deletions(-)

diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java
index ec952732431..83c6819d507 100644
--- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java
+++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java
@@ -124,6 +124,7 @@ import java.util.function.BiConsumer;
 import java.util.function.BiPredicate;
 import java.util.function.Function;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
 import lombok.Getter;
 import lombok.NonNull;
@@ -1037,7 +1038,7 @@ public abstract class EntityRepository<T extends EntityInterface> {
     validateTags(entity);
     prepare(entity, update);
     setFullyQualifiedName(entity);
-    validateExtension(entity);
+    validateExtension(entity, update);
     // Domain is already validated
   }
 
@@ -1778,11 +1779,37 @@ public abstract class EntityRepository<T extends EntityInterface> {
     daoCollection.entityExtensionTimeSeriesDao().deleteBeforeTimestamp(fqn, extension, timestamp);
   }
 
-  private void validateExtension(T entity) {
+  private void validateExtension(T entity, Entry<String, JsonNode> field) {
     if (entity.getExtension() == null) {
       return;
     }
 
+    // Validate single extension field
+    JsonNode jsonNode = JsonUtils.valueToTree(entity.getExtension());
+    String fieldName = field.getKey();
+    JsonNode fieldValue = field.getValue();
+
+    JsonSchema jsonSchema = TypeRegistry.instance().getSchema(entityType, fieldName);
+    if (jsonSchema == null) {
+      throw new IllegalArgumentException(CatalogExceptionMessage.unknownCustomField(fieldName));
+    }
+    String customPropertyType = TypeRegistry.getCustomPropertyType(entityType, fieldName);
+    String propertyConfig = TypeRegistry.getCustomPropertyConfig(entityType, fieldName);
+    validateAndUpdateExtensionBasedOnPropertyType(
+        entity, (ObjectNode) jsonNode, fieldName, fieldValue, customPropertyType, propertyConfig);
+    Set<ValidationMessage> validationMessages = jsonSchema.validate(fieldValue);
+    if (!validationMessages.isEmpty()) {
+      throw new IllegalArgumentException(
+          CatalogExceptionMessage.jsonValidationError(fieldName, validationMessages.toString()));
+    }
+  }
+
+  private void validateExtension(T entity, boolean update) {
+    // Validate complete extension field only on POST
+    if (entity.getExtension() == null || update) {
+      return;
+    }
+
     JsonNode jsonNode = JsonUtils.valueToTree(entity.getExtension());
     Iterator<Entry<String, JsonNode>> customFields = jsonNode.fields();
     while (customFields.hasNext()) {
@@ -3235,7 +3262,7 @@ public abstract class EntityRepository<T extends EntityInterface> {
         updateDescription();
         updateDisplayName();
         updateOwners();
-        updateExtension();
+        updateExtension(consolidatingChanges);
         updateTags(
             updated.getFullyQualifiedName(), FIELD_TAGS, original.getTags(), updated.getTags());
         updateDomain();
@@ -3260,7 +3287,7 @@ public abstract class EntityRepository<T extends EntityInterface> {
         updateDescription();
         updateDisplayName();
         updateOwnersForImport();
-        updateExtension();
+        updateExtension(consolidatingChanges);
         updateTagsForImport(
             updated.getFullyQualifiedName(), FIELD_TAGS, original.getTags(), updated.getTags());
         updateDomainForImport();
@@ -3413,7 +3440,7 @@ public abstract class EntityRepository<T extends EntityInterface> {
       applyTags(updatedTags, fqn);
     }
 
-    private void updateExtension() {
+    private void updateExtension(boolean consolidatingChanges) {
       Object origExtension = original.getExtension();
       Object updatedExtension = updated.getExtension();
       if (origExtension == updatedExtension) {
@@ -3433,39 +3460,40 @@ public abstract class EntityRepository<T extends EntityInterface> {
         return;
       }
 
-      List<JsonNode> added = new ArrayList<>();
-      List<JsonNode> deleted = new ArrayList<>();
-      JsonNode origFields = JsonUtils.valueToTree(origExtension);
-      JsonNode updatedFields = JsonUtils.valueToTree(updatedExtension);
+      List<JsonNode> addedFields = new ArrayList<>();
+      List<JsonNode> deletedFields = new ArrayList<>();
+      List<JsonNode> updatedFields = new ArrayList<>();
+      JsonNode origExtensionFields = JsonUtils.valueToTree(origExtension);
+      JsonNode updatedExtensionFields = JsonUtils.valueToTree(updatedExtension);
+      Set<String> allKeys = new HashSet<>();
+      origExtensionFields.fieldNames().forEachRemaining(allKeys::add);
+      updatedExtensionFields.fieldNames().forEachRemaining(allKeys::add);
 
-      // Check for updated and deleted fields
-      for (Iterator<Entry<String, JsonNode>> it = origFields.fields(); it.hasNext(); ) {
-        Entry<String, JsonNode> orig = it.next();
-        JsonNode updatedField = updatedFields.get(orig.getKey());
-        if (updatedField == null) {
-          deleted.add(JsonUtils.getObjectNode(orig.getKey(), orig.getValue()));
-        } else {
-          // TODO converting to a string is a hack for now because JsonNode equals issues
-          recordChange(
-              getExtensionField(orig.getKey()),
-              orig.getValue().toString(),
-              updatedField.toString());
+      for (String key : allKeys) {
+        JsonNode origValue = origExtensionFields.get(key);
+        JsonNode updatedValue = updatedExtensionFields.get(key);
+
+        if (origValue == null) {
+          addedFields.add(JsonUtils.getObjectNode(key, updatedValue));
+        } else if (updatedValue == null) {
+          deletedFields.add(JsonUtils.getObjectNode(key, origValue));
+        } else if (!origValue.equals(updatedValue)) {
+          updatedFields.add(JsonUtils.getObjectNode(key, updatedValue));
+          recordChange(getExtensionField(key), origValue.toString(), updatedValue.toString());
         }
       }
 
-      // Check for added fields
-      for (Iterator<Entry<String, JsonNode>> it = updatedFields.fields(); it.hasNext(); ) {
-        Entry<String, JsonNode> updatedField = it.next();
-        JsonNode orig = origFields.get(updatedField.getKey());
-        if (orig == null) {
-          added.add(JsonUtils.getObjectNode(updatedField.getKey(), updatedField.getValue()));
+      if (!consolidatingChanges) {
+        for (JsonNode node :
+            Stream.of(addedFields, updatedFields, deletedFields).flatMap(List::stream).toList()) {
+          node.fields().forEachRemaining(field -> validateExtension(updated, field));
         }
       }
-      if (!added.isEmpty()) {
-        fieldAdded(changeDescription, FIELD_EXTENSION, JsonUtils.pojoToJson(added));
+      if (!addedFields.isEmpty()) {
+        fieldAdded(changeDescription, FIELD_EXTENSION, JsonUtils.pojoToJson(addedFields));
       }
-      if (!deleted.isEmpty()) {
-        fieldDeleted(changeDescription, FIELD_EXTENSION, JsonUtils.pojoToJson(deleted));
+      if (!deletedFields.isEmpty()) {
+        fieldDeleted(changeDescription, FIELD_EXTENSION, JsonUtils.pojoToJson(deletedFields));
       }
       removeExtension(original);
       storeExtension(updated);
diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryTermResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryTermResourceTest.java
index 7b9886d87f8..10b56a3ba0f 100644
--- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryTermResourceTest.java
+++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryTermResourceTest.java
@@ -43,6 +43,10 @@ import static org.openmetadata.service.util.EntityUtil.toTagLabels;
 import static org.openmetadata.service.util.TestUtils.*;
 import static org.openmetadata.service.util.TestUtils.UpdateType.MINOR_UPDATE;
 
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
 import jakarta.ws.rs.client.WebTarget;
 import jakarta.ws.rs.core.Response;
 import java.io.IOException;
@@ -78,6 +82,7 @@ import org.openmetadata.schema.api.data.CreateGlossaryTerm;
 import org.openmetadata.schema.api.data.CreateTable;
 import org.openmetadata.schema.api.data.TermReference;
 import org.openmetadata.schema.api.feed.ResolveTask;
+import org.openmetadata.schema.entity.Type;
 import org.openmetadata.schema.entity.classification.Classification;
 import org.openmetadata.schema.entity.classification.Tag;
 import org.openmetadata.schema.entity.data.EntityHierarchy;
@@ -86,9 +91,11 @@ import org.openmetadata.schema.entity.data.GlossaryTerm;
 import org.openmetadata.schema.entity.data.GlossaryTerm.Status;
 import org.openmetadata.schema.entity.data.Table;
 import org.openmetadata.schema.entity.feed.Thread;
+import org.openmetadata.schema.entity.type.CustomProperty;
 import org.openmetadata.schema.entity.type.Style;
 import org.openmetadata.schema.type.ChangeDescription;
 import org.openmetadata.schema.type.Column;
+import org.openmetadata.schema.type.CustomPropertyConfig;
 import org.openmetadata.schema.type.EntityReference;
 import org.openmetadata.schema.type.Include;
 import org.openmetadata.schema.type.TagLabel;
@@ -98,11 +105,13 @@ import org.openmetadata.schema.type.api.BulkOperationResult;
 import org.openmetadata.schema.type.api.BulkResponse;
 import org.openmetadata.service.Entity;
 import org.openmetadata.service.governance.workflows.WorkflowHandler;
+import org.openmetadata.service.jdbi3.GlossaryTermRepository;
 import org.openmetadata.service.resources.EntityResourceTest;
 import org.openmetadata.service.resources.databases.TableResourceTest;
 import org.openmetadata.service.resources.feeds.FeedResource.ThreadList;
 import org.openmetadata.service.resources.feeds.FeedResourceTest;
 import org.openmetadata.service.resources.feeds.MessageParser.EntityLink;
+import org.openmetadata.service.resources.metadata.TypeResourceTest;
 import org.openmetadata.service.resources.tags.ClassificationResourceTest;
 import org.openmetadata.service.resources.tags.TagResourceTest;
 import org.openmetadata.service.util.EntityUtil;
@@ -1597,6 +1606,149 @@ public class GlossaryTermResourceTest extends EntityResourceTest<GlossaryTerm, C
     deleteEntity(term.getId(), ADMIN_AUTH_HEADERS);
   }
 
+  @Test
+  void test_validateCustomPropertyUpdate() throws IOException {
+    // This test verifies handling of corrupt custom property values by testing:
+    // 1. You can still update other properties even when a property contains corrupt enum Key
+    // 2. The system prevents updates to corrupt property (does not block other entity updates) if
+    // invalid values are still present.
+    // 3. You can successfully update a corrupted property by fixing or removing its invalid value
+
+    Glossary glossary = createGlossary("TestCustomPropsGlossary", null, null);
+
+    // Define custom property definitions
+    TypeResourceTest typeResourceTest = new TypeResourceTest();
+    Type entityType =
+        typeResourceTest.getEntityByName(
+            Entity.GLOSSARY_TERM, "customProperties", ADMIN_AUTH_HEADERS);
+
+    CustomProperty enumCp =
+        new CustomProperty()
+            .withName("certified")
+            .withDescription("Certification status")
+            .withPropertyType(ENUM_TYPE.getEntityReference())
+            .withCustomPropertyConfig(
+                new CustomPropertyConfig()
+                    .withConfig(
+                        Map.of(
+                            "values",
+                            List.of("draft", "official", "verified"),
+                            "multiSelect",
+                            true)));
+
+    entityType =
+        typeResourceTest.addAndCheckCustomProperty(
+            entityType.getId(), enumCp, OK, ADMIN_AUTH_HEADERS);
+
+    CustomProperty sensitivityCp =
+        new CustomProperty()
+            .withName("sensitivity")
+            .withDescription("Data sensitivity")
+            .withPropertyType(ENUM_TYPE.getEntityReference())
+            .withCustomPropertyConfig(
+                new CustomPropertyConfig()
+                    .withConfig(
+                        Map.of(
+                            "values",
+                            List.of("confidential", "internal", "public", "restricted"),
+                            "multiSelect",
+                            false)));
+
+    entityType =
+        typeResourceTest.addAndCheckCustomProperty(
+            entityType.getId(), sensitivityCp, OK, ADMIN_AUTH_HEADERS);
+
+    // Create a glossary term
+    CreateGlossaryTerm createTerm =
+        new CreateGlossaryTerm()
+            .withName("TestCustomPropsTerm")
+            .withGlossary(glossary.getFullyQualifiedName())
+            .withDescription("TestCustomPropsTerm");
+    GlossaryTerm term = createAndCheckEntity(createTerm, ADMIN_AUTH_HEADERS);
+
+    // Simulate a term with an invalid value for one property (as if created in version <1.5x)
+    // by directly setting the extension with an invalid value inside database
+    // Get the repository to directly access the database
+    GlossaryTermRepository termRepository =
+        (GlossaryTermRepository) Entity.getEntityRepository(GLOSSARY_TERM);
+
+    // Create corrupt data with an invalid enum value and store inside database bypassing validation
+    JsonNode corruptExtension =
+        JsonUtils.valueToTree(Map.of("certified", List.of("wrongValue", "official")));
+    term.setExtension(corruptExtension);
+    termRepository.storeExtension(term);
+    // Verify the corrupt extension was stored
+    GlossaryTerm termWithCorruptData = getEntity(term.getId(), "extension", ADMIN_AUTH_HEADERS);
+
+    JsonNode updatedExtension = JsonUtils.valueToTree(termWithCorruptData.getExtension());
+    assertEquals("wrongValue", updatedExtension.get("certified").get(1).asText());
+
+    // SCENARIO 1: Try to update a different property with a valid value via the API
+    // The API should allow updating other properties even with corrupt data present
+    String json = JsonUtils.pojoToJson(termWithCorruptData);
+
+    // Get the existing extension and add the new field to it
+    Map<String, Object> existingExtension =
+        (Map<String, Object>) termWithCorruptData.getExtension();
+    Map<String, Object> extensionUpdate = new HashMap<>(existingExtension);
+    extensionUpdate.put("sensitivity", List.of("internal"));
+    termWithCorruptData.setExtension(extensionUpdate);
+
+    // Patch the entity with our updated extension that preserves the corrupt field
+    GlossaryTerm updatedTerm =
+        patchEntity(term.getId(), json, termWithCorruptData, ADMIN_AUTH_HEADERS);
+
+    // Verify both the corrupt data and new valid property are present after the update
+    JsonNode resultExtension = JsonUtils.valueToTree(updatedTerm.getExtension());
+    assertEquals("wrongValue", resultExtension.get("certified").get(1).asText());
+    assertEquals("internal", resultExtension.get("sensitivity").get(0).asText());
+
+    // SCENARIO 2: Try to modify existing corrupt property by adding new valid value and not
+    // removing the wrong one
+    // This should fail with a 400 error
+    String jsonForWrongUpdate = JsonUtils.pojoToJson(updatedTerm);
+    Map<String, Object> invalidExtension = new HashMap<>(existingExtension);
+    invalidExtension.put("certified", List.of("wrongValue", "official"));
+    invalidExtension.put("sensitivity", List.of("internal"));
+    updatedTerm.setExtension(invalidExtension);
+
+    // Expect a 400 error when trying to update the corrupt property with another wrong value
+    HttpResponseException exception =
+        assertThrows(
+            HttpResponseException.class,
+            () -> patchEntity(term.getId(), jsonForWrongUpdate, updatedTerm, ADMIN_AUTH_HEADERS));
+    assertEquals(400, exception.getStatusCode());
+    assertTrue(
+        exception.getMessage().contains("Values '[wrongValue"),
+        "Error should mention the invalid values");
+
+    // SCENARIO 3: Remove wrong values in corrupt property and add only valid values
+    // This should succeed
+    String jsonForValidUpdate = JsonUtils.pojoToJson(updatedTerm);
+    Map<String, Object> validExtension = new HashMap<>();
+    validExtension.put("certified", List.of("draft", "official"));
+    validExtension.put("sensitivity", List.of("internal"));
+    updatedTerm.setExtension(validExtension);
+
+    ObjectMapper mapper = new ObjectMapper();
+    ArrayNode patchArray = mapper.createArrayNode();
+    ObjectNode patchOp = mapper.createObjectNode();
+    patchOp.put("op", "replace");
+    patchOp.put("path", "/extension");
+    patchOp.set("value", mapper.valueToTree(validExtension));
+    patchArray.add(patchOp);
+
+    // This should succeed since we're replacing the corrupt property with valid values
+    GlossaryTerm properlyUpdatedTerm = patchEntity(term.getId(), patchArray, ADMIN_AUTH_HEADERS);
+
+    // Verify the corrupt data is replaced with valid values
+    JsonNode finalExtension = JsonUtils.valueToTree(properlyUpdatedTerm.getExtension());
+    JsonNode certifiedValues = finalExtension.get("certified");
+    assertNotNull(certifiedValues, "Certified values should exist");
+    assertTrue(certifiedValues.isArray(), "Certified should be an array");
+    assertEquals(2, certifiedValues.size(), "Should contain 2 values");
+  }
+
   public Glossary createGlossary(
       TestInfo test, List<EntityReference> reviewers, List<EntityReference> owners)
       throws IOException {