691 better way to deal with sub schema duplications in schema extraction (#978)

* make schema extraction avoid duplicate subschemas

* fix type errors

Author: Logende

meta_configurator/src/components/panels/gui-editor/properties/DateProperty.vue

@@ -4,7 +4,7 @@ import {ref, watch} from 'vue';
 import DatePicker from 'primevue/datepicker';
 import type {JsonSchemaWrapper} from '@/schema/jsonSchemaWrapper';
 import type {PathElement} from '@/utility/path';
-import type {ValidationResult} from '@/schema/validationService';
+import type {ValidationResult} from '@/schema/validationUtils';
 import {isReadOnly} from '@/components/panels/gui-editor/configTreeNodeReadingUtils';
 
 const props = defineProps<{
@@ -30,12 +30,13 @@ watch(
   }
 );
 
-function updateValue(newDate: Date | undefined) {
+function updateValue(value: Date | (Date | null)[] | Date[] | null | undefined) {
+  // DatePicker is used in single-date mode here, so we only handle a single Date (or empty).
+  const newDate = value instanceof Date ? value : undefined;
   if (!newDate) {
     emit('update:propertyData', undefined);
     return;
   }
-  // convert Date back to ISO date string (YYYY-MM-DD)
   const isoString = newDate.toISOString().split('T')[0];
   emit('update:propertyData', isoString);
 }

meta_configurator/src/data/managedSession.ts

@@ -6,7 +6,7 @@ import {getDataForMode, getSchemaForMode} from '@/data/useDataLink';
 import {SessionMode} from '@/store/sessionMode';
 import type {SearchResult} from '@/utility/search';
 import {EffectiveSchema} from '@/schema/effectiveSchemaCalculator';
-import type {ConfigDataTreeNode} from '@/components/panels/gui-editor/configDataTreeNode';
+import type {GuiEditorTreeNode} from '@/components/panels/gui-editor/configDataTreeNode';
 
 export class ManagedSession {
   constructor(public mode: SessionMode) {}
@@ -81,7 +81,7 @@ export class ManagedSession {
   /**
    * Returns true if the node is selected, or if the node or any of its children matches a search result.
    */
-  public isNodeHighlighted(node: ConfigDataTreeNode) {
+  public isNodeHighlighted(node: GuiEditorTreeNode) {
     const selectedPath = pathToString(this.currentSelectedElement.value);
     if (node.key && node.key === selectedPath) {
       return true;

meta_configurator/src/schema/__tests__/schemaManipulationUtils.test.ts

@@ -3,6 +3,7 @@ import {shallowRef} from 'vue';
 import {ManagedData} from '@/data/managedData';
 import {SessionMode} from '@/store/sessionMode';
 import {
+  doesIdenticalSchemaDefinitionExist,
   extractAllInlinedSchemaElements,
   extractInlinedSchemaElement,
 } from '@/schema/schemaManipulationUtils';
@@ -162,6 +163,129 @@ describe('schemaManipulationUtils', () => {
     });
   });
 
+  it('reuses an existing matching definition under a different name when duplicate reuse is enabled', () => {
+    // existing def is named "vector" but has the same content as the inlined a2 sub-schema
+    schemaData.setDataAt(['$defs', 'vector'], {
+      type: 'object',
+      properties: {
+        a2a: {
+          type: 'string',
+        },
+      },
+    });
+
+    const extractedPath = extractInlinedSchemaElement(
+      ['properties', 'a', 'properties', 'a2'],
+      schemaData,
+      // candidate name is different from the existing one; the dedupe should still find vector by content
+      'a2',
+      true
+    );
+
+    expect(extractedPath).toEqual(['$defs', 'vector']);
+    expect(schemaData.dataAt(['$defs', 'a2'])).toBeUndefined();
+    expect(schemaData.dataAt(['properties', 'a', 'properties', 'a2'])).toEqual({
+      $ref: '#/$defs/vector',
+    });
+    expect(schemaData.dataAt(['properties', 'a', 'properties', 'a3'])).toEqual({
+      type: 'object',
+      $ref: '#/$defs/vector',
+    });
+  });
+
+  it('collapses identical sub-schemas into a single $defs entry during bulk extraction', () => {
+    schemaData = new ManagedData(
+      shallowRef({
+        type: 'object',
+        properties: {
+          point1: {
+            type: 'object',
+            properties: {
+              x: {type: 'number'},
+              y: {type: 'number'},
+            },
+          },
+          point2: {
+            type: 'object',
+            properties: {
+              x: {type: 'number'},
+              y: {type: 'number'},
+            },
+          },
+          point3: {
+            // key order differs from point1/point2; deep equality should still consider it equal
+            type: 'object',
+            properties: {
+              y: {type: 'number'},
+              x: {type: 'number'},
+            },
+          },
+        },
+      }),
+      SessionMode.SchemaEditor
+    );
+
+    const extractedCount = extractAllInlinedSchemaElements(schemaData, false, false);
+
+    // root is excluded by extractRootElement=false. The 3 inlined point objects should all
+    // collapse into a single $defs entry — so only one definition is actually created.
+    expect(Object.keys(schemaData.data.value.$defs)).toHaveLength(1);
+    const defName = Object.keys(schemaData.data.value.$defs)[0]!;
+    const expectedRef = {$ref: `#/$defs/${defName}`};
+    expect(schemaData.data.value.properties.point1).toEqual(expectedRef);
+    expect(schemaData.data.value.properties.point2).toEqual(expectedRef);
+    expect(schemaData.data.value.properties.point3).toEqual(expectedRef);
+    // each of the three properties was processed
+    expect(extractedCount).toBe(3);
+  });
+
+  describe('doesIdenticalSchemaDefinitionExist', () => {
+    it('returns the path of an existing definition with deeply equal content', () => {
+      schemaData.setDataAt(['$defs', 'foo'], {
+        type: 'object',
+        properties: {x: {type: 'number'}},
+      });
+
+      const match = doesIdenticalSchemaDefinitionExist(schemaData, {
+        // keys in different order, but structurally identical
+        properties: {x: {type: 'number'}},
+        type: 'object',
+      });
+
+      expect(match).toEqual(['$defs', 'foo']);
+    });
+
+    it('returns undefined when no equivalent definition exists', () => {
+      schemaData.setDataAt(['$defs', 'foo'], {type: 'string'});
+
+      const match = doesIdenticalSchemaDefinitionExist(schemaData, {type: 'number'});
+
+      expect(match).toBeUndefined();
+    });
+
+    it('returns undefined when there is no $defs section', () => {
+      const match = doesIdenticalSchemaDefinitionExist(schemaData, {type: 'string'});
+      expect(match).toBeUndefined();
+    });
+
+    it('also searches the legacy "definitions" section', () => {
+      schemaData.setDataAt(['definitions', 'legacyFoo'], {type: 'object'});
+
+      const match = doesIdenticalSchemaDefinitionExist(schemaData, {type: 'object'});
+
+      expect(match).toEqual(['definitions', 'legacyFoo']);
+    });
+
+    it('prefers a $defs match over a definitions match when both exist', () => {
+      schemaData.setDataAt(['definitions', 'legacyFoo'], {type: 'string'});
+      schemaData.setDataAt(['$defs', 'modernFoo'], {type: 'string'});
+
+      const match = doesIdenticalSchemaDefinitionExist(schemaData, {type: 'string'});
+
+      expect(match).toEqual(['$defs', 'modernFoo']);
+    });
+  });
+
   it('extracts inlined subschemas that are nested inside an existing $defs entry', () => {
     schemaData = new ManagedData(
       shallowRef({

meta_configurator/src/schema/schemaManipulationUtils.ts

@@ -7,6 +7,7 @@ import {constructSchemaGraph} from '@/schema/graph-representation/schemaGraphCon
 import type {SchemaNodeData} from '@/schema/graph-representation/schemaGraphTypes';
 import {updateReferences} from '@/utility/renameUtils';
 import {stringToIdentifier} from '@/utility/stringToIdentifier';
+import _ from 'lodash';
 
 export function extractAllInlinedSchemaElements(
   schemaData: ManagedData,
@@ -33,7 +34,7 @@ export function extractAllInlinedSchemaElements(
       node.title,
       node.fallbackDisplayName
     );
-    extractInlinedSchemaElement(node.absolutePath, schemaData, newIdentifier);
+    extractInlinedSchemaElement(node.absolutePath, schemaData, newIdentifier, true);
     nodesExtracted++;
   });
 
@@ -57,23 +58,17 @@ export function extractInlinedSchemaElement(
   };
 
   if (forgetIfDuplicateExists) {
-    // if an existing definition exists with the same content, we can just reference that
-    const existingElementDefPath = ['$defs', elementName];
-    const existingElementDef = dataAt(existingElementDefPath, schemaData.data.value);
-    if (existingElementDef) {
-      if (JSON.stringify(existingElementDef) === JSON.stringify(dataAtPath)) {
-        const referenceToNewElement = '#' + pathToJsonPointer(existingElementDefPath);
-        schemaData.setDataAt(absoluteElementPath, {
-          $ref: referenceToNewElement,
-        });
-        updateReferences(
-          absoluteElementPath,
-          existingElementDefPath,
-          schemaData.data.value,
-          updateDataFct
-        );
-        return existingElementDefPath;
-      }
+    // if an equivalent definition already exists anywhere in $defs (not just at the
+    // candidate name), reference that one instead of creating a duplicate. This collapses
+    // identical sub-schemas
+    const existingDefPath = doesIdenticalSchemaDefinitionExist(schemaData, dataAtPath);
+    if (existingDefPath) {
+      const referenceToExistingElement = '#' + pathToJsonPointer(existingDefPath);
+      schemaData.setDataAt(absoluteElementPath, {
+        $ref: referenceToExistingElement,
+      });
+      updateReferences(absoluteElementPath, existingDefPath, schemaData.data.value, updateDataFct);
+      return existingDefPath;
     }
   }
 
@@ -87,6 +82,32 @@ export function extractInlinedSchemaElement(
   return newElementId;
 }
 
+/**
+ * Looks through every entry in the schema's top-level $defs and (legacy) definitions
+ * sections and returns the path of the first one whose content is deeply equal to
+ * `schemaToCheck`. Returns undefined if no such definition exists.
+ *
+ * Used to avoid creating duplicate definition entries when extracting sub-schemas: if an
+ * equivalent definition is already there, the caller can reference it instead.
+ */
+export function doesIdenticalSchemaDefinitionExist(
+  schemaData: ManagedData,
+  schemaToCheck: any
+): Path | undefined {
+  for (const defsKey of ['$defs', 'definitions']) {
+    const defs = dataAt([defsKey], schemaData.data.value);
+    if (!defs || typeof defs !== 'object') {
+      continue;
+    }
+    for (const name of Object.keys(defs)) {
+      if (_.isEqual(defs[name], schemaToCheck)) {
+        return [defsKey, name];
+      }
+    }
+  }
+  return undefined;
+}
+
 export function createIdentifierForExtractedElement(
   name: string | undefined,
   title: string | undefined,