From c305d1bd6092d85828fdb27a7814f40b4ef6fa46 Mon Sep 17 00:00:00 2001
From: Carsten Wickner <Carsten.Wickner@gmx.de>
Date: Wed, 10 Jan 2024 00:03:51 +0100
Subject: [PATCH] feat: Introduce Option for duplicate member attribute removal

---
 CHANGELOG.md                                  |  7 +++-
 .../victools/jsonschema/generator/Option.java |  7 ++++
 .../jsonschema/generator/OptionPreset.java    |  9 +++--
 .../jsonschema/generator/SchemaBuilder.java   |  4 +-
 .../generator/SchemaGeneratorConfig.java      | 10 +++++
 .../generator/impl/SchemaCleanUpUtils.java    | 16 +++++++-
 .../impl/SchemaGeneratorConfigImpl.java       |  5 +++
 .../jsonschema/generator/impl/Util.java       | 18 +++++++++
 .../SchemaGeneratorMemberCleanUpTest.java     | 37 +++++++++++++++----
 9 files changed, 100 insertions(+), 13 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d7fd1732..90376078 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,7 +5,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
-*-*
+### `jsonschema-generator`
+#### Added
+- new `Option.DUPLICATE_MEMBER_ATTRIBUTE_CLEANUP_AT_THE_END` discard duplicate elements from member sub-schemas
+
+#### Changed
+- new `Option.DUPLICATE_MEMBER_ATTRIBUTE_CLEANUP_AT_THE_END` by default included in standard `OptionPreset`s
 
 ## [4.33.1] - 2023-12-19
 ### `jsonschema-module-jackson`
diff --git a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/Option.java b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/Option.java
index d154891e..6ccbf912 100644
--- a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/Option.java
+++ b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/Option.java
@@ -322,6 +322,13 @@ public enum Option {
      * @since 4.6.0
      */
     ALLOF_CLEANUP_AT_THE_END(null, null),
+    /**
+     * Whether at the end of the schema generation, all member sub-schemas referencing a common definition should be checked for any duplicated
+     * attributes, which should be removed from the inline member sub-schemas in favor of the equivalent in the single common definition.
+     *
+     * @since 4.34.0
+     */
+    DUPLICATE_MEMBER_ATTRIBUTE_CLEANUP_AT_THE_END(null, null),
     /**
      * Whether at the end of the schema generation, all sub-schemas without an explicit "type" indication should be augmented by the implied "type"
      * based on the other tags in the respective schema.
diff --git a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/OptionPreset.java b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/OptionPreset.java
index 7d8634ca..ad60275c 100644
--- a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/OptionPreset.java
+++ b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/OptionPreset.java
@@ -46,7 +46,8 @@ public class OptionPreset {
             Option.DEFINITIONS_FOR_ALL_OBJECTS,
             Option.NULLABLE_FIELDS_BY_DEFAULT,
             Option.NULLABLE_METHOD_RETURN_VALUES_BY_DEFAULT,
-            Option.ALLOF_CLEANUP_AT_THE_END
+            Option.ALLOF_CLEANUP_AT_THE_END,
+            Option.DUPLICATE_MEMBER_ATTRIBUTE_CLEANUP_AT_THE_END
     );
 
     /**
@@ -63,7 +64,8 @@ public class OptionPreset {
             Option.PUBLIC_NONSTATIC_FIELDS,
             Option.NONPUBLIC_NONSTATIC_FIELDS_WITH_GETTERS,
             Option.NONPUBLIC_NONSTATIC_FIELDS_WITHOUT_GETTERS,
-            Option.ALLOF_CLEANUP_AT_THE_END
+            Option.ALLOF_CLEANUP_AT_THE_END,
+            Option.DUPLICATE_MEMBER_ATTRIBUTE_CLEANUP_AT_THE_END
     );
 
     /**
@@ -79,7 +81,8 @@ public class OptionPreset {
             Option.NONSTATIC_NONVOID_NONGETTER_METHODS,
             Option.SIMPLIFIED_ENUMS,
             Option.SIMPLIFIED_OPTIONALS,
-            Option.ALLOF_CLEANUP_AT_THE_END
+            Option.ALLOF_CLEANUP_AT_THE_END,
+            Option.DUPLICATE_MEMBER_ATTRIBUTE_CLEANUP_AT_THE_END
     );
 
     private final Set<Option> defaultEnabledOptions;
diff --git a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/SchemaBuilder.java b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/SchemaBuilder.java
index 4189ef6a..2af19938 100644
--- a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/SchemaBuilder.java
+++ b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/SchemaBuilder.java
@@ -195,7 +195,9 @@ private void performCleanup(ObjectNode definitionsNode, String referenceKeyPrefi
             cleanUpUtils.reduceAllOfNodes(this.schemaNodes);
         }
         cleanUpUtils.reduceAnyOfNodes(this.schemaNodes);
-        cleanUpUtils.reduceRedundantMemberAttributes(this.schemaNodes, definitionsNode, referenceKeyPrefix);
+        if (this.config.shouldDiscardDuplicateMemberAttributes()) {
+            cleanUpUtils.reduceRedundantMemberAttributes(this.schemaNodes, definitionsNode, referenceKeyPrefix);
+        }
         if (this.config.shouldIncludeStrictTypeInfo()) {
             cleanUpUtils.setStrictTypeInfo(this.schemaNodes, true);
         }
diff --git a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/SchemaGeneratorConfig.java b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/SchemaGeneratorConfig.java
index 842a5c24..de98d869 100644
--- a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/SchemaGeneratorConfig.java
+++ b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/SchemaGeneratorConfig.java
@@ -130,6 +130,16 @@ public interface SchemaGeneratorConfig extends StatefulConfig {
      */
     boolean shouldCleanupUnnecessaryAllOfElements();
 
+    /**
+     * Determine whether duplicate elements should be removed from member/property sub-schemas if equal elements are contained in the referenced
+     * common definition.
+     *
+     * @return whether to discard duplicate elements from property schemas during the last schema generation step
+     *
+     * @since 4.34.0
+     */
+    boolean shouldDiscardDuplicateMemberAttributes();
+
     /**
      * Determine whether sub schemas should get the {@link SchemaKeyword#TAG_TYPE} added implicitly based on other contained tags, if it is missing.
      *
diff --git a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/SchemaCleanUpUtils.java b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/SchemaCleanUpUtils.java
index 18b41d7a..26fbac75 100644
--- a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/SchemaCleanUpUtils.java
+++ b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/SchemaCleanUpUtils.java
@@ -25,6 +25,7 @@
 import com.github.victools.jsonschema.generator.SchemaVersion;
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
@@ -607,9 +608,22 @@ private void reduceRedundantMemberAttributesIfPossible(ObjectNode schemaNode,
      * @param referencedDefinition attribute names and values in the common definition, which don't need to be repeated
      */
     private void reduceRedundantAttributesIfPossible(ObjectNode memberSchema, Map<String, JsonNode> referencedDefinition) {
+        Set<String> skippedKeywords = new HashSet<>();
+        String ifKeyword = this.config.getKeyword(SchemaKeyword.TAG_IF);
+        String thenKeyword = this.config.getKeyword(SchemaKeyword.TAG_THEN);
+        String elseKeyword = this.config.getKeyword(SchemaKeyword.TAG_ELSE);
+        boolean shouldSkipConditionals = !Util.nullSafeEquals(memberSchema.get(ifKeyword), referencedDefinition.get(ifKeyword))
+                || !Util.nullSafeEquals(memberSchema.get(thenKeyword), referencedDefinition.get(thenKeyword))
+                || !Util.nullSafeEquals(memberSchema.get(elseKeyword), referencedDefinition.get(elseKeyword));
+        if (shouldSkipConditionals) {
+            skippedKeywords.add(ifKeyword);
+            skippedKeywords.add(thenKeyword);
+            skippedKeywords.add(elseKeyword);
+        }
         for (Iterator<Map.Entry<String, JsonNode>> it = memberSchema.fields(); it.hasNext(); ) {
             Map.Entry<String, JsonNode> memberAttribute = it.next();
-            if (memberAttribute.getValue().equals(referencedDefinition.get(memberAttribute.getKey()))) {
+            String keyword = memberAttribute.getKey();
+            if (!skippedKeywords.contains(keyword) && memberAttribute.getValue().equals(referencedDefinition.get(keyword))) {
                 // remove member attribute, that also exists on the referenced definition
                 it.remove();
             }
diff --git a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/SchemaGeneratorConfigImpl.java b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/SchemaGeneratorConfigImpl.java
index 5a835f32..a05d94af 100644
--- a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/SchemaGeneratorConfigImpl.java
+++ b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/SchemaGeneratorConfigImpl.java
@@ -158,6 +158,11 @@ public boolean shouldCleanupUnnecessaryAllOfElements() {
         return this.isOptionEnabled(Option.ALLOF_CLEANUP_AT_THE_END);
     }
 
+    @Override
+    public boolean shouldDiscardDuplicateMemberAttributes() {
+        return this.isOptionEnabled(Option.DUPLICATE_MEMBER_ATTRIBUTE_CLEANUP_AT_THE_END);
+    }
+
     @Override
     public boolean shouldIncludeStrictTypeInfo() {
         return this.isOptionEnabled(Option.STRICT_TYPE_INFO);
diff --git a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/Util.java b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/Util.java
index 50032d2f..2dd472ff 100644
--- a/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/Util.java
+++ b/jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/impl/Util.java
@@ -92,4 +92,22 @@ public static <T> List<T> nullSafe(List<T> list) {
         }
         return list;
     }
+
+    /**
+     * Ensure the two given values are either both {@code null} or equal to each other.
+     *
+     * @param one first value to check
+     * @param other second value to check
+     * @return whether the two given values are equal
+     */
+    public static boolean nullSafeEquals(Object one, Object other) {
+        if (one == null) {
+            return other == null;
+        }
+        if (other == null) {
+            return false;
+        }
+        return one.hashCode() == other.hashCode()
+               && one.equals(other);
+    }
 }
diff --git a/jsonschema-generator/src/test/java/com/github/victools/jsonschema/generator/SchemaGeneratorMemberCleanUpTest.java b/jsonschema-generator/src/test/java/com/github/victools/jsonschema/generator/SchemaGeneratorMemberCleanUpTest.java
index ca6d1847..5bd3df41 100644
--- a/jsonschema-generator/src/test/java/com/github/victools/jsonschema/generator/SchemaGeneratorMemberCleanUpTest.java
+++ b/jsonschema-generator/src/test/java/com/github/victools/jsonschema/generator/SchemaGeneratorMemberCleanUpTest.java
@@ -17,23 +17,46 @@
 package com.github.victools.jsonschema.generator;
 
 import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.node.ObjectNode;
+import java.util.Arrays;
+import java.util.List;
 import java.util.Map;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 
 /**
  * Test for {@link SchemaGenerator} class.
  */
 public class SchemaGeneratorMemberCleanUpTest {
 
-    @Test
-    public void testMemberCleanUp() {
-        SchemaGeneratorConfig generatorConfig = new SchemaGeneratorConfigBuilder(SchemaVersion.DRAFT_2019_09, OptionPreset.PLAIN_JSON)
-                .with(Option.MAP_VALUES_AS_ADDITIONAL_PROPERTIES)
-                .with(Option.DEFINITIONS_FOR_ALL_OBJECTS)
-                .build();
+    private static Stream<Arguments> testMemberCleanup() {
+        return Stream.of(
+                Arguments.of(true, Arrays.asList("$ref")),
+                Arguments.of(false, Arrays.asList("$ref", "additionalProperties"))
+        );
+    }
+
+    @ParameterizedTest
+    @MethodSource
+    public void testMemberCleanup(boolean enableCleanup, List<String> expectedMemberAttributes) {
+        SchemaGeneratorConfigBuilder configBuilder = new SchemaGeneratorConfigBuilder(SchemaVersion.DRAFT_2019_09, OptionPreset.PLAIN_JSON)
+                .with(Option.MAP_VALUES_AS_ADDITIONAL_PROPERTIES, Option.DEFINITIONS_FOR_ALL_OBJECTS);
+        if (enableCleanup) {
+            configBuilder.with(Option.DUPLICATE_MEMBER_ATTRIBUTE_CLEANUP_AT_THE_END);
+        } else {
+            configBuilder.without(Option.DUPLICATE_MEMBER_ATTRIBUTE_CLEANUP_AT_THE_END);
+        }
+        SchemaGeneratorConfig generatorConfig = configBuilder.build();
         ObjectNode schema = new SchemaGenerator(generatorConfig).generateSchema(TestClass.class);
-        System.out.println(schema.toPrettyString());
+        JsonNode memberSchema = schema.get(generatorConfig.getKeyword(SchemaKeyword.TAG_PROPERTIES)).get("mapValue");
+        Assertions.assertEquals(expectedMemberAttributes.size(), memberSchema.size());
+        memberSchema.fieldNames()
+                .forEachRemaining(fieldName -> Assertions.assertTrue(expectedMemberAttributes.contains(fieldName)));
     }
 
     private static class TestClass {