Refactor OpenApiDocumentFactory into separate modules#1144
Merged
Conversation
…mentLoader modules from OpenApiDocumentFactory DocumentEquivalenceComparer: canonical JSON equivalence comparison for merge conflict detection. Extracted from OpenApiDocumentFactory with public methods for testability. IDocumentMerger + DocumentMerger: multi-document merge logic (paths, schemas, definitions, security schemes, tags). Depends on DocumentEquivalenceComparer for conflict detection. IDocumentLoader + DocumentLoader: document loading from file or HTTP URL with YAML/JSON detection, OpenApiMultiFileReader for external references, and NSwag fallback. OpenApiDocumentFactory: deepened to thin composition of IDocumentLoader + IDocumentMerger. Public API unchanged.
Replace reflection-based InvokeMerge, InvokeAreEquivalent, InvokeCreateCanonicalSchemaToken, InvokeCreateCanonicalJsonToken, InvokeAddReferencedSchemas, InvokeGetDefinitionName, InvokeCreateOpenApiJson, and InvokeRemoveNullProperties with direct calls to DocumentMerger and DocumentEquivalenceComparer. InvokeMergeIfMissingOrThrowOnConflict continues to use reflection on DocumentMerger for targeted merge-conflict tests. All 2159 tests pass.
christianhelle
added
enhancement
christianhelle
added
enhancement
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughExtracts all OpenAPI document loading and merging logic out of ChangesOpenAPI Document Loading and Merging Decomposition
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Refitter.Core/DocumentEquivalenceComparer.cs`:
- Around line 11-154: Add XML documentation comments to all public methods in
the DocumentEquivalenceComparer class. For each public method (AreEquivalent,
CreateCanonicalJsonToken, NormalizeJsonToken, CreateCanonicalSchemaToken,
RemoveNullProperties, CreateOpenApiJson, AddReferencedSchemas, and
GetDefinitionName), add a summary comment block that describes what the method
does, documents all parameters with param tags, and documents the return value
with a returns tag. This ensures compliance with the C# API documentation
requirement.
- Around line 76-79: The canonicalization process is order-sensitive for
unordered schema constructs like allOf, oneOf, anyOf, and enum, which means
semantically equivalent schemas with different declaration order can be treated
as conflicts. Sort these arrays before adding them to the JSON structure in the
AddSchemaArray method calls and wherever enum values are being processed to
ensure consistent, order-insensitive canonicalization regardless of the input
declaration order.
In `@src/Refitter.Core/DocumentLoader.cs`:
- Around line 28-30: The LoadAsync method accepts the openApiPath parameter
without validating it for null or blank values, allowing invalid input to
propagate to downstream helper methods and cause unclear runtime errors. Add
explicit input validation at the beginning of the LoadAsync method to check if
openApiPath is null, empty, or contains only whitespace, and throw an
ArgumentException with a descriptive message if validation fails. This will
catch the issue at the boundary and provide clear, actionable error messages
instead of failures in the helpers that consume this parameter.
- Around line 99-103: The IsYaml method performs suffix checks on the raw input
path without accounting for query strings or URL fragments. Paths like
`.../openapi.yaml?raw=1` fail the EndsWith check and are misclassified as JSON.
Modify the IsYaml method to first extract the base path by removing query string
parameters (everything from the '?' character onward) and URL fragments
(everything from the '#' character onward), then perform the EndsWith check for
"yaml" and "yml" extensions on this cleaned path portion.
- Around line 48-51: The catch block for Exception is too broad and captures
cancellation-related exceptions like OperationCanceledException and
TaskCanceledException, forcing a fallback attempt that contradicts cancellation
semantics. Modify the catch block to be more specific about which exceptions
should trigger the fallback to CreateUsingNSwagAsync. Either catch only the
specific exception types that warrant a fallback behavior, or add logic to
re-throw cancellation exceptions before attempting the CreateUsingNSwagAsync
fallback.
In `@src/Refitter.Core/DocumentMerger.cs`:
- Around line 12-17: Add XML documentation comments to the public members of the
DocumentMerger class that are currently undocumented. Specifically, add a
summary comment above the DocumentMerger constructor that explains its purpose
and the comparer parameter, and add a summary comment above the Merge method
that describes what it does, its parameter, and return value. Follow standard C#
XML documentation format with <summary>, <param>, and <returns> tags as
appropriate.
- Around line 17-20: The Merge method accesses documents[0] without validating
that the documents array is not null or empty, which will result in an unhelpful
IndexOutOfRangeException at runtime. Add a guard clause at the beginning of the
Merge method to check if the documents parameter is null or empty, and if so
throw an ArgumentException or similar with a clear error message explaining that
at least one document is required. This validation should occur before any
attempt to access documents[0] or call CloneDocument.
- Around line 30-35: Add null checks for the Components property before
accessing its Schemas collection in the DocumentMerger.cs file. Currently, both
document.Components and baseDocument.Components are accessed without null
validation, which can cause NullReferenceException when parsing Swagger 2.0
documents where the "components" section is missing. Wrap the existing
conditional block that checks if document.Components.Schemas.Count is greater
than zero with an additional null check for document.Components itself, and
similarly ensure baseDocument.Components is not null before accessing
baseDocument.Components.Schemas in the MergeIfMissingOrThrowOnConflict call.
This should mirror the same defensive null-checking pattern already applied to
Definitions and SecurityDefinitions handling in the same method.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3c633d0-5731-49bd-9c31-5e779bfc937c
📒 Files selected for processing (7)
src/Refitter.Core/DocumentEquivalenceComparer.cssrc/Refitter.Core/DocumentLoader.cssrc/Refitter.Core/DocumentMerger.cssrc/Refitter.Core/IDocumentLoader.cssrc/Refitter.Core/IDocumentMerger.cssrc/Refitter.Core/OpenApiDocumentFactory.cssrc/Refitter.Tests/OpenApi/OpenApiDocumentFactoryMergeTests.cs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1144 +/- ##
==========================================
- Coverage 94.31% 94.14% -0.18%
==========================================
Files 48 55 +7
Lines 2796 2868 +72
==========================================
+ Hits 2637 2700 +63
- Misses 53 58 +5
- Partials 106 110 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
christianhelle
changed the title
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 8 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 8 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Split the 488-line OpenApiDocumentFactory into 4 independently testable modules, as specified in docs/prd/prd-deepen-openapi-document-factory.md.
Changes
New modules
Modified
Summary by CodeRabbit
Summary