fix: missing enum breaking diff in oneOf combiner in merged document step 2

Author: makeev-pavel

src/core/compare.ts

@@ -34,7 +34,7 @@ import {
 } from '../types'
 import { getObjectValue, isArray, isDiffAdd, isDiffRemove, isDiffReplace, isNumber, isObject, typeOf } from '../utils'
 import { ANY_COMBINER_PATH, DiffAction, JSO_ROOT } from './constants'
-import { addDiffObjectToContainer, diffFactory, NEVER_KEY } from './diff'
+import { addDiffObjectToContainer, createDiffEntry, diffFactory, NEVER_KEY } from './diff'
 import { arrayMappingResolver, objectMappingResolver } from './mapping'
 
 const extractDeclarationPaths = (jso: Record<PropertyKey, unknown>, originMetaKey: symbol, property: PropertyKey): JsonPath[] => {
@@ -156,21 +156,24 @@ const cleanUpRecursive = (ctx: NodeContext): NodeContext => {
 }
 
 export const getOrCreateChildDiffAdd = (diffUniquenessCache: EvaluationCacheService, childCtx: CompareContext) => {
-  return diffUniquenessCache.cacheEvaluationResultByFootprint<[unknown, string, CompareScope, typeof DiffAction.add], DiffEntry<DiffAdd>>([childCtx.after.value, buildPathsIdentifier(childCtx.after.declarativePaths), childCtx.scope, DiffAction.add], () => {
+  const addDiffEntry = diffUniquenessCache.cacheEvaluationResultByFootprint<[unknown, string, CompareScope, typeof DiffAction.add], DiffAdd>([childCtx.after.value, buildPathsIdentifier(childCtx.after.declarativePaths), childCtx.scope, DiffAction.add], () => {
     return diffFactory.added(childCtx)
-  }, {} as DiffEntry<DiffAdd>, (result, guard) => {
+  }, {} as DiffAdd, (result, guard) => {
     Object.assign(guard, result)
     return guard
   })
+
+  return createDiffEntry(childCtx, addDiffEntry)
 }
 
 export const getOrCreateChildDiffRemove = (diffUniquenessCache: EvaluationCacheService, childCtx: CompareContext) => {
-  return diffUniquenessCache.cacheEvaluationResultByFootprint<[unknown, string, CompareScope, typeof DiffAction.remove], DiffEntry<DiffRemove>>([childCtx.before.value, buildPathsIdentifier(childCtx.before.declarativePaths), childCtx.scope, DiffAction.remove], () => {
+  const getOrCreateDiffEntry = diffUniquenessCache.cacheEvaluationResultByFootprint<[unknown, string, CompareScope, typeof DiffAction.remove], DiffRemove>([childCtx.before.value, buildPathsIdentifier(childCtx.before.declarativePaths), childCtx.scope, DiffAction.remove], () => {
     return diffFactory.removed(childCtx)
-  }, {} as DiffEntry<DiffRemove>, (result, guard) => {
+  }, {} as DiffRemove, (result, guard) => {
     Object.assign(guard, result)
     return guard
   })
+  return createDiffEntry(childCtx, getOrCreateDiffEntry)
 }
 
 const adaptValues = (beforeJso: JsonNode, beforeKey: PropertyKey, afterJso: JsonNode, afterKey: PropertyKey, adapter: AdapterResolver[] | undefined, options: InternalCompareOptions) => {
@@ -265,7 +268,7 @@ const useMergeFactory = (onDiff: DiffCallback, options: InternalCompareOptions):
 
     const reuseResult: ReusableMergeResult = mergedJsoCache.cacheEvaluationResultByFootprint<[typeof ctx.before.value, typeof ctx.after.value, typeof beforeDeclarativePathsId, typeof afterDeclarativePathsId, CompareScope], ReusableMergeResult>([ctx.before.value, ctx.after.value, beforeDeclarativePathsId, afterDeclarativePathsId, ctx.scope], ([beforeValue, afterValue]) => {
       if (!ignoreKeyDifference && beforeKey !== afterKey) {
-        const diffEntry = diffFactory.renamed(ctx)
+        const diffEntry = createDiffEntry(ctx, diffFactory.renamed(ctx))
         addDiff(diffEntry.diff)
         addDiffObjectToContainer(mergedJso, metaKey, [diffEntry])
       }
@@ -279,7 +282,7 @@ const useMergeFactory = (onDiff: DiffCallback, options: InternalCompareOptions):
 
       // types are different
       if (typeOf(beforeValue) !== typeOf(afterValue)) {
-        const diffEntry = diffFactory.replaced(ctx)
+        const diffEntry = createDiffEntry(ctx, diffFactory.replaced(ctx))
         addDiff(diffEntry.diff)
         return { diffsToPullUp: [diffEntry], mergedValue: afterValue } satisfies ReusableMergeResult
       }
@@ -336,7 +339,7 @@ const useMergeFactory = (onDiff: DiffCallback, options: InternalCompareOptions):
         diffsToPullUp: diffsToPullUp,
       }
       if (beforeValue !== afterValue) {
-        const diffEntry = diffFactory.replaced(ctx)
+        const diffEntry = createDiffEntry(ctx, diffFactory.replaced(ctx))
         diffsToPullUp.push(diffEntry)
         addDiff(diffEntry.diff)
       }

src/core/diff.ts

@@ -31,51 +31,46 @@ export const createDiff = <D extends Diff>(diff: Omit<D, 'type'>, ctx: CompareCo
   return mutableDiffCopy
 }
 
-export const diffFactory: DiffFactory = {
-  added: (ctx) => ({
-    propertyKey: ctx.mergeKey,
-    diff: createDiff({
-      afterValue: ctx.after?.value,
-      afterNormalizedValue: ctx.after?.value,
-      action: DiffAction.add,
-      afterDeclarationPaths: ctx.after.declarativePaths,
-      scope: ctx.scope,
-    }, ctx),
-  }),
-  removed: (ctx) => ({
-    propertyKey: ctx.mergeKey,
-    diff: createDiff({
-      beforeValue: ctx.before.value,
-      beforeNormalizedValue: ctx.before.value,
-      action: DiffAction.remove,
-      beforeDeclarationPaths: ctx.before.declarativePaths,
-      scope: ctx.scope,
-    }, ctx),
-  }),
-  replaced: (ctx) => ({
-    propertyKey: ctx.mergeKey,
-    diff: createDiff({
-      beforeValue: ctx.before.value,
-      beforeNormalizedValue: ctx.before.value,
-      afterValue: ctx.after.value,
-      afterNormalizedValue: ctx.after.value,
-      action: DiffAction.replace,
-      afterDeclarationPaths: ctx.after.declarativePaths,
-      beforeDeclarationPaths: ctx.before.declarativePaths,
-      scope: ctx.scope,
-    }, ctx),
-  }),
-  renamed: (ctx) => ({
+export function createDiffEntry(ctx: CompareContext, diff: Diff): DiffEntry<Diff> {
+  return ({
     propertyKey: ctx.mergeKey,
-    diff: createDiff({
-      beforeKey: ctx.before?.key,
-      afterKey: ctx.after?.key,
-      action: DiffAction.rename,
-      afterDeclarationPaths: ctx.after?.declarativePaths ?? [],
-      beforeDeclarationPaths: ctx.before?.declarativePaths ?? [],
-      scope: ctx.scope,
-    }, ctx),
-  }),
+    diff: diff,
+  })
+}
+
+export const diffFactory: DiffFactory = {
+  added: (ctx) => createDiff({
+    afterValue: ctx.after?.value,
+    afterNormalizedValue: ctx.after?.value,
+    action: DiffAction.add,
+    afterDeclarationPaths: ctx.after.declarativePaths,
+    scope: ctx.scope,
+  }, ctx),
+  removed: (ctx) => createDiff({
+    beforeValue: ctx.before.value,
+    beforeNormalizedValue: ctx.before.value,
+    action: DiffAction.remove,
+    beforeDeclarationPaths: ctx.before.declarativePaths,
+    scope: ctx.scope,
+  }, ctx),
+  replaced: (ctx) => createDiff({
+    beforeValue: ctx.before.value,
+    beforeNormalizedValue: ctx.before.value,
+    afterValue: ctx.after.value,
+    afterNormalizedValue: ctx.after.value,
+    action: DiffAction.replace,
+    afterDeclarationPaths: ctx.after.declarativePaths,
+    beforeDeclarationPaths: ctx.before.declarativePaths,
+    scope: ctx.scope,
+  }, ctx),
+  renamed: (ctx) => createDiff({
+    beforeKey: ctx.before?.key,
+    afterKey: ctx.after?.key,
+    action: DiffAction.rename,
+    afterDeclarationPaths: ctx.after?.declarativePaths ?? [],
+    beforeDeclarationPaths: ctx.before?.declarativePaths ?? [],
+    scope: ctx.scope,
+  }, ctx),
 }
 
 export const addDiffObjectToContainer = (

src/graphapi/graphapi.resolver.ts

@@ -1,4 +1,4 @@
-import { diffFactory } from '../core'
+import { createDiffEntry, diffFactory } from '../core'
 import { CompareResolver } from '../types/rules'
 import { isObject, isString } from '../utils'
 
@@ -9,17 +9,17 @@ export const complexTypeCompareResolver: CompareResolver = (ctx) => {
   const afterValue = after.value
 
   if (!isObject(beforeValue) || !isObject(afterValue) || !isString(beforeValue.kind) || !isString(afterValue.kind)) {
-    const diffEntry = diffFactory.replaced(ctx)
+    const diffEntry = createDiffEntry(ctx, diffFactory.replaced(ctx))
     // actually we don't make deep copy here and create "way to modify" original source. But fix not so trivial and expensive for performance
     return { diffs: [diffEntry.diff], ownerDiffEntry: diffEntry, merged: after.value }
   }
 
-  if (beforeValue.kind === afterValue.kind) { 
+  if (beforeValue.kind === afterValue.kind) {
     return undefined
   }
 
   //TODO add more better way to compare interface vs type 
 
-  const diffEntry = diffFactory.replaced(ctx)
+  const diffEntry = createDiffEntry(ctx, diffFactory.replaced(ctx))
   return { diffs: [diffEntry.diff], ownerDiffEntry: diffEntry, merged: after.value }
 }

src/jsonSchema/jsonSchema.resolver.ts

@@ -8,6 +8,7 @@ import {
   getOrCreateChildDiffAdd,
   getOrCreateChildDiffRemove,
   nestedCompare,
+  createDiffEntry,
 } from '../core'
 import type { CompareResolver, Diff, DiffEntry } from '../types'
 import { isArray, isObject, onlyExistedArrayIndexes } from '../utils'
@@ -21,7 +22,7 @@ export const combinersCompareResolver: CompareResolver = (ctx) => {
   }
 
   if (!isArray(before.value) || !isArray(after.value)) {
-    const diffEntry = diffFactory.replaced(ctx)
+    const diffEntry = createDiffEntry(ctx, diffFactory.replaced(ctx))
     // actually we don't make deep copy here and create "way to modify" original source
     return { diffs: [diffEntry.diff], ownerDiffEntry: diffEntry, merged: after.value }
   }

src/types/compare.ts

@@ -142,10 +142,10 @@ export interface DiffEntry<D extends Diff> {
 }
 
 export interface DiffFactory {
-  added: (ctx: CompareContext) => DiffEntry<DiffAdd>
-  removed: (ctx: CompareContext) => DiffEntry<DiffRemove>
-  replaced: (ctx: CompareContext) => DiffEntry<DiffReplace>
-  renamed: (ctx: CompareContext) => DiffEntry<DiffRename>
+  added: (ctx: CompareContext) => DiffAdd
+  removed: (ctx: CompareContext) => DiffRemove
+  replaced: (ctx: CompareContext) => DiffReplace
+  renamed: (ctx: CompareContext) => DiffRename
 }
 
 export interface ContextInput extends MergeState {

test/bugs.test.ts

@@ -1,4 +1,13 @@
-import { annotation, apiDiff, ClassifierType, CompareOptions, DiffAction, nonBreaking, unclassified } from '../src'
+import {
+  annotation,
+  apiDiff,
+  breaking,
+  ClassifierType,
+  CompareOptions,
+  DiffAction,
+  nonBreaking,
+  unclassified,
+} from '../src'
 import offeringQualificationBefore from './helper/resources/api-v2-offeringqualification-qualification-post/before.json'
 import offeringQualificationAfter from './helper/resources/api-v2-offeringqualification-qualification-post/after.json'
 import readDefaultValueOfRequiredBefore from './helper/resources/read-default-value-of-required-field/before.json'
@@ -30,6 +39,15 @@ import spearedParamsAfter from './helper/resources/speared-parameters/after.json
 import wildcardContentSchemaMediaTypeCombinedWithSpecificMediaTypeBefore from './helper/resources/wildcard-content-schema-media-type-combined-with-specific-media-type/before.json'
 import wildcardContentSchemaMediaTypeCombinedWithSpecificMediaTypeAfter from './helper/resources/wildcard-content-schema-media-type-combined-with-specific-media-type/after.json'
 
+import shouldNotMissRemoveDiffForEnumEntryInOneOfBefore from './helper/resources/should-not-miss-remove-diff-for-enum-entry-in-oneOf/before.json'
+import shouldNotMissRemoveDiffForEnumEntryInOneOfAfter from './helper/resources/should-not-miss-remove-diff-for-enum-entry-in-oneOf/after.json'
+
+import shouldCalculateDiffsCorrectlyInOneOfBefore from './helper/resources/should-calculate-diffs-correctly-in-oneOf/before.json'
+import shouldCalculateDiffsCorrectlyInOneOfAfter from './helper/resources/should-calculate-diffs-correctly-in-oneOf/after.json'
+
+import shouldCalculateDiffsCorrectlyInAnyOfBefore from './helper/resources/should-calculate-diffs-correctly-in-anyOf/before.json'
+import shouldCalculateDiffsCorrectlyInAnyOfAfter from './helper/resources/should-calculate-diffs-correctly-in-anyOf/after.json'
+
 import { diffsMatcher } from './helper/matchers'
 import { TEST_DIFF_FLAG, TEST_ORIGINS_FLAG } from './helper'
 import { JSON_SCHEMA_NODE_SYNTHETIC_TYPE_NOTHING } from '@netcracker/qubership-apihub-api-unifier'
@@ -227,4 +245,91 @@ describe('Real Data', () => {
       }),
     ]))
   })
+
+  it('should not miss remove diff for enum entry in oneOf', () => {
+    const before: any = shouldNotMissRemoveDiffForEnumEntryInOneOfBefore
+    const after: any = shouldNotMissRemoveDiffForEnumEntryInOneOfAfter
+    const { merged } = apiDiff(before, after, OPTIONS)
+
+    expect(
+      Object.values((merged as any).paths['/path1'].post.requestBody.content['application/json'].schema.oneOf[1].properties.scope.items.enum[TEST_DIFF_FLAG])
+    ).toEqual(diffsMatcher([
+      expect.objectContaining({
+        beforeValue: 'query',
+        action: DiffAction.remove,
+        type: breaking,
+      }),
+      expect.objectContaining({
+        beforeValue: 'subscription',
+        action: DiffAction.remove,
+        type: breaking,
+      }),
+      expect.objectContaining({
+        afterValue: 'argument',
+        action: DiffAction.add,
+        type: nonBreaking,
+      }),
+      expect.objectContaining({
+        afterValue: 'annotation',
+        action: DiffAction.add,
+        type: nonBreaking,
+      }),
+    ]))
+  })
+
+  // check diffUniquenessCache works correctly for oneOf
+  it('should calculate diffs correctly in oneOf', () => {
+    const before: any = shouldCalculateDiffsCorrectlyInOneOfBefore
+    const after: any = shouldCalculateDiffsCorrectlyInOneOfAfter
+    const { diffs } = apiDiff(before, after, OPTIONS)
+
+    expect(diffs).toEqual(diffsMatcher([
+      expect.objectContaining({
+        action: DiffAction.add,
+        afterValue: "eventType",
+        afterNormalizedValue: "eventType",
+        afterDeclarationPaths: [[
+          "paths",
+          "/path1",
+          "get",
+          "responses",
+          "200",
+          "content",
+          "application/json",
+          "schema",
+          "required",
+          0
+        ]],
+        type: nonBreaking,
+      }),
+    ]))
+  })
+
+  // check diffUniquenessCache works correctly for anyOf
+  it('should calculate diffs correctly in anyOf', () => {
+    const before: any = shouldCalculateDiffsCorrectlyInAnyOfBefore
+    const after: any = shouldCalculateDiffsCorrectlyInAnyOfAfter
+    const { diffs } = apiDiff(before, after, OPTIONS)
+
+    expect(diffs).toEqual(diffsMatcher([
+      expect.objectContaining({
+        action: DiffAction.add,
+        afterValue: "eventType",
+        afterNormalizedValue: "eventType",
+        afterDeclarationPaths: [[
+          "paths",
+          "/path1",
+          "get",
+          "responses",
+          "200",
+          "content",
+          "application/json",
+          "schema",
+          "required",
+          0
+        ]],
+        type: nonBreaking,
+      }),
+    ]))
+  })
 })

test/helper/resources/should-calculate-diffs-correctly-in-anyOf/after.json

@@ -0,0 +1,56 @@
+{
+  "openapi": "3.0.3",
+  "info": {
+    "title": "test",
+    "version": "0.1.0"
+  },
+  "paths": {
+    "/path1": {
+      "get": {
+        "responses": {
+          "200": {
+            "description": "Success",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "type": "object",
+                  "required": [
+                    "eventType"
+                  ],
+                  "oneOf": [
+                    {
+                      "type": "object",
+                      "required": [
+                        "memberId",
+                        "memberName"
+                      ],
+                      "properties": {
+                        "eventType": {
+                          "type": "string"
+                        },
+                        "memberId": {
+                          "type": "string"
+                        },
+                        "memberName": {
+                          "type": "string"
+                        }
+                      }
+                    },
+                    {
+                      "type": "object",
+                      "properties": {
+                        "eventType": {
+                          "type": "string"
+                        }
+                      }
+                    }
+                  ]
+                }
+              }
+            }
+          }
+        }
+      }
+    }
+  }
+}