Skip to content

Commit 8f4508a

Browse files
makeev-pavelmakeev-pavel
authored
fix: missing enum breaking diff in oneOf combiner in merged document … (#41)
1 parent e98fc11 commit 8f4508a

File tree

10 files changed

+378
-63
lines changed

10 files changed

+378
-63
lines changed

‎src/core/compare.ts‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ import {
3434
} from '../types'
3535
import { getObjectValue, isArray, isDiffAdd, isDiffRemove, isDiffReplace, isNumber, isObject, typeOf } from '../utils'
3636
import { ANY_COMBINER_PATH, DiffAction, JSO_ROOT } from './constants'
37-
import { addDiffObjectToContainer, diffFactory, NEVER_KEY } from './diff'
37+
import { addDiffObjectToContainer, createDiffEntry, diffFactory, NEVER_KEY } from './diff'
3838
import { arrayMappingResolver, objectMappingResolver } from './mapping'
3939

4040
const extractDeclarationPaths = (jso: Record<PropertyKey, unknown>, originMetaKey: symbol, property: PropertyKey): JsonPath[] => {
@@ -156,21 +156,24 @@ const cleanUpRecursive = (ctx: NodeContext): NodeContext => {
156156
}
157157

158158
export const getOrCreateChildDiffAdd = (diffUniquenessCache: EvaluationCacheService, childCtx: CompareContext) => {
159-
return diffUniquenessCache.cacheEvaluationResultByFootprint<[unknown, string, CompareScope, typeof DiffAction.add], DiffEntry<DiffAdd>>([childCtx.after.value, buildPathsIdentifier(childCtx.after.declarativePaths), childCtx.scope, DiffAction.add], () => {
159+
const diff = diffUniquenessCache.cacheEvaluationResultByFootprint<[unknown, string, CompareScope, typeof DiffAction.add], DiffAdd>([childCtx.after.value, buildPathsIdentifier(childCtx.after.declarativePaths), childCtx.scope, DiffAction.add], () => {
160160
return diffFactory.added(childCtx)
161-
}, {} as DiffEntry<DiffAdd>, (result, guard) => {
161+
}, {} as DiffAdd, (result, guard) => {
162162
Object.assign(guard, result)
163163
return guard
164164
})
165+
166+
return createDiffEntry(childCtx, diff)
165167
}
166168

167169
export const getOrCreateChildDiffRemove = (diffUniquenessCache: EvaluationCacheService, childCtx: CompareContext) => {
168-
return diffUniquenessCache.cacheEvaluationResultByFootprint<[unknown, string, CompareScope, typeof DiffAction.remove], DiffEntry<DiffRemove>>([childCtx.before.value, buildPathsIdentifier(childCtx.before.declarativePaths), childCtx.scope, DiffAction.remove], () => {
170+
const diff = diffUniquenessCache.cacheEvaluationResultByFootprint<[unknown, string, CompareScope, typeof DiffAction.remove], DiffRemove>([childCtx.before.value, buildPathsIdentifier(childCtx.before.declarativePaths), childCtx.scope, DiffAction.remove], () => {
169171
return diffFactory.removed(childCtx)
170-
}, {} as DiffEntry<DiffRemove>, (result, guard) => {
172+
}, {} as DiffRemove, (result, guard) => {
171173
Object.assign(guard, result)
172174
return guard
173175
})
176+
return createDiffEntry(childCtx, diff)
174177
}
175178

176179
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):
235238

236239
const beforeKey = unsafeKey ?? (isArray(beforeJso) ? +Object.keys(keyMap).pop()! : Object.keys(keyMap).pop())
237240
const afterKey = keyMap[beforeKey]
238-
const mergeKey = isArray(mergedJso) && isNumber(beforeKey) ? beforeKey : afterKey//THIS IS VERY FRAGILE. Cause this logic duplicate this line mergedJsoValue[keyInMerge] = afterValue[keyInAfter]
241+
const mergeKey = isArray(mergedJso) && isNumber(beforeKey) ? beforeKey : afterKey //gitleaks:allow //THIS IS VERY FRAGILE. Cause this logic duplicate this line mergedJsoValue[keyInMerge] = afterValue[keyInAfter]
239242

240243
// skip if node was removed
241244
if (!(beforeKey in keyMap)) {
@@ -265,7 +268,7 @@ const useMergeFactory = (onDiff: DiffCallback, options: InternalCompareOptions):
265268

266269
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]) => {
267270
if (!ignoreKeyDifference && beforeKey !== afterKey) {
268-
const diffEntry = diffFactory.renamed(ctx)
271+
const diffEntry = createDiffEntry(ctx, diffFactory.renamed(ctx))
269272
addDiff(diffEntry.diff)
270273
addDiffObjectToContainer(mergedJso, metaKey, [diffEntry])
271274
}
@@ -279,7 +282,7 @@ const useMergeFactory = (onDiff: DiffCallback, options: InternalCompareOptions):
279282

280283
// types are different
281284
if (typeOf(beforeValue) !== typeOf(afterValue)) {
282-
const diffEntry = diffFactory.replaced(ctx)
285+
const diffEntry = createDiffEntry(ctx, diffFactory.replaced(ctx))
283286
addDiff(diffEntry.diff)
284287
return { diffsToPullUp: [diffEntry], mergedValue: afterValue } satisfies ReusableMergeResult
285288
}
@@ -336,7 +339,7 @@ const useMergeFactory = (onDiff: DiffCallback, options: InternalCompareOptions):
336339
diffsToPullUp: diffsToPullUp,
337340
}
338341
if (beforeValue !== afterValue) {
339-
const diffEntry = diffFactory.replaced(ctx)
342+
const diffEntry = createDiffEntry(ctx, diffFactory.replaced(ctx))
340343
diffsToPullUp.push(diffEntry)
341344
addDiff(diffEntry.diff)
342345
}

‎src/core/diff.ts‎

Lines changed: 39 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -31,51 +31,46 @@ export const createDiff = <D extends Diff>(diff: Omit<D, 'type'>, ctx: CompareCo
3131
return mutableDiffCopy
3232
}
3333

34-
export const diffFactory: DiffFactory = {
35-
added: (ctx) => ({
36-
propertyKey: ctx.mergeKey,
37-
diff: createDiff({
38-
afterValue: ctx.after?.value,
39-
afterNormalizedValue: ctx.after?.value,
40-
action: DiffAction.add,
41-
afterDeclarationPaths: ctx.after.declarativePaths,
42-
scope: ctx.scope,
43-
}, ctx),
44-
}),
45-
removed: (ctx) => ({
46-
propertyKey: ctx.mergeKey,
47-
diff: createDiff({
48-
beforeValue: ctx.before.value,
49-
beforeNormalizedValue: ctx.before.value,
50-
action: DiffAction.remove,
51-
beforeDeclarationPaths: ctx.before.declarativePaths,
52-
scope: ctx.scope,
53-
}, ctx),
54-
}),
55-
replaced: (ctx) => ({
56-
propertyKey: ctx.mergeKey,
57-
diff: createDiff({
58-
beforeValue: ctx.before.value,
59-
beforeNormalizedValue: ctx.before.value,
60-
afterValue: ctx.after.value,
61-
afterNormalizedValue: ctx.after.value,
62-
action: DiffAction.replace,
63-
afterDeclarationPaths: ctx.after.declarativePaths,
64-
beforeDeclarationPaths: ctx.before.declarativePaths,
65-
scope: ctx.scope,
66-
}, ctx),
67-
}),
68-
renamed: (ctx) => ({
34+
export function createDiffEntry(ctx: CompareContext, diff: Diff): DiffEntry<Diff> {
35+
return ({
6936
propertyKey: ctx.mergeKey,
70-
diff: createDiff({
71-
beforeKey: ctx.before?.key,
72-
afterKey: ctx.after?.key,
73-
action: DiffAction.rename,
74-
afterDeclarationPaths: ctx.after?.declarativePaths ?? [],
75-
beforeDeclarationPaths: ctx.before?.declarativePaths ?? [],
76-
scope: ctx.scope,
77-
}, ctx),
78-
}),
37+
diff: diff,
38+
})
39+
}
40+
41+
export const diffFactory: DiffFactory = {
42+
added: (ctx) => createDiff({
43+
afterValue: ctx.after?.value,
44+
afterNormalizedValue: ctx.after?.value,
45+
action: DiffAction.add,
46+
afterDeclarationPaths: ctx.after.declarativePaths,
47+
scope: ctx.scope,
48+
}, ctx),
49+
removed: (ctx) => createDiff({
50+
beforeValue: ctx.before.value,
51+
beforeNormalizedValue: ctx.before.value,
52+
action: DiffAction.remove,
53+
beforeDeclarationPaths: ctx.before.declarativePaths,
54+
scope: ctx.scope,
55+
}, ctx),
56+
replaced: (ctx) => createDiff({
57+
beforeValue: ctx.before.value,
58+
beforeNormalizedValue: ctx.before.value,
59+
afterValue: ctx.after.value,
60+
afterNormalizedValue: ctx.after.value,
61+
action: DiffAction.replace,
62+
afterDeclarationPaths: ctx.after.declarativePaths,
63+
beforeDeclarationPaths: ctx.before.declarativePaths,
64+
scope: ctx.scope,
65+
}, ctx),
66+
renamed: (ctx) => createDiff({
67+
beforeKey: ctx.before?.key,
68+
afterKey: ctx.after?.key,
69+
action: DiffAction.rename,
70+
afterDeclarationPaths: ctx.after?.declarativePaths ?? [],
71+
beforeDeclarationPaths: ctx.before?.declarativePaths ?? [],
72+
scope: ctx.scope,
73+
}, ctx),
7974
}
8075

8176
export const addDiffObjectToContainer = (

‎src/graphapi/graphapi.resolver.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { diffFactory } from '../core'
1+
import { createDiffEntry, diffFactory } from '../core'
22
import { CompareResolver } from '../types/rules'
33
import { isObject, isString } from '../utils'
44

@@ -9,17 +9,17 @@ export const complexTypeCompareResolver: CompareResolver = (ctx) => {
99
const afterValue = after.value
1010

1111
if (!isObject(beforeValue) || !isObject(afterValue) || !isString(beforeValue.kind) || !isString(afterValue.kind)) {
12-
const diffEntry = diffFactory.replaced(ctx)
12+
const diffEntry = createDiffEntry(ctx, diffFactory.replaced(ctx))
1313
// actually we don't make deep copy here and create "way to modify" original source. But fix not so trivial and expensive for performance
1414
return { diffs: [diffEntry.diff], ownerDiffEntry: diffEntry, merged: after.value }
1515
}
1616

17-
if (beforeValue.kind === afterValue.kind) {
17+
if (beforeValue.kind === afterValue.kind) {
1818
return undefined
1919
}
2020

2121
//TODO add more better way to compare interface vs type
2222

23-
const diffEntry = diffFactory.replaced(ctx)
23+
const diffEntry = createDiffEntry(ctx, diffFactory.replaced(ctx))
2424
return { diffs: [diffEntry.diff], ownerDiffEntry: diffEntry, merged: after.value }
2525
}

‎src/jsonSchema/jsonSchema.resolver.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
getOrCreateChildDiffAdd,
99
getOrCreateChildDiffRemove,
1010
nestedCompare,
11+
createDiffEntry,
1112
} from '../core'
1213
import type { CompareResolver, Diff, DiffEntry } from '../types'
1314
import { isArray, isObject, onlyExistedArrayIndexes } from '../utils'
@@ -21,7 +22,7 @@ export const combinersCompareResolver: CompareResolver = (ctx) => {
2122
}
2223

2324
if (!isArray(before.value) || !isArray(after.value)) {
24-
const diffEntry = diffFactory.replaced(ctx)
25+
const diffEntry = createDiffEntry(ctx, diffFactory.replaced(ctx))
2526
// actually we don't make deep copy here and create "way to modify" original source
2627
return { diffs: [diffEntry.diff], ownerDiffEntry: diffEntry, merged: after.value }
2728
}

‎src/types/compare.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,10 @@ export interface DiffEntry<D extends Diff> {
142142
}
143143

144144
export interface DiffFactory {
145-
added: (ctx: CompareContext) => DiffEntry<DiffAdd>
146-
removed: (ctx: CompareContext) => DiffEntry<DiffRemove>
147-
replaced: (ctx: CompareContext) => DiffEntry<DiffReplace>
148-
renamed: (ctx: CompareContext) => DiffEntry<DiffRename>
145+
added: (ctx: CompareContext) => DiffAdd
146+
removed: (ctx: CompareContext) => DiffRemove
147+
replaced: (ctx: CompareContext) => DiffReplace
148+
renamed: (ctx: CompareContext) => DiffRename
149149
}
150150

151151
export interface ContextInput extends MergeState {

‎test/bugs.test.ts‎

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,13 @@
1-
import { annotation, apiDiff, ClassifierType, CompareOptions, DiffAction, nonBreaking, unclassified } from '../src'
1+
import {
2+
annotation,
3+
apiDiff,
4+
breaking,
5+
ClassifierType,
6+
CompareOptions,
7+
DiffAction,
8+
nonBreaking,
9+
unclassified,
10+
} from '../src'
211
import offeringQualificationBefore from './helper/resources/api-v2-offeringqualification-qualification-post/before.json'
312
import offeringQualificationAfter from './helper/resources/api-v2-offeringqualification-qualification-post/after.json'
413
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
3039
import wildcardContentSchemaMediaTypeCombinedWithSpecificMediaTypeBefore from './helper/resources/wildcard-content-schema-media-type-combined-with-specific-media-type/before.json'
3140
import wildcardContentSchemaMediaTypeCombinedWithSpecificMediaTypeAfter from './helper/resources/wildcard-content-schema-media-type-combined-with-specific-media-type/after.json'
3241

42+
import shouldNotMissRemoveDiffForEnumEntryInOneOfBefore from './helper/resources/should-not-miss-remove-diff-for-enum-entry-in-oneOf/before.json'
43+
import shouldNotMissRemoveDiffForEnumEntryInOneOfAfter from './helper/resources/should-not-miss-remove-diff-for-enum-entry-in-oneOf/after.json'
44+
45+
import shouldReportSingleDiffWhenRequiredPropertyIsChangedForTheCombinerBefore from './helper/resources/should-report-single-diff-when-required-property-is-changed-for-the-combiner/before.json'
46+
import shouldReportSingleDiffWhenRequiredPropertyIsChangedForTheCombinerAfter from './helper/resources/should-report-single-diff-when-required-property-is-changed-for-the-combiner/after.json'
47+
3348
import { diffsMatcher } from './helper/matchers'
3449
import { TEST_DIFF_FLAG, TEST_ORIGINS_FLAG } from './helper'
3550
import { JSON_SCHEMA_NODE_SYNTHETIC_TYPE_NOTHING } from '@netcracker/qubership-apihub-api-unifier'
@@ -227,4 +242,64 @@ describe('Real Data', () => {
227242
}),
228243
]))
229244
})
245+
246+
it('should not miss remove diff for enum entry in oneOf', () => {
247+
const before: any = shouldNotMissRemoveDiffForEnumEntryInOneOfBefore
248+
const after: any = shouldNotMissRemoveDiffForEnumEntryInOneOfAfter
249+
const { merged } = apiDiff(before, after, OPTIONS)
250+
251+
expect(
252+
Object.values((merged as any).paths['/path1'].post.requestBody.content['application/json'].schema.oneOf[1].properties.scope.items.enum[TEST_DIFF_FLAG])
253+
).toEqual(diffsMatcher([
254+
expect.objectContaining({
255+
beforeValue: 'query',
256+
action: DiffAction.remove,
257+
type: breaking,
258+
}),
259+
expect.objectContaining({
260+
beforeValue: 'subscription',
261+
action: DiffAction.remove,
262+
type: breaking,
263+
}),
264+
expect.objectContaining({
265+
afterValue: 'argument',
266+
action: DiffAction.add,
267+
type: nonBreaking,
268+
}),
269+
expect.objectContaining({
270+
afterValue: 'annotation',
271+
action: DiffAction.add,
272+
type: nonBreaking,
273+
}),
274+
]))
275+
})
276+
277+
// check diffUniquenessCache works correctly for oneOf
278+
it('should report single diff when required property is changed for the combiner', () => {
279+
const before: any = shouldReportSingleDiffWhenRequiredPropertyIsChangedForTheCombinerBefore
280+
const after: any = shouldReportSingleDiffWhenRequiredPropertyIsChangedForTheCombinerAfter
281+
const { diffs } = apiDiff(before, after, OPTIONS)
282+
283+
expect(diffs).toHaveLength(1)
284+
expect(diffs).toEqual(diffsMatcher([
285+
expect.objectContaining({
286+
action: DiffAction.add,
287+
afterValue: "eventType",
288+
afterNormalizedValue: "eventType",
289+
afterDeclarationPaths: [[
290+
"paths",
291+
"/path1",
292+
"get",
293+
"responses",
294+
"200",
295+
"content",
296+
"application/json",
297+
"schema",
298+
"required",
299+
0
300+
]],
301+
type: nonBreaking,
302+
}),
303+
]))
304+
})
230305
})

‎test/helper/resources/should-not-miss-remove-diff-for-enum-entry-in-oneOf/after.json‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
{
2+
"openapi": "3.0.3",
3+
"info": {
4+
"title": "test",
5+
"version": "0.1.0"
6+
},
7+
"paths": {
8+
"/path1": {
9+
"post": {
10+
"requestBody": {
11+
"content": {
12+
"application/json": {
13+
"schema": {
14+
"oneOf": [
15+
{
16+
"type": "object",
17+
"title": "SearchRestParams",
18+
"properties": {
19+
"apiType": {
20+
"type": "string",
21+
"enum": [
22+
"rest"
23+
]
24+
},
25+
"scope": {
26+
"type": "array",
27+
"items": {
28+
"type": "string",
29+
"enum": [
30+
"request"
31+
]
32+
}
33+
}
34+
}
35+
},
36+
{
37+
"type": "object",
38+
"title": "SearchGQLParams",
39+
"properties": {
40+
"scope": {
41+
"type": "array",
42+
"items": {
43+
"type": "string",
44+
"enum": [
45+
"argument",
46+
"annotation"
47+
]
48+
}
49+
}
50+
}
51+
}
52+
]
53+
}
54+
}
55+
}
56+
},
57+
"responses": {
58+
"200": {
59+
"description": "Success",
60+
"content": {}
61+
}
62+
}
63+
}
64+
}
65+
}
66+
}

0 commit comments

Comments
 (0)