diff --git a/src/core/compare.ts b/src/core/compare.ts index 49aff4f..2837282 100644 --- a/src/core/compare.ts +++ b/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, 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>([childCtx.after.value, buildPathsIdentifier(childCtx.after.declarativePaths), childCtx.scope, DiffAction.add], () => { + const diff = 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, (result, guard) => { + }, {} as DiffAdd, (result, guard) => { Object.assign(guard, result) return guard }) + + return createDiffEntry(childCtx, diff) } export const getOrCreateChildDiffRemove = (diffUniquenessCache: EvaluationCacheService, childCtx: CompareContext) => { - return diffUniquenessCache.cacheEvaluationResultByFootprint<[unknown, string, CompareScope, typeof DiffAction.remove], DiffEntry>([childCtx.before.value, buildPathsIdentifier(childCtx.before.declarativePaths), childCtx.scope, DiffAction.remove], () => { + const diff = 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, (result, guard) => { + }, {} as DiffRemove, (result, guard) => { Object.assign(guard, result) return guard }) + return createDiffEntry(childCtx, diff) } const adaptValues = (beforeJso: JsonNode, beforeKey: PropertyKey, afterJso: JsonNode, afterKey: PropertyKey, adapter: AdapterResolver[] | undefined, options: InternalCompareOptions) => { @@ -235,7 +238,7 @@ const useMergeFactory = (onDiff: DiffCallback, options: InternalCompareOptions): const beforeKey = unsafeKey ?? (isArray(beforeJso) ? +Object.keys(keyMap).pop()! : Object.keys(keyMap).pop()) const afterKey = keyMap[beforeKey] - const mergeKey = isArray(mergedJso) && isNumber(beforeKey) ? beforeKey : afterKey//THIS IS VERY FRAGILE. Cause this logic duplicate this line mergedJsoValue[keyInMerge] = afterValue[keyInAfter] + const mergeKey = isArray(mergedJso) && isNumber(beforeKey) ? beforeKey : afterKey //gitleaks:allow //THIS IS VERY FRAGILE. Cause this logic duplicate this line mergedJsoValue[keyInMerge] = afterValue[keyInAfter] // skip if node was removed if (!(beforeKey in keyMap)) { @@ -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) } diff --git a/src/core/diff.ts b/src/core/diff.ts index b95668b..2da9b7e 100644 --- a/src/core/diff.ts +++ b/src/core/diff.ts @@ -31,51 +31,46 @@ export const createDiff = (diff: Omit, 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 { + 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 = ( diff --git a/src/graphapi/graphapi.resolver.ts b/src/graphapi/graphapi.resolver.ts index f373b07..9b8c32f 100644 --- a/src/graphapi/graphapi.resolver.ts +++ b/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 } } diff --git a/src/jsonSchema/jsonSchema.resolver.ts b/src/jsonSchema/jsonSchema.resolver.ts index bc33a99..1a7042b 100644 --- a/src/jsonSchema/jsonSchema.resolver.ts +++ b/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 } } diff --git a/src/types/compare.ts b/src/types/compare.ts index 5cad881..5d2dd2f 100644 --- a/src/types/compare.ts +++ b/src/types/compare.ts @@ -142,10 +142,10 @@ export interface DiffEntry { } export interface DiffFactory { - added: (ctx: CompareContext) => DiffEntry - removed: (ctx: CompareContext) => DiffEntry - replaced: (ctx: CompareContext) => DiffEntry - renamed: (ctx: CompareContext) => DiffEntry + added: (ctx: CompareContext) => DiffAdd + removed: (ctx: CompareContext) => DiffRemove + replaced: (ctx: CompareContext) => DiffReplace + renamed: (ctx: CompareContext) => DiffRename } export interface ContextInput extends MergeState { diff --git a/test/bugs.test.ts b/test/bugs.test.ts index ae02bd6..a6ce3cf 100644 --- a/test/bugs.test.ts +++ b/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,12 @@ 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 shouldReportSingleDiffWhenRequiredPropertyIsChangedForTheCombinerBefore from './helper/resources/should-report-single-diff-when-required-property-is-changed-for-the-combiner/before.json' +import shouldReportSingleDiffWhenRequiredPropertyIsChangedForTheCombinerAfter from './helper/resources/should-report-single-diff-when-required-property-is-changed-for-the-combiner/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 +242,64 @@ 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 report single diff when required property is changed for the combiner', () => { + const before: any = shouldReportSingleDiffWhenRequiredPropertyIsChangedForTheCombinerBefore + const after: any = shouldReportSingleDiffWhenRequiredPropertyIsChangedForTheCombinerAfter + const { diffs } = apiDiff(before, after, OPTIONS) + + expect(diffs).toHaveLength(1) + 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, + }), + ])) + }) }) diff --git a/test/helper/resources/should-not-miss-remove-diff-for-enum-entry-in-oneOf/after.json b/test/helper/resources/should-not-miss-remove-diff-for-enum-entry-in-oneOf/after.json new file mode 100644 index 0000000..82720f4 --- /dev/null +++ b/test/helper/resources/should-not-miss-remove-diff-for-enum-entry-in-oneOf/after.json @@ -0,0 +1,66 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "test", + "version": "0.1.0" + }, + "paths": { + "/path1": { + "post": { + "requestBody": { + "content": { + "application/json": { + "schema": { + "oneOf": [ + { + "type": "object", + "title": "SearchRestParams", + "properties": { + "apiType": { + "type": "string", + "enum": [ + "rest" + ] + }, + "scope": { + "type": "array", + "items": { + "type": "string", + "enum": [ + "request" + ] + } + } + } + }, + { + "type": "object", + "title": "SearchGQLParams", + "properties": { + "scope": { + "type": "array", + "items": { + "type": "string", + "enum": [ + "argument", + "annotation" + ] + } + } + } + } + ] + } + } + } + }, + "responses": { + "200": { + "description": "Success", + "content": {} + } + } + } + } + } +} \ No newline at end of file diff --git a/test/helper/resources/should-not-miss-remove-diff-for-enum-entry-in-oneOf/before.json b/test/helper/resources/should-not-miss-remove-diff-for-enum-entry-in-oneOf/before.json new file mode 100644 index 0000000..9a30dde --- /dev/null +++ b/test/helper/resources/should-not-miss-remove-diff-for-enum-entry-in-oneOf/before.json @@ -0,0 +1,66 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "test", + "version": "0.1.0" + }, + "paths": { + "/path1": { + "post": { + "requestBody": { + "content": { + "application/json": { + "schema": { + "oneOf": [ + { + "type": "object", + "title": "SearchRestParams", + "properties": { + "apiType": { + "type": "string", + "enum": [ + "Rest" + ] + }, + "scope": { + "type": "array", + "items": { + "type": "string", + "enum": [ + "request" + ] + } + } + } + }, + { + "type": "object", + "title": "SearchGQLParams", + "properties": { + "scope": { + "type": "array", + "items": { + "type": "string", + "enum": [ + "query", + "subscription" + ] + } + } + } + } + ] + } + } + } + }, + "responses": { + "200": { + "description": "Success", + "content": {} + } + } + } + } + } +} \ No newline at end of file diff --git a/test/helper/resources/should-report-single-diff-when-required-property-is-changed-for-the-combiner/after.json b/test/helper/resources/should-report-single-diff-when-required-property-is-changed-for-the-combiner/after.json new file mode 100644 index 0000000..149cd51 --- /dev/null +++ b/test/helper/resources/should-report-single-diff-when-required-property-is-changed-for-the-combiner/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" + } + } + } + ] + } + } + } + } + } + } + } + } +} diff --git a/test/helper/resources/should-report-single-diff-when-required-property-is-changed-for-the-combiner/before.json b/test/helper/resources/should-report-single-diff-when-required-property-is-changed-for-the-combiner/before.json new file mode 100644 index 0000000..d3560d1 --- /dev/null +++ b/test/helper/resources/should-report-single-diff-when-required-property-is-changed-for-the-combiner/before.json @@ -0,0 +1,53 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "test", + "version": "0.1.0" + }, + "paths": { + "/path1": { + "get": { + "responses": { + "200": { + "description": "Success", + "content": { + "application/json": { + "schema": { + "type": "object", + "oneOf": [ + { + "type": "object", + "required": [ + "memberId", + "memberName" + ], + "properties": { + "eventType": { + "type": "string" + }, + "memberId": { + "type": "string" + }, + "memberName": { + "type": "string" + } + } + }, + { + "type": "object", + "properties": { + "eventType": { + "type": "string" + } + } + } + ] + } + } + } + } + } + } + } + } +}