[Breaking Change]s tooling missed breaking changes due to file renames

[mikekistler]

The Cross Version Breaking Changes checks for azure-rest-api-specs-pr PR 11624 failed to compare against the prior stable version (2020-08-20) and thus missed breaking changes in the new stable version:

This is likely because the files in the prior stable version had slightly different names -- "CommunicationService.json" rather than "CommunicationServices.json".

[konrad-jamrozik]

Related:

• https://github.com/Azure/azure-sdk-tools/issues/7786

[konrad-jamrozik]

Recent email thread: Re: Need Breaking Change Approval on New EventHub Api-2024-05-preview

Bumped priority from low to high.

@mikekistler what do you think about following protection?

Given new GA (say 2024-04-20) and previous GA (say 2023-12-20), we do the following:

1. If spec foo.json exists in previous GA (2023-12-20) but not in the new GA (2024-04-20) we add BreakingChangeReviewRequired, thus blocking the PR.
2. If spec foo.json does not exist in previous GA (2023-12-20) but exists in the new GA (2024-04-20) we do nothing.

Case 1. should protect against file renames. In the reported example, the 2020-08-20/CommunicationService.json no longer exists in 2023-03-31 hence we block.

Case 2. doesn't require any special handling. Adding a new spec file in a new version should be benign. Even if the spec author would try to move some API route definitions from existing spec file to the new one, our checks should detect it, by noticing that existing file lost some content.

For example:

• previous GA: foo.json with routes /customers and /orders.
• new GA: foo.json with route /customers and bar.json with route /orders, where /orders now has been changed in a way that is breaking (e.g. query param renamed).

In such case breaking change detection will notice that /orders is missing from foo.json and report it as a breaking change.

If we implement this the question remains how to actually handle renames.

The simplest solution that comes to my mind is to ask the PR author to have a PR that does only the rename and add Approved-BreakingChange-Benign to such a PR. Later on we could add more advanced automation that detects if the PR is composed solely of a set of file renames that are benign, and if so, then no breaking changes review label is added.

Note the related work item about renames:

• https://github.com/Azure/azure-sdk-tools/issues/7786

[konrad-jamrozik]

Curiously, we already have code logging this case, written 3 years ago.

Source:

  async checkAPIsBeingMovedToANewSpec(swagger: string) {
    const oldApis = this.versionManager.getOldVersionApis(swagger);
    if (oldApis.length) {
      console.log(
        `The swagger ${swagger} has no previous version being found, but its APIs were found in other swaggers. It means that you are moving some APIs to this new swagger file.`
      );
      this.unifiedStore.appendError(
        "APIsBeingMovedToANewSpec",
        "Attention: There are some existing APIs currently documented in a new spec file. The validation may not be able to report breaking changes with these APIs. It is recommended not to rename swagger file or move public APIs to a new file when creating a new API version." +
          `The existing APIs being moved are:${_.uniq(
            oldApis.map((api) => api.operationId)
          ).join(",")};`,
        "Warning",
        swagger
      );
      console.log(
        `The following are details for existing APIs being moved to the new spec：`
      );
      for (const api of oldApis) {
        console.log(
          `  swagger file:${api.swagger},operationId:${api.operationId} \n`
        );
      }
    }
  }

[konrad-jamrozik]

Another case on Teams here