diff --git a/.agents/skills/run-apichief/SKILL.md b/.agents/skills/run-apichief/SKILL.md index e3cc6769ef7..d78eef1d2e6 100644 --- a/.agents/skills/run-apichief/SKILL.md +++ b/.agents/skills/run-apichief/SKILL.md @@ -17,7 +17,7 @@ ApiChief can run against either a compiled assembly or a previously emitted base | `emit baseline` | Emit a JSON API baseline | `-o ` | | `emit summary` | Emit a human-readable API summary | `-o `, `-x` | | `emit review` | Emit API review files | `-o `, `-n` | -| `emit delta` | Emit a delta against an existing baseline | ``, `-o ` | +| `emit delta` | Emit a delta against an existing baseline, or markdown diff review files | ``, `-o `, `--diff` | | `check breaking` | Fail if breaking changes exist vs. a baseline | `` | Default to `emit baseline` if the user only asks to "run ApiChief". @@ -105,6 +105,9 @@ Before running, report the selected TFM if it matters for the task. # Emit API review artifacts & $dotnet $apiChief $assemblyPath emit review -o ".\\artifacts\\tmp\\API.$name" + +# Emit GitHub-friendly markdown diff files against a baseline +& $dotnet $apiChief $assemblyPath emit delta ".\\src\\$name\\$name.baseline.json" --diff -o ".\\artifacts\\tmp\\API.$name.Diff" ``` `emit delta` also supports passing a `.json` file as the current input instead of a DLL. @@ -124,4 +127,5 @@ After `emit baseline`: - show the chosen TFM(s) - report the output path(s) - for `check breaking`, state pass/fail -- for `emit delta`, mention that exit code `0` means changes, `2` means no changes, and `-1` means an error +- for `emit delta` and `emit delta --diff`, mention that exit code `0` means changes, `2` means no changes, and `-1` means an error +- prefer `emit delta --diff` output because it generates ready-to-post ```diff fenced markdown split across per-type `.md` files diff --git a/.github/workflows/api-review-baselines.yml b/.github/workflows/api-review-baselines.yml index 76551cc729a..5d407919e6d 100644 --- a/.github/workflows/api-review-baselines.yml +++ b/.github/workflows/api-review-baselines.yml @@ -1,8 +1,8 @@ -# This workflow labels merged PRs that modify `*.baseline.json` files with `api-review`, -# computes ApiChief deltas between the original and merged baseline files, and posts the -# results back to the pull request as a review comment. +# This workflow labels PRs that modify `*.baseline.json` files with `api-review`, +# computes ApiChief deltas between the base and selected PR baseline files, and posts the +# results back to the pull request as a comment. -name: Comment API baseline deltas on merged PRs +name: Comment API baseline deltas on PRs on: pull_request_target: @@ -10,25 +10,45 @@ on: branches: - main - release/** + workflow_dispatch: + inputs: + pr-number: + description: Pull request number to process + required: true + type: number permissions: contents: read issues: write - pull-requests: read + pull-requests: write jobs: api-review: - if: github.event.pull_request.merged == true + if: github.event_name == 'workflow_dispatch' || github.event.pull_request.merged == true runs-on: ubuntu-latest steps: - name: Detect changed baseline files and add label id: detect - uses: actions/github-script@v8 + uses: actions/github-script@v9 with: script: | const owner = context.repo.owner; const repo = context.repo.repo; - const prNumber = context.payload.pull_request.number; + const prNumberInput = context.eventName === 'workflow_dispatch' + ? context.payload.inputs?.['pr-number'] + : context.payload.pull_request.number; + const prNumber = Number(prNumberInput); + + if (!Number.isInteger(prNumber) || prNumber <= 0) { + core.setFailed(`Invalid PR number: ${prNumber}`); + return; + } + + const { data: pullRequest } = await github.rest.pulls.get({ + owner, + repo, + pull_number: prNumber + }); const files = await github.paginate(github.rest.pulls.listFiles, { owner, @@ -45,11 +65,18 @@ jobs: previous_filename: file.previous_filename ?? null })); + core.setOutput('pr_number', String(prNumber)); + core.setOutput('base_sha', pullRequest.base.sha); + core.setOutput('target_sha', pullRequest.merge_commit_sha ?? pullRequest.head.sha); core.setOutput('has_baselines', String(baselineFiles.length > 0)); core.setOutput('files_json', JSON.stringify(baselineFiles)); + if (!pullRequest.merged) { + console.log(`PR #${prNumber} is not merged; using head SHA ${pullRequest.head.sha}.`); + } + if (baselineFiles.length === 0) { - console.log('No changed baseline files detected on this merged PR.'); + console.log(`No changed baseline files detected on PR #${prNumber}.`); return; } @@ -60,13 +87,13 @@ jobs: labels: ['api-review'] }); - console.log(`Detected ${baselineFiles.length} changed baseline file(s).`); + console.log(`Detected ${baselineFiles.length} changed baseline file(s) on PR #${prNumber}.`); - - name: Check out merged commit + - name: Check out selected commit if: steps.detect.outputs.has_baselines == 'true' - uses: actions/checkout@v4 + uses: actions/checkout@v6 with: - ref: ${{ github.event.pull_request.merge_commit_sha }} + ref: ${{ steps.detect.outputs.target_sha }} fetch-depth: 1 - name: Restore repo-local .NET SDK @@ -74,7 +101,7 @@ jobs: shell: bash run: ./restore.sh - - name: Build ApiChief and compute baseline deltas + - name: Build ApiChief and compute review diffs if: steps.detect.outputs.has_baselines == 'true' id: delta shell: bash @@ -82,8 +109,8 @@ jobs: FILES_JSON: ${{ steps.detect.outputs.files_json }} OWNER: ${{ github.repository_owner }} REPO: ${{ github.event.repository.name }} - BASE_SHA: ${{ github.event.pull_request.base.sha }} - MERGE_SHA: ${{ github.event.pull_request.merge_commit_sha }} + BASE_SHA: ${{ steps.detect.outputs.base_sha }} + TARGET_SHA: ${{ steps.detect.outputs.target_sha }} run: | set -euo pipefail @@ -110,7 +137,7 @@ jobs: owner = os.environ['OWNER'] repo = os.environ['REPO'] base_sha = os.environ['BASE_SHA'] - merge_sha = os.environ['MERGE_SHA'] + target_sha = os.environ['TARGET_SHA'] dotnet = os.environ['DOTNET'] apichief = os.environ['APICHIEF_PATH'] workdir = pathlib.Path(os.environ['WORKDIR']) @@ -130,25 +157,36 @@ jobs: return False raise - def truncate(text: str, limit: int = 20000) -> str: - if len(text) <= limit: - return text - return text[:limit] + '\n... (truncated)\n' + max_comment_length = 60000 + comment_bodies: list[str] = [] + current_body = '' + + def append_section(section: str) -> None: + nonlocal current_body - sections: list[str] = [] + if not current_body: + current_body = section + return + + candidate = current_body + '\n\n' + section + if len(candidate) > max_comment_length: + comment_bodies.append(current_body) + current_body = section + else: + current_body = candidate for index, file in enumerate(files): filename = file['filename'] old_path = workdir / f'{index}.old.baseline.json' new_path = workdir / f'{index}.new.baseline.json' - delta_path = workdir / f'{index}.delta.json' + review_dir = workdir / f'{index}.review' old_exists = download_file(base_sha, filename, old_path) - new_exists = download_file(merge_sha, filename, new_path) + new_exists = download_file(target_sha, filename, new_path) if old_exists and new_exists: result = subprocess.run( - [dotnet, apichief, str(new_path), 'emit', 'delta', str(old_path), '-o', str(delta_path)], + [dotnet, apichief, str(new_path), 'emit', 'delta', str(old_path), '--diff', '-o', str(review_dir)], capture_output=True, text=True, check=False) @@ -158,73 +196,77 @@ jobs: if result.returncode != 0: raise RuntimeError( - f'ApiChief delta failed for {filename} with exit code {result.returncode}:\n' + f'ApiChief delta diff failed for {filename} with exit code {result.returncode}:\n' f'{result.stdout}\n{result.stderr}') - delta_text = truncate(delta_path.read_text(encoding='utf-8')) - sections.append( - f"### `{filename}`\n\n" - f"
\nShow delta\n\n```json\n{delta_text}\n```\n
") + review_files = sorted(review_dir.rglob('*.md')) + if not review_files: + continue + + section = ( + f"## API review baseline changes for `{filename}`\n\n" + 'The diff below was generated by `ApiChief` between the base and selected PR versions.\n\n') + + for review_file in review_files: + section += ( + f"{review_file.read_text(encoding='utf-8').rstrip()}\n\n") + + append_section(section.rstrip()) elif new_exists: - sections.append(f"### `{filename}`\n\nThis baseline file was **added** in the merged PR.") + append_section( + f"## API review baseline changes for `{filename}`\n\n" + 'This baseline file was **added** in the selected PR.') elif old_exists: - sections.append(f"### `{filename}`\n\nThis baseline file was **removed** in the merged PR.") + append_section( + f"## API review baseline changes for `{filename}`\n\n" + 'This baseline file was **removed** in the selected PR.') - if not sections: - sections.append('No API deltas were produced for the modified baseline files.') + if not current_body: + append_section( + '## API review baseline changes\n\n' + 'No API deltas were produced for the modified baseline files.') - body = ( - '\n' - '## API review baseline changes\n\n' - 'This merged PR modified one or more `*.baseline.json` files. ' - 'The deltas below were generated by `ApiChief` between the pre-merge and merged versions.\n\n' - + '\n\n'.join(sections) - ) + if current_body: + comment_bodies.append(current_body) - comment_path = workdir / 'comment.md' - comment_path.write_text(body[:65000], encoding='utf-8') + comments_dir = workdir / 'comments' + comments_dir.mkdir(parents=True, exist_ok=True) + + for index, body in enumerate(comment_bodies, start=1): + comment_path = comments_dir / f'{index:03}.md' + comment_path.write_text(body.rstrip() + '\n', encoding='utf-8') with open(os.environ['GITHUB_OUTPUT'], 'a', encoding='utf-8') as output: - output.write(f'comment_path={comment_path}\n') + output.write(f'comments_dir={comments_dir}\n') PY - - name: Upsert PR comment with delta + - name: Create PR comment with diffs if: steps.detect.outputs.has_baselines == 'true' - uses: actions/github-script@v8 + uses: actions/github-script@v9 env: - COMMENT_PATH: ${{ steps.delta.outputs.comment_path }} + COMMENTS_DIR: ${{ steps.delta.outputs.comments_dir }} + PR_NUMBER: ${{ steps.detect.outputs.pr_number }} with: script: | const fs = require('fs'); + const path = require('path'); const owner = context.repo.owner; const repo = context.repo.repo; - const issue_number = context.payload.pull_request.number; - const marker = ''; - const body = fs.readFileSync(process.env.COMMENT_PATH, 'utf8'); + const issue_number = Number(process.env.PR_NUMBER); + const commentsDir = process.env.COMMENTS_DIR; - const comments = await github.paginate(github.rest.issues.listComments, { - owner, - repo, - issue_number, - per_page: 100 - }); + const commentFiles = fs.readdirSync(commentsDir) + .filter(file => file.endsWith('.md')) + .sort((a, b) => a.localeCompare(b)); - const existing = comments.find(comment => comment.body?.includes(marker)); - - if (existing) { - await github.rest.issues.updateComment({ - owner, - repo, - comment_id: existing.id, - body - }); - console.log(`Updated existing API review comment (${existing.id}).`); - } else { + for (const file of commentFiles) { + const body = fs.readFileSync(path.join(commentsDir, file), 'utf8'); await github.rest.issues.createComment({ owner, repo, issue_number, body }); - console.log('Created new API review comment.'); } + + console.log(`Created ${commentFiles.length} API review comment(s) for PR #${issue_number}.`); diff --git a/.github/workflows/issues-closed.yml b/.github/workflows/issues-closed.yml index 2d8d81e3e78..de01fd7c0f3 100644 --- a/.github/workflows/issues-closed.yml +++ b/.github/workflows/issues-closed.yml @@ -16,7 +16,7 @@ jobs: runs-on: ubuntu-slim steps: - name: Reclose as not planned if external user - uses: actions/github-script@v8 + uses: actions/github-script@v9 with: script: | const issue = context.payload.issue; diff --git a/.github/workflows/label-and-milestone-issues.yml b/.github/workflows/label-and-milestone-issues.yml index 7a5f30005ff..f1c98b36754 100644 --- a/.github/workflows/label-and-milestone-issues.yml +++ b/.github/workflows/label-and-milestone-issues.yml @@ -22,7 +22,7 @@ jobs: runs-on: ubuntu-slim steps: - name: Label issues and update milestones - uses: actions/github-script@v8 + uses: actions/github-script@v9 with: script: | const owner = context.repo.owner; diff --git a/.github/workflows/validate-pr-target-branch.yml b/.github/workflows/validate-pr-target-branch.yml index 5cc4b2b4bbb..c8380e1381c 100644 --- a/.github/workflows/validate-pr-target-branch.yml +++ b/.github/workflows/validate-pr-target-branch.yml @@ -19,7 +19,7 @@ jobs: runs-on: ubuntu-slim steps: - name: Check PR target branch and author permissions - uses: actions/github-script@v8 + uses: actions/github-script@v9 with: script: | const pr = context.payload.pull_request; diff --git a/.gitignore b/.gitignore index 55ebf0a8e2d..c64dc0e4c07 100644 --- a/.gitignore +++ b/.gitignore @@ -218,6 +218,9 @@ _pkginfo.txt # but keep track of directories ending in .cache !*.[Cc]ache/ +# VS Code cache files +*.lscache + # Others ClientBin/ ~$* diff --git a/eng/Tools/ApiChief/Commands/EmitDelta.cs b/eng/Tools/ApiChief/Commands/EmitDelta.cs index f2354555598..eb4a031bc82 100644 --- a/eng/Tools/ApiChief/Commands/EmitDelta.cs +++ b/eng/Tools/ApiChief/Commands/EmitDelta.cs @@ -2,8 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using System.CommandLine; using System.IO; +using System.Linq; using System.Threading.Tasks; using ApiChief.Model; @@ -11,6 +13,8 @@ namespace ApiChief.Commands; internal static class EmitDelta { + private static readonly HashSet InvalidFileNameChars = [..Path.GetInvalidFileNameChars(), ',']; + public const int NoChangesExitCode = 2; private sealed class EmitDeltaArgs @@ -20,6 +24,8 @@ private sealed class EmitDeltaArgs public string BaselinePath { get; set; } = string.Empty; public string? Output { get; set; } + + public bool Diff { get; set; } } public static Command Create(Argument assemblyPathArgument) @@ -31,13 +37,19 @@ public static Command Create(Argument assemblyPathArgument) var outputOption = new Option("--output", ["-o"]) { - Description = "Path of the delta file to produce" + Description = "Path of the delta file or diff directory to produce" + }; + + var diffOption = new Option("--diff") + { + Description = "Emit GitHub-friendly markdown diff files instead of JSON delta output" }; var command = new Command("delta", "Creates an API delta") { baselinePathArgument, outputOption, + diffOption, }; command.SetAction(parseResult => ExecuteAsync(new EmitDeltaArgs @@ -45,6 +57,7 @@ public static Command Create(Argument assemblyPathArgument) AssemblyPath = parseResult.GetValue(assemblyPathArgument), BaselinePath = parseResult.GetValue(baselinePathArgument) ?? string.Empty, Output = parseResult.GetValue(outputOption), + Diff = parseResult.GetValue(diffOption), })); return command; @@ -53,32 +66,18 @@ public static Command Create(Argument assemblyPathArgument) private static async Task ExecuteAsync(EmitDeltaArgs args) { - ApiModel current; - ApiModel baseline; - - try - { - current = LoadCurrentModel(args.AssemblyPath!.FullName); - } - catch (Exception ex) + if (args.Diff) { - Console.Error.WriteLine($"Unable to load current API model from '{args.AssemblyPath!.FullName}': {ex.Message}"); - return -1; + return await ExecuteDiffAsync(args).ConfigureAwait(false); } - try - { - baseline = ApiModel.LoadFromFile(args.BaselinePath); - } - catch (Exception ex) + var exitCode = TryCreateDeltaModel(args.AssemblyPath!.FullName, args.BaselinePath, out var current); + if (exitCode != 0 && exitCode != NoChangesExitCode) { - Console.Error.WriteLine($"Unable to load baseline report '{args.BaselinePath}': {ex.Message}"); - return -1; + return exitCode; } - baseline.EvaluateDelta(current); - - var result = current.ToString(); + var result = current!.ToString(); var hasChanges = current.Types.Count > 0; if (args.Output == null) @@ -101,6 +100,310 @@ private static async Task ExecuteAsync(EmitDeltaArgs args) return hasChanges ? 0 : NoChangesExitCode; } + private static async Task ExecuteDiffAsync(EmitDeltaArgs args) + { + var exitCode = TryCreateDeltaModel(args.AssemblyPath!.FullName, args.BaselinePath, out var deltaModel); + if (exitCode != 0) + { + return exitCode; + } + + var output = args.Output; + if (string.IsNullOrWhiteSpace(output)) + { + var name = Path.GetFileNameWithoutExtension(args.AssemblyPath!.Name); + output = $"API.{name}.Diff"; + } + + var names = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var type in deltaModel!.Types.OrderBy(t => t.Type, StringComparer.Ordinal)) + { + var file = GetOutputFileStem(type.Type); + + var tmp = file; + if (names.Contains(tmp)) + { + var count = 2; + while (names.Contains(tmp)) + { + tmp = file + "_" + count++; + } + } + + names.Add(tmp); + var fullFile = Path.Combine(output, tmp + ".md"); + + try + { + Directory.CreateDirectory(output); + } + catch (Exception ex) + { + Console.Error.WriteLine($"Unable to create output directory '{output}': {ex.Message}"); + return -1; + } + + try + { + await File.WriteAllTextAsync(fullFile, FormatDiffMarkdown(type)).ConfigureAwait(false); + } + catch (Exception ex) + { + Console.Error.WriteLine($"Unable to write output delta diff file '{fullFile}': {ex.Message}"); + return -1; + } + } + + return 0; + } + + private static string FormatDiffMarkdown(ApiType type) + { + List lines = []; + + var typeAdded = type.Additions != null && type.Removals == null; + var typeRemoved = type.Removals != null && type.Additions == null; + + if (typeRemoved) + { + lines.Add($"- {type.Type}"); + } + else if (typeAdded) + { + lines.Add($"+ {type.Type}"); + } + + AppendStageDiffLine(lines, type.Removals, '-'); + AppendStageDiffLine(lines, type.Additions, '+'); + AppendGroupedDiffMembers(lines, type.Removals, type.Additions); + + return $"### `{type.Type}`{Environment.NewLine}{Environment.NewLine}```diff{Environment.NewLine}{string.Join(Environment.NewLine, lines)}{Environment.NewLine}```{Environment.NewLine}"; + } + + private static void AppendStageDiffLine(List lines, ApiType? changeSet, char prefix) + { + if (changeSet?.Stage != null && changeSet.Stage != ApiStage.Stable) + { + lines.Add($"{prefix} [Stage] {changeSet.Stage}"); + } + } + + private static void AppendGroupedDiffMembers(List lines, ApiType? removals, ApiType? additions) + { + var removedEntries = GetDiffEntries(removals, '-'); + var addedEntries = GetDiffEntries(additions, '+'); + + var sharedNames = removedEntries + .Select(static entry => entry.Name) + .Intersect(addedEntries.Select(static entry => entry.Name), StringComparer.Ordinal) + .ToHashSet(StringComparer.Ordinal); + + foreach (var name in sharedNames.OrderBy(static name => name, StringComparer.Ordinal)) + { + foreach (var entry in removedEntries.Where(entry => entry.Name == name)) + { + lines.Add(entry.Line); + } + + foreach (var entry in addedEntries.Where(entry => entry.Name == name)) + { + lines.Add(entry.Line); + } + } + + foreach (var entry in removedEntries.Where(entry => !sharedNames.Contains(entry.Name))) + { + lines.Add(entry.Line); + } + + foreach (var entry in addedEntries.Where(entry => !sharedNames.Contains(entry.Name))) + { + lines.Add(entry.Line); + } + } + + private static List<(string Name, string Line)> GetDiffEntries(ApiType? changeSet, char prefix) + { + List<(string Name, string Line)> entries = []; + + if (changeSet == null) + { + return entries; + } + + AddDiffMembers(entries, changeSet.Fields, prefix); + AddDiffMembers(entries, changeSet.Properties, prefix); + AddDiffMembers(entries, changeSet.Methods, prefix); + + return entries; + } + + private static void AddDiffMembers(List<(string Name, string Line)> entries, ISet? members, char prefix) + { + if (members == null) + { + return; + } + + foreach (var member in members.OrderBy(m => GetMemberName(m.Member), StringComparer.Ordinal).ThenBy(m => m.Member, StringComparer.Ordinal)) + { + entries.Add((GetMemberName(member.Member), $"{prefix} {member.Member}")); + } + } + + private static string GetMemberName(string declaration) + { + var headerEnd = declaration.IndexOf(" {", StringComparison.Ordinal); + var parameterListStart = declaration.IndexOf('('); + + if (headerEnd < 0 || (parameterListStart >= 0 && parameterListStart < headerEnd)) + { + headerEnd = parameterListStart; + } + + if (headerEnd < 0) + { + headerEnd = declaration.Length; + } + + var header = declaration[..headerEnd].Trim(); + var lastDot = header.LastIndexOf('.'); + if (lastDot >= 0) + { + return header[(lastDot + 1)..]; + } + + var lastSpace = header.LastIndexOf(' '); + return lastSpace >= 0 ? header[(lastSpace + 1)..] : header; + } + + private static string GetOutputFileStem(string typeDeclaration) + => SanitizeFileName(StripTypeNameDecorations(typeDeclaration)); + + private static string StripTypeNameDecorations(string typeDeclaration) + { + var typeName = RemoveBracketedSections(typeDeclaration).Trim(); + + var removedPrefix = true; + while (removedPrefix) + { + removedPrefix = false; + + foreach (var prefix in new[] + { + "abstract ", "sealed ", "static ", "readonly ", "ref ", "partial ", + "record ", "class ", "struct ", "interface ", "enum ", "delegate " + }) + { + if (!typeName.StartsWith(prefix, StringComparison.Ordinal)) + { + continue; + } + + typeName = typeName[prefix.Length..].TrimStart(); + removedPrefix = true; + break; + } + } + + var whereIndex = typeName.IndexOf(" where ", StringComparison.Ordinal); + if (whereIndex >= 0) + { + typeName = typeName[..whereIndex]; + } + + var terminator = typeName.IndexOfAny([':', '(']); + if (terminator >= 0) + { + typeName = typeName[..terminator]; + } + + return typeName.Trim(); + } + + private static string RemoveBracketedSections(string value) + { + var buffer = new char[value.Length]; + var index = 0; + var depth = 0; + + foreach (var c in value) + { + switch (c) + { + case '[': + depth++; + continue; + case ']' when depth > 0: + depth--; + continue; + default: + if (depth == 0) + { + buffer[index++] = c; + } + + break; + } + } + + return new string(buffer, 0, index); + } + + private static string SanitizeFileName(string value) + { + var trimmed = value.Trim(); + if (trimmed.Length == 0) + { + return "_"; + } + + var buffer = new char[trimmed.Length]; + var current = 0; + foreach (var c in trimmed) + { + if (c == ' ') + { + continue; + } + + buffer[current++] = InvalidFileNameChars.Contains(c) ? '_' : c; + } + + return new string(buffer, 0, current); + } + + private static int TryCreateDeltaModel(string currentPath, string baselinePath, out ApiModel? current) + { + ApiModel baseline; + + try + { + current = LoadCurrentModel(currentPath); + } + catch (Exception ex) + { + Console.Error.WriteLine($"Unable to load current API model from '{currentPath}': {ex.Message}"); + current = null; + return -1; + } + + try + { + baseline = ApiModel.LoadFromFile(baselinePath); + } + catch (Exception ex) + { + Console.Error.WriteLine($"Unable to load baseline report '{baselinePath}': {ex.Message}"); + current = null; + return -1; + } + + baseline.EvaluateDelta(current); + + return current.Types.Count > 0 ? 0 : NoChangesExitCode; + } + private static ApiModel LoadCurrentModel(string path) => Path.GetExtension(path).Equals(".json", StringComparison.OrdinalIgnoreCase) ? ApiModel.LoadFromFile(path) diff --git a/eng/Tools/ApiChief/Model/ApiModel.cs b/eng/Tools/ApiChief/Model/ApiModel.cs index 060a2644338..15b67a3d6bb 100644 --- a/eng/Tools/ApiChief/Model/ApiModel.cs +++ b/eng/Tools/ApiChief/Model/ApiModel.cs @@ -44,8 +44,8 @@ public static ApiModel LoadFromAssembly(string path) public static ApiModel LoadFromFile(string path) => JsonSerializer.Deserialize(File.ReadAllText(path), _serializerOptions)!; - public void EvaluateDelta(ApiModel current) - => current.Types = FindChanges(this, current); + public void EvaluateDelta(ApiModel current, bool includeSharedMembers = false) + => current.Types = FindChanges(this, current, includeSharedMembers); public bool HasRemovals() => Types.Any(static type => type.Removals != null); @@ -53,7 +53,7 @@ public bool HasRemovals() public override string ToString() => JsonSerializer.Serialize(this, _serializerOptions).ReplaceLineEndings(Environment.NewLine); - private static ISet FindChanges(ApiModel baseline, ApiModel current) + private static ISet FindChanges(ApiModel baseline, ApiModel current, bool includeSharedMembers) { ISet result = new HashSet(); @@ -66,7 +66,7 @@ private static ISet FindChanges(ApiModel baseline, ApiModel current) continue; } - var typeDelta = CreateTypeDelta(currentType, currentType, baselineType, includeSharedMembers: true); + var typeDelta = CreateTypeDelta(currentType, currentType, baselineType, includeSharedMembers); if (typeDelta != null) { result.Add(typeDelta); diff --git a/eng/Tools/ApiChief/Program.cs b/eng/Tools/ApiChief/Program.cs index 24e36985eb2..1baa6e4eab1 100644 --- a/eng/Tools/ApiChief/Program.cs +++ b/eng/Tools/ApiChief/Program.cs @@ -14,7 +14,7 @@ public static Task Main(string[] args) { var assemblyPathArgument = new Argument("assembly-path") { - Description = "Path to the assembly to work with." + Description = "Path to the assembly or baseline file to work with." }; var rootCommand = new RootCommand("Helps with .NET API management activities") diff --git a/src/EFCore.Relational/Migrations/HistoryRepository.cs b/src/EFCore.Relational/Migrations/HistoryRepository.cs index 48934123be0..714797d9fea 100644 --- a/src/EFCore.Relational/Migrations/HistoryRepository.cs +++ b/src/EFCore.Relational/Migrations/HistoryRepository.cs @@ -85,7 +85,14 @@ protected virtual string MigrationIdColumnName .FindProperty(nameof(HistoryRow.MigrationId))! .GetColumnName(); - private IModel EnsureModel() + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + [EntityFrameworkInternal] + protected virtual IModel EnsureModel() { if (_model == null) { diff --git a/src/EFCore.SqlServer/Migrations/Internal/SqlServerHistoryRepository.cs b/src/EFCore.SqlServer/Migrations/Internal/SqlServerHistoryRepository.cs index e8128cd0ef5..6189774c621 100644 --- a/src/EFCore.SqlServer/Migrations/Internal/SqlServerHistoryRepository.cs +++ b/src/EFCore.SqlServer/Migrations/Internal/SqlServerHistoryRepository.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Text; +using Microsoft.EntityFrameworkCore.SqlServer.Metadata.Internal; namespace Microsoft.EntityFrameworkCore.SqlServer.Migrations.Internal; @@ -24,6 +25,36 @@ public SqlServerHistoryRepository(HistoryRepositoryDependencies dependencies) { } + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override IReadOnlyList GetCreateCommands() + { + // TODO: This is a hack around https://github.com/dotnet/efcore/issues/34991: provider-specific conventions may add + // database-level annotations (e.g. full-text catalogs) to the model, and the default EF logic causes them to be created + // at this point, when the history table is being created. This is too early, and causes the later actual migration to fail. + // So we filter out full-text catalog annotations from AlterDatabaseOperation. + // This follows the same approach as the Npgsql provider (npgsql/efcore.pg#3713). +#pragma warning disable EF1001 // Internal EF Core API usage. + var model = EnsureModel(); +#pragma warning restore EF1001 // Internal EF Core API usage. + + var operations = Dependencies.ModelDiffer.GetDifferences(null, model.GetRelationalModel()); + + foreach (var operation in operations) + { + if (operation is AlterDatabaseOperation alterDatabaseOperation) + { + alterDatabaseOperation.RemoveAnnotation(SqlServerAnnotationNames.FullTextCatalogs); + } + } + + return Dependencies.MigrationsSqlGenerator.Generate(operations, model); + } + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/test/EFCore.SqlServer.Tests/Migrations/SqlServerHistoryRepositoryTest.cs b/test/EFCore.SqlServer.Tests/Migrations/SqlServerHistoryRepositoryTest.cs index a19a9c536f1..273953819b5 100644 --- a/test/EFCore.SqlServer.Tests/Migrations/SqlServerHistoryRepositoryTest.cs +++ b/test/EFCore.SqlServer.Tests/Migrations/SqlServerHistoryRepositoryTest.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using Microsoft.Data.SqlClient; +using Microsoft.EntityFrameworkCore.SqlServer.Metadata.Internal; // ReSharper disable InconsistentNaming namespace Microsoft.EntityFrameworkCore.Migrations; @@ -21,6 +22,25 @@ [ProductVersion] nvarchar(32) NOT NULL, CONSTRAINT [PK___EFMigrationsHistory] PRIMARY KEY ([MigrationId]) ); +""", sql, ignoreLineEndingDifferences: true); + } + + [ConditionalFact] + public void GetCreateScript_works_with_full_text_catalog() + { + // Inject a model finalizing convention that adds a full-text catalog to the model, simulating a scenario where + // provider conventions add database-level annotations. Without filtering, the history table creation script would + // include CREATE FULLTEXT CATALOG. + var sql = CreateHistoryRepository(addFullTextCatalogConvention: true).GetCreateScript(); + + Assert.Equal( + """ +CREATE TABLE [__EFMigrationsHistory] ( + [MigrationId] nvarchar(150) NOT NULL, + [ProductVersion] nvarchar(32) NOT NULL, + CONSTRAINT [PK___EFMigrationsHistory] PRIMARY KEY ([MigrationId]) +); + """, sql, ignoreLineEndingDifferences: true); } @@ -149,17 +169,28 @@ public void GetEndIfScript_works() """, sql, ignoreLineEndingDifferences: true); } - private static IHistoryRepository CreateHistoryRepository(string schema = null) - => new TestDbContext( + private static IHistoryRepository CreateHistoryRepository( + string schema = null, + Action configureModel = null, + bool addFullTextCatalogConvention = false) + { + var serviceProvider = addFullTextCatalogConvention + ? SqlServerTestHelpers.Instance.CreateServiceProvider( + new ServiceCollection().AddSingleton()) + : SqlServerTestHelpers.Instance.CreateServiceProvider(); + + return new TestDbContext( new DbContextOptionsBuilder() - .UseInternalServiceProvider(SqlServerTestHelpers.Instance.CreateServiceProvider()) + .UseInternalServiceProvider(serviceProvider) .UseSqlServer( new SqlConnection("Database=DummyDatabase"), b => b.MigrationsHistoryTable(HistoryRepository.DefaultTableName, schema)) - .Options) + .Options, + configureModel) .GetService(); + } - private class TestDbContext(DbContextOptions options) : DbContext(options) + private class TestDbContext(DbContextOptions options, Action configureModel = null) : DbContext(options) { public DbSet Blogs { get; set; } @@ -169,9 +200,35 @@ public IQueryable TableFunction() protected override void OnModelCreating(ModelBuilder modelBuilder) { + configureModel?.Invoke(modelBuilder); + } + } + + /// + /// A convention plugin that adds a full-text catalog annotation to the model, simulating what a provider convention + /// might do. This allows testing that properly filters + /// out the full-text catalog from the history table creation script. + /// + private class FullTextCatalogConventionPlugin : IConventionSetPlugin + { + public ConventionSet ModifyConventions(ConventionSet conventionSet) + { + conventionSet.ModelFinalizingConventions.Add(new FullTextCatalogAddingConvention()); + return conventionSet; } } + private class FullTextCatalogAddingConvention : IModelFinalizingConvention + { +#pragma warning disable EF1001 // Internal EF Core API usage. + public void ProcessModelFinalizing( + IConventionModelBuilder modelBuilder, + IConventionContext context) + => SqlServerFullTextCatalog.AddFullTextCatalog( + (IMutableModel)modelBuilder.Metadata, "TestCatalog", ConfigurationSource.Convention); +#pragma warning restore EF1001 // Internal EF Core API usage. + } + private class Blog { public int Id { get; set; }