diff --git a/eng/tools/openapi-diff-runner/src/command-helpers.ts b/eng/tools/openapi-diff-runner/src/command-helpers.ts index 046178d2ac9d..da213cd4f0ab 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,60 @@ 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)); + // Try to manually match deletions with additions to form renames + + // Store matches in temp array, to avoid modifying "filteredDeletions" while iterating + const matchedRenames: { from: string; to: string }[] = []; + + for (const deletion of filteredDeletions) { + const deletionDir = path.dirname(deletion).toLowerCase(); + + const deletionMatches = filteredDeletions.filter( + (d) => path.dirname(d).toLowerCase() === deletionDir, + ); + if (deletionMatches.length > 1) { + // If more than one deletion from same folder, we can't match. + // Example would be migrating from "a.json, b.json" to "c.json" + continue; + } + + const additionMatches = filteredAdditions.filter( + (a) => path.dirname(a).toLowerCase() === deletionDir, + ); + if (additionMatches.length !== 1) { + // If more than one addition in same folder, we can't match. + // Example would be migrating from "a.json" to "b.json, c.json" + continue; + } + + // Exactly one matching deletion and addition, so treat it like a "git rename" + matchedRenames.push({ from: deletion, to: additionMatches[0] }); + } + + // Update arrays with matchedRenames (if any) + 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 +206,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..2defacfbbd04 100644 --- a/eng/tools/openapi-diff-runner/test/command-helpers.test.ts +++ b/eng/tools/openapi-diff-runner/test/command-helpers.test.ts @@ -349,14 +349,15 @@ describe("command-helpers", () => { // 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 +376,7 @@ describe("command-helpers", () => { additions: [], modifications: [], deletions: [], + renames: [], total: 0, }); }); @@ -415,17 +417,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, // 2 additions + 1 modification + 1 deletion + 1 rename }); }); @@ -474,18 +475,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 addition + 1 modification + 1 deletion + 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 221b9d2bb389..a13117016cba 100644 --- a/eng/tools/openapi-diff-runner/test/commands.test.ts +++ b/eng/tools/openapi-diff-runner/test/commands.test.ts @@ -234,31 +234,48 @@ const cases = [ ], }, }, + // TODO: Add case for new stable, renamed file { name: "rename one file, one version", changedFiles: { renames: [ { - from: "specification/foo/data-plane/Foo/stable/2025-01-01/foo.json", - to: "specification/foo/data-plane/Foo/stable/2025-01-01/openapi.json", + from: "specification/foo/data-plane/Foo/stable/2025-03-01/foo.json", + to: "specification/foo/data-plane/Foo/stable/2025-03-01/openapi.json", }, ], }, - // TODO: After code is fixed, should not create *any* dummy swaggers expectedCreateDummySwaggers: { - old: ["specification/foo/data-plane/Foo/stable/2025-01-01/openapi.json"], - new: ["specification/foo/data-plane/Foo/stable/2025-01-01/foo.json"], + old: [], + new: [], }, - // TODO: After code is fixed, should only compare before and after renamed file expectedOadCalls: { sameVersion: [ { - old: "specification/foo/data-plane/Foo/stable/2025-01-01/foo.json", - new: "specification/foo/data-plane/Foo/stable/2025-01-01/foo.json", + old: "specification/foo/data-plane/Foo/stable/2025-03-01/foo.json", + new: "specification/foo/data-plane/Foo/stable/2025-03-01/openapi.json", }, + ], + crossVersion: [], + }, + }, + { + 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: ["specification/foo/data-plane/Foo/stable/2025-03-01/openapi.json"], + deletions: ["specification/foo/data-plane/Foo/stable/2025-03-01/foo.json"], + }, + expectedCreateDummySwaggers: { + old: [], + new: [], + }, + expectedOadCalls: { + sameVersion: [ { - old: "specification/foo/data-plane/Foo/stable/2025-01-01/openapi.json", - new: "specification/foo/data-plane/Foo/stable/2025-01-01/openapi.json", + old: "specification/foo/data-plane/Foo/stable/2025-03-01/foo.json", + new: "specification/foo/data-plane/Foo/stable/2025-03-01/openapi.json", }, ], crossVersion: [], @@ -267,59 +284,88 @@ const cases = [ { name: "rename one file, change case of service", changedFiles: { - addtions: ["specification/foo/data-plane/foo/stable/2025-01-01/openapi.json"], - deletions: ["specification/foo/data-plane/Foo/stable/2025-01-01/foo.json"], + additions: ["specification/foo/data-plane/foo/stable/2025-03-01/openapi.json"], + deletions: ["specification/foo/data-plane/Foo/stable/2025-03-01/foo.json"], + }, + expectedCreateDummySwaggers: { + old: [], + new: [], + }, + expectedOadCalls: { + sameVersion: [ + { + old: "specification/foo/data-plane/Foo/stable/2025-03-01/foo.json", + new: "specification/foo/data-plane/foo/stable/2025-03-01/openapi.json", + }, + ], + crossVersion: [], + }, + }, + { + name: "rename files, change case of service, two versions", + changedFiles: { + additions: ["specification/foo/data-plane/foo/stable/2025-03-01/openapi.json"], + deletions: ["specification/foo/data-plane/Foo/stable/2025-03-01/foo.json"], + renames: [ + { + from: "specification/foo/data-plane/Foo/stable/2025-01-01/foo.json", + to: "specification/foo/data-plane/foo/stable/2025-01-01/foo.json", + }, + ], }, - // TODO: After code is fixed, should not create *any* dummy swaggers expectedCreateDummySwaggers: { old: [], - new: ["specification/foo/data-plane/Foo/stable/2025-01-01/foo.json"], + new: [], }, - // TODO: After code is fixed, should only compare before and after renamed file expectedOadCalls: { sameVersion: [ { old: "specification/foo/data-plane/Foo/stable/2025-01-01/foo.json", - new: "specification/foo/data-plane/Foo/stable/2025-01-01/foo.json", + new: "specification/foo/data-plane/foo/stable/2025-01-01/foo.json", + }, + { + old: "specification/foo/data-plane/Foo/stable/2025-03-01/foo.json", + new: "specification/foo/data-plane/foo/stable/2025-03-01/openapi.json", + }, + ], + crossVersion: [], + }, + }, + { + name: "convert two swaggers to one", + 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", + ], + renames: [], + }, + expectedCreateDummySwaggers: { + old: ["specification/bar/data-plane/Bar/stable/2025-03-01/openapi.json"], + new: [ + "specification/bar/data-plane/Bar/stable/2025-03-01/bar.json", + "specification/bar/data-plane/Bar/stable/2025-03-01/baz.json", + ], + }, + expectedOadCalls: { + sameVersion: [ + { + old: "specification/bar/data-plane/Bar/stable/2025-03-01/bar.json", + new: "specification/bar/data-plane/Bar/stable/2025-03-01/bar.json", + }, + { + old: "specification/bar/data-plane/Bar/stable/2025-03-01/baz.json", + new: "specification/bar/data-plane/Bar/stable/2025-03-01/baz.json", + }, + { + old: "specification/bar/data-plane/Bar/stable/2025-03-01/openapi.json", + new: "specification/bar/data-plane/Bar/stable/2025-03-01/openapi.json", }, ], crossVersion: [], }, }, - - // Currently failing, code needs better support for renames - // - // { - // name: "change case folder, rename file", - // changedFiles: { - // additions: [ - // "specification/nginx/resource-manager/Nginx.NginxPlus/preview/2025-03-01-preview/openapi.json", - // ], - // deletions: [ - // "specification/nginx/resource-manager/NGINX.NGINXPLUS/preview/2025-03-01-preview/swagger.json", - // ], - // renames: [ - // { - // from: "specification/nginx/resource-manager/NGINX.NGINXPLUS/stable/2023-09-01/swagger.json", - // to: "specification/nginx/resource-manager/Nginx.NginxPlus/stable/2023-09-01/swagger.json", - // }, - // ], - // }, - // existingFiles: [ - // "specification/nginx/resource-manager/NGINX.NGINXPLUS/stable/2023-09-01/swagger.json", - // "specification/nginx/resource-manager/NGINX.NGINXPLUS/preview/2025-03-01-preview/swagger.json", - // ], - // expectedOadCalls: [ - // { - // old: "specification/nginx/resource-manager/NGINX.NGINXPLUS/preview/2025-03-01-preview/swagger.json", - // new: "specification/nginx/resource-manager/Nginx.NginxPlus/preview/2025-03-01-preview/openapi.json", - // }, - // { - // old: "specification/nginx/resource-manager/NGINX.NGINXPLUS/stable/2023-09-01/swagger.json", - // new: "specification/nginx/resource-manager/Nginx.NginxPlus/stable/2023-09-01/swagger.json", - // }, - // ], - // }, ]; describe("validateBreakingChange", () => { 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 },