diff --git a/eng/tools/openapi-diff-runner/src/command-helpers.ts b/eng/tools/openapi-diff-runner/src/command-helpers.ts index 046178d2ac9d..921c76dd64aa 100644 --- a/eng/tools/openapi-diff-runner/src/command-helpers.ts +++ b/eng/tools/openapi-diff-runner/src/command-helpers.ts @@ -131,6 +131,7 @@ export async function getSwaggerDiffs( additions: string[]; modifications: string[]; deletions: string[]; + renames: { from: string; to: string }[]; total: number; }> { try { @@ -143,22 +144,48 @@ export async function getSwaggerDiffs( }); // Filter each array to only include Swagger files using the swagger filter from changed-files.js - const filteredAdditions = result.additions.filter(swagger); + let filteredAdditions = result.additions.filter(swagger); const filteredModifications = result.modifications.filter(swagger); - const filteredDeletions = result.deletions.filter(swagger); - const filteredRenames = result.renames.filter( + let filteredDeletions = result.deletions.filter(swagger); + let filteredRenames = result.renames.filter( (rename) => swagger(rename.from) && swagger(rename.to), ); - // Add renamed files to the additions array and deletions array - filteredAdditions.push(...filteredRenames.map((rename) => rename.to)); - filteredDeletions.push(...filteredRenames.map((rename) => rename.from)); + // Store matches in temp array, to avoid modifying "filteredDeletions" while iterating + const matchedRenames: { from: string; to: string }[] = []; + + // Try to manually match deletions with additions to form renames + for (const deletion of filteredDeletions) { + const deletionDir = path.dirname(deletion).toLowerCase(); + + const additionMatches = filteredAdditions.filter( + (a) => path.dirname(a).toLowerCase() === deletionDir, + ); + + // If there are multiple matching additions (which should be rare), it's safest + // to add all the combinations as "renames". This will probably always trigger a check failure, + // but it's safest to "fail closed" this way, rather than ignoring the files. + for (const addition of additionMatches) { + matchedRenames.push({ from: deletion, to: addition }); + } + } + + const renamesFrom = matchedRenames.map((r) => r.from); + const renamesTo = matchedRenames.map((r) => r.to); + filteredAdditions = filteredAdditions.filter((a) => !renamesTo.includes(a)); + filteredDeletions = filteredDeletions.filter((d) => !renamesFrom.includes(d)); + filteredRenames = filteredRenames.concat(matchedRenames); return { additions: filteredAdditions, modifications: filteredModifications, deletions: filteredDeletions, - total: filteredAdditions.length + filteredModifications.length + filteredDeletions.length, + renames: filteredRenames, + total: + filteredAdditions.length + + filteredModifications.length + + filteredDeletions.length + + filteredRenames.length, }; } catch (error) { logError(`Error getting categorized changed files: ${error}`); @@ -167,6 +194,7 @@ export async function getSwaggerDiffs( additions: [], modifications: [], deletions: [], + renames: [], total: 0, }; } diff --git a/eng/tools/openapi-diff-runner/src/commands.ts b/eng/tools/openapi-diff-runner/src/commands.ts index 0dea61b5a09a..38ef1ab0ddcd 100644 --- a/eng/tools/openapi-diff-runner/src/commands.ts +++ b/eng/tools/openapi-diff-runner/src/commands.ts @@ -75,6 +75,8 @@ export async function validateBreakingChange(context: Context): Promise const deletedSwaggers = diffs.deletions || []; + const renamedSwaggers = diffs.renames || []; + const newExistingVersionDirs: string[] = []; const addedVersionDirs = [...newSwaggers.map((f: string) => path.dirname(f))]; @@ -156,6 +158,7 @@ export async function validateBreakingChange(context: Context): Promise needCompareOldSwaggers, newVersionSwaggers, newVersionChangedSwaggers, + renamedSwaggers, oadTracer, ); diff --git a/eng/tools/openapi-diff-runner/src/detect-breaking-change.ts b/eng/tools/openapi-diff-runner/src/detect-breaking-change.ts index b56dee2903c9..f0f4aff3a494 100644 --- a/eng/tools/openapi-diff-runner/src/detect-breaking-change.ts +++ b/eng/tools/openapi-diff-runner/src/detect-breaking-change.ts @@ -61,6 +61,7 @@ export interface BreakingChangeDetectionContext { existingVersionSwaggers: string[]; // Files in existing API version directories newVersionSwaggers: string[]; // Files in completely new API version directories newVersionChangedSwaggers: string[]; // Files in existing API version directories that have changed + renamedSwaggers: { from: string; to: string }[]; oadTracer: OadTraceData; msgs: ResultMessageRecord[]; runtimeErrors: RawMessageRecord[]; @@ -75,6 +76,7 @@ export function createBreakingChangeDetectionContext( existingVersionSwaggers: string[], newVersionSwaggers: string[], newVersionChangedSwaggers: string[], + renamedSwaggers: { from: string; to: string }[], oadTracer: OadTraceData, ): BreakingChangeDetectionContext { return { @@ -82,6 +84,7 @@ export function createBreakingChangeDetectionContext( existingVersionSwaggers, newVersionSwaggers, newVersionChangedSwaggers, + renamedSwaggers, oadTracer, msgs: [], runtimeErrors: [], @@ -132,6 +135,20 @@ export async function checkBreakingChangeOnSameVersion( logMessage("Processing completed", LogLevel.EndGroup); } + for (const { from, to } of detectionContext.renamedSwaggers) { + logMessage(`Processing rename: ${from} -> ${to}`, LogLevel.Group); + const { oadViolationsCnt, errorCnt } = await doBreakingChangeDetection( + detectionContext, + path.resolve(detectionContext.context.prInfo!.tempRepoFolder, from), + to, + BREAKING_CHANGES_CHECK_TYPES.SAME_VERSION, + specIsPreview(to) ? ApiVersionLifecycleStage.PREVIEW : ApiVersionLifecycleStage.STABLE, + ); + aggregateOadViolationsCnt += oadViolationsCnt; + aggregateErrorCnt += errorCnt; + logMessage("Processing completed", LogLevel.EndGroup); + } + logMessage( `RETURN definition checkBreakingChangeOnSameVersion. ` + `msgs.length: ${detectionContext.msgs.length}, ` + diff --git a/eng/tools/openapi-diff-runner/test/command-helpers.test.ts b/eng/tools/openapi-diff-runner/test/command-helpers.test.ts index 2a433b9d9cba..5d11430e74d5 100644 --- a/eng/tools/openapi-diff-runner/test/command-helpers.test.ts +++ b/eng/tools/openapi-diff-runner/test/command-helpers.test.ts @@ -347,16 +347,16 @@ describe("command-helpers", () => { headCommitish: TEST_CONSTANTS.COMMITS.HEAD, }); - // Expected result should have renames added to additions and deletions, and no renames property const expectedResult = { - additions: [...mockResult.additions, ...mockResult.renames.map((rename) => rename.to)], + additions: mockResult.additions, modifications: mockResult.modifications, - deletions: [...mockResult.deletions, ...mockResult.renames.map((rename) => rename.from)], + deletions: mockResult.deletions, + renames: mockResult.renames, total: mockResult.additions.length + mockResult.modifications.length + mockResult.deletions.length + - mockResult.renames.length * 2, + mockResult.renames.length, }; expect(result).toEqual(expectedResult); @@ -375,6 +375,7 @@ describe("command-helpers", () => { additions: [], modifications: [], deletions: [], + renames: [], total: 0, }); }); @@ -415,17 +416,16 @@ describe("command-helpers", () => { // Only Swagger files should be returned, with renames added to additions and deletions expect(result).toEqual({ - additions: [ - TEST_CONSTANTS.SWAGGER_PATHS.FOO, - TEST_CONSTANTS.SWAGGER_PATHS.BAZ, - TEST_CONSTANTS.SWAGGER_PATHS.NEW_MGMT, // from valid rename - ], + additions: [TEST_CONSTANTS.SWAGGER_PATHS.FOO, TEST_CONSTANTS.SWAGGER_PATHS.BAZ], modifications: [TEST_CONSTANTS.SWAGGER_PATHS.QUX_MGMT], - deletions: [ - TEST_CONSTANTS.SWAGGER_PATHS.OLD_DATA, - TEST_CONSTANTS.SWAGGER_PATHS.OLD_MGMT, // from valid rename + deletions: [TEST_CONSTANTS.SWAGGER_PATHS.OLD_DATA], + renames: [ + { + from: TEST_CONSTANTS.SWAGGER_PATHS.OLD_MGMT, + to: TEST_CONSTANTS.SWAGGER_PATHS.NEW_MGMT, + }, ], - total: 6, // 3 additions + 1 modification + 2 deletions (including rename files) + total: 5, // 3 additions + 1 modification + 2 deletions (including rename files) }); }); @@ -474,18 +474,20 @@ describe("command-helpers", () => { // Renames should be added to additions and deletions, not returned as renames expect(result).toEqual({ - additions: [ - TEST_CONSTANTS.SWAGGER_PATHS.FOO, - TEST_CONSTANTS.SWAGGER_PATHS.NEW_MGMT, // from rename.to - "specification/newapi/data-plane/stable/2023-01-01/newapi.json", // from rename.to - ], + additions: [TEST_CONSTANTS.SWAGGER_PATHS.FOO], modifications: [TEST_CONSTANTS.SWAGGER_PATHS.BAZ], - deletions: [ - TEST_CONSTANTS.SWAGGER_PATHS.OLD_DATA, - TEST_CONSTANTS.SWAGGER_PATHS.OLD_MGMT, // from rename.from - "specification/oldapi/data-plane/stable/2023-01-01/oldapi.json", // from rename.from + deletions: [TEST_CONSTANTS.SWAGGER_PATHS.OLD_DATA], + renames: [ + { + from: TEST_CONSTANTS.SWAGGER_PATHS.OLD_MGMT, + to: TEST_CONSTANTS.SWAGGER_PATHS.NEW_MGMT, + }, + { + from: "specification/oldapi/data-plane/stable/2023-01-01/oldapi.json", + to: "specification/newapi/data-plane/stable/2023-01-01/newapi.json", + }, ], - total: 7, // 3 additions + 1 modification + 3 deletions (including from renames) + total: 5, // 1 additions + 1 modification + 1 deletions + 2 renames }); }); }); diff --git a/eng/tools/openapi-diff-runner/test/commands.test.ts b/eng/tools/openapi-diff-runner/test/commands.test.ts index 544c61ebeccd..33f9a6e9096a 100644 --- a/eng/tools/openapi-diff-runner/test/commands.test.ts +++ b/eng/tools/openapi-diff-runner/test/commands.test.ts @@ -231,52 +231,121 @@ const cases: TestCase[] = [ changedFiles: { renames: [ { - from: "Foo/stable/2025-01-01/foo.json", - to: "Foo/stable/2025-01-01/openapi.json", + from: "Foo/stable/2025-03-01/foo.json", + to: "Foo/stable/2025-03-01/openapi.json", + }, + ], + }, + expectedOadCalls: { + sameVersion: [ + { + old: "Foo/stable/2025-03-01/foo.json", + new: "Foo/stable/2025-03-01/openapi.json", }, ], }, - // TODO: After code is fixed, should not create *any* dummy swaggers - expectedCreateDummySwaggers: { - old: ["Foo/stable/2025-01-01/openapi.json"], - new: ["Foo/stable/2025-01-01/foo.json"], + }, + { + name: "rename one file, one version, as add/remove", + changedFiles: { + // If a file is renamed, but has too many changes, "git diff" may return it as an + // add/delete, rather than a rename. + additions: ["Foo/stable/2025-03-01/openapi.json"], + deletions: ["Foo/stable/2025-03-01/foo.json"], + }, + expectedOadCalls: { + sameVersion: [ + { + old: "Foo/stable/2025-03-01/foo.json", + new: "Foo/stable/2025-03-01/openapi.json", + }, + ], + }, + }, + { + name: "rename one file, change case of service", + changedFiles: { + additions: ["FOO/stable/2025-03-01/openapi.json"], + deletions: ["Foo/stable/2025-03-01/foo.json"], + }, + expectedOadCalls: { + sameVersion: [ + { + old: "Foo/stable/2025-03-01/foo.json", + new: "FOO/stable/2025-03-01/openapi.json", + }, + ], + }, + }, + { + // Minimal repro for https://github.com/Azure/azure-rest-api-specs/issues/38245 + name: "rename files, change case of service, two versions", + changedFiles: { + additions: ["FOO/stable/2025-03-01/openapi.json"], + deletions: ["Foo/stable/2025-03-01/foo.json"], + renames: [ + { + from: "Foo/stable/2025-01-01/foo.json", + to: "FOO/stable/2025-01-01/foo.json", + }, + ], }, - // TODO: After code is fixed, should only compare before and after renamed file expectedOadCalls: { sameVersion: [ { old: "Foo/stable/2025-01-01/foo.json", - new: "Foo/stable/2025-01-01/foo.json", + new: "FOO/stable/2025-01-01/foo.json", }, { - old: "Foo/stable/2025-01-01/openapi.json", - new: "Foo/stable/2025-01-01/openapi.json", + old: "Foo/stable/2025-03-01/foo.json", + new: "FOO/stable/2025-03-01/openapi.json", + }, + ], + }, + }, + { + name: "convert two swaggers to one (TSP conversion)", + changedFiles: { + additions: ["specification/bar/data-plane/Bar/stable/2025-03-01/openapi.json"], + deletions: [ + "specification/bar/data-plane/Bar/stable/2025-03-01/bar.json", + "specification/bar/data-plane/Bar/stable/2025-03-01/baz.json", + ], + }, + expectedOadCalls: { + // Comparisons are probably invalid, and will fail, but better than comparing to a dummy file + sameVersion: [ + { + old: "specification/bar/data-plane/Bar/stable/2025-03-01/bar.json", + new: "specification/bar/data-plane/Bar/stable/2025-03-01/openapi.json", + }, + { + old: "specification/bar/data-plane/Bar/stable/2025-03-01/baz.json", + new: "specification/bar/data-plane/Bar/stable/2025-03-01/openapi.json", + }, + ], + }, + }, + { + name: "convert one swagger to two (very rare)", + changedFiles: { + additions: ["Foo/stable/2025-03-01/openapi1.json", "Foo/stable/2025-03-01/openapi2.json"], + deletions: ["Foo/stable/2025-03-01/foo.json"], + }, + expectedOadCalls: { + // Comparisons are probably invalid, and will fail, but better than comparing to a dummy file + sameVersion: [ + { + old: "Foo/stable/2025-03-01/foo.json", + new: "Foo/stable/2025-03-01/openapi1.json", + }, + { + old: "Foo/stable/2025-03-01/foo.json", + new: "Foo/stable/2025-03-01/openapi2.json", }, ], }, }, - // { - // name: "rename one file, change case of service", - // changedFiles: { - // additions: ["foo/stable/2025-01-01/openapi.json"], - // deletions: ["Foo/stable/2025-01-01/foo.json"], - // }, - // // TODO: After code is fixed, should not create *any* dummy swaggers - // expectedCreateDummySwaggers: { - // old: [], - // new: ["Foo/stable/2025-01-01/foo.json"], - // }, - // // TODO: After code is fixed, should only compare before and after renamed file - // expectedOadCalls: { - // sameVersion: [ - // { - // old: "Foo/stable/2025-01-01/foo.json", - // new: "Foo/stable/2025-01-01/foo.json", - // }, - // ], - // crossVersion: [], - // }, - // }, ]; describe("validateBreakingChange", () => { @@ -323,6 +392,10 @@ describe("validateBreakingChange", () => { expect(statusCode).toEqual(0); + expect(mockCreateDummySwagger).toBeCalledTimes( + expectedCreateDummySwaggers.old.length + expectedCreateDummySwaggers.new.length, + ); + for (const expected of expectedCreateDummySwaggers.old) { expect(mockCreateDummySwagger).toBeCalledWith( expect.anything(), @@ -334,9 +407,7 @@ describe("validateBreakingChange", () => { expect(mockCreateDummySwagger).toBeCalledWith(expect.anything(), resolve(expected)); } - expect(mockCreateDummySwagger).toBeCalledTimes( - expectedCreateDummySwaggers.old.length + expectedCreateDummySwaggers.new.length, - ); + expect(mockRunOad).toBeCalledTimes(data.expectedOadCalls.length); for (const expected of data.expectedOadCalls) { expect(mockRunOad).toBeCalledWith( @@ -344,8 +415,6 @@ describe("validateBreakingChange", () => { expected.new, ); } - - expect(mockRunOad).toBeCalledTimes(data.expectedOadCalls.length); } }, ); diff --git a/eng/tools/openapi-diff-runner/test/detect-breaking-change.test.ts b/eng/tools/openapi-diff-runner/test/detect-breaking-change.test.ts index f672b4ab4778..47eed9ef8540 100644 --- a/eng/tools/openapi-diff-runner/test/detect-breaking-change.test.ts +++ b/eng/tools/openapi-diff-runner/test/detect-breaking-change.test.ts @@ -197,6 +197,7 @@ describe("detect-breaking-change", () => { existingVersionSwaggers: ["existing1.json", "existing2.json"], newVersionSwaggers: ["new1.json", "new2.json"], newVersionChangedSwaggers: ["changed1.json", "changed2.json"], + renamedSwaggers: [], msgs: [], runtimeErrors: [], oadTracer: { traces: [], baseBranch: "main", context },