refactor: extract CSharpClientGeneratorFactory schema mutations into IOpenApiDocumentMutator pipeline#1135
Closed
christianhelle wants to merge 5 commits into
Closed
refactor: extract CSharpClientGeneratorFactory schema mutations into IOpenApiDocumentMutator pipeline#1135christianhelle wants to merge 5 commits into
christianhelle wants to merge 5 commits into
Conversation
…e and explicit property copy
…ymorphic serialization
Contributor
📝 WalkthroughWalkthroughThe PR restructures OpenAPI document preprocessing in ChangesMutator Pipeline Architecture
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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: 1
🧹 Nitpick comments (5)
src/Refitter.Core/SchemaWalker.cs (1)
37-107: ⚖️ Poor tradeoffConsider extracting nested enumeration logic into helper methods.
SonarCloud flags this method with Cognitive Complexity 44 (threshold 15). While the logic clearly mirrors the OpenAPI document structure, you could reduce complexity by extracting sub-enumerations:
EnumerateComponentSchemasEnumeratePathItemSchemasEnumerateOperationSchemasThis is optional; the current implementation is readable and all tests pass.
🤖 Prompt for 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. In `@src/Refitter.Core/SchemaWalker.cs` around lines 37 - 107, The EnumerateDocumentSchemaRoots method has high cognitive complexity due to nested loops and conditionals; refactor by extracting the nested enumeration blocks into three helper iterator methods: EnumerateComponentSchemas(OpenApiDocument), EnumeratePathItemSchemas(PathItem), and EnumerateOperationSchemas(OpenApiOperation) (or similar names) and have EnumerateDocumentSchemaRoots call them; move the logic that yields document.Components.Schemas into EnumerateComponentSchemas, the per-path logic (including yielding pathItem.Parameters and iterating pathItem.Values) into EnumeratePathItemSchemas which calls EnumerateOperationSchemas for each operation, and move the per-operation logic (yielding operation.ActualParameters, requestBody content schemas, response headers and response content schemas) into EnumerateOperationSchemas so the main method simply iterates document.Paths and delegates to these helpers, preserving all yielded types and null checks.Source: Linters/SAST tools
src/Refitter.Core/OneOfDiscriminatorToAllOfMutator.cs (1)
8-47: ⚖️ Poor tradeoffConsider extracting helper methods to reduce cognitive complexity.
The method handles multiple responsibilities: finding discriminated unions, validating schema types, converting union members to inheritance, and cleaning up. Extracting helpers such as
ShouldTransformSchema,AddInheritanceToSubSchema, andClearUnionCollectionswould improve readability and align with the 15-complexity threshold flagged by SonarCloud.♻️ Example refactoring
public void Mutate(OpenApiDocument document) { if (document.Components?.Schemas == null) return; foreach (var kvp in document.Components.Schemas) { var schema = kvp.Value?.ActualSchema; if (schema == null) continue; - if (schema.DiscriminatorObject == null) - continue; - - var unionSchemas = schema.OneOf.Concat(schema.AnyOf).ToArray(); - if (unionSchemas.Length == 0) + if (!ShouldTransformSchema(schema, out var unionSchemas)) continue; if (schema.Type == JsonObjectType.None || schema.Type == JsonObjectType.Null) schema.Type = JsonObjectType.Object; - foreach (var subSchemaRef in unionSchemas) - { - var subSchema = subSchemaRef?.ActualSchema; - if (subSchema == null) - continue; - - bool alreadyInherits = subSchema.AllOf.Any( - a => a.HasReference && a.ActualSchema == schema); - if (!alreadyInherits) - { - var reference = new JsonSchema { Reference = schema }; - subSchema.AllOf.Add(reference); - } - } + foreach (var subSchemaRef in unionSchemas) + AddInheritanceToSubSchema(subSchemaRef, schema); schema.OneOf.Clear(); schema.AnyOf.Clear(); } } +private static bool ShouldTransformSchema(JsonSchema schema, out JsonSchema[] unionSchemas) +{ + unionSchemas = []; + if (schema.DiscriminatorObject == null) + return false; + + unionSchemas = schema.OneOf.Concat(schema.AnyOf).ToArray(); + return unionSchemas.Length > 0; +} + +private static void AddInheritanceToSubSchema(JsonSchema? subSchemaRef, JsonSchema parentSchema) +{ + var subSchema = subSchemaRef?.ActualSchema; + if (subSchema == null) + return; + + bool alreadyInherits = subSchema.AllOf.Any( + a => a.HasReference && a.ActualSchema == parentSchema); + if (!alreadyInherits) + { + var reference = new JsonSchema { Reference = parentSchema }; + subSchema.AllOf.Add(reference); + } +}🤖 Prompt for 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. In `@src/Refitter.Core/OneOfDiscriminatorToAllOfMutator.cs` around lines 8 - 47, The Mutate method in OneOfDiscriminatorToAllOfMutator is too complex; extract small helpers to make intent explicit and reduce cognitive complexity: create ShouldTransformSchema(JsonSchema schema) to encapsulate the early checks (schema != null, schema.DiscriminatorObject != null, unionSchemas.Length>0 and ensure schema.Type is set to JsonObjectType.Object when needed), create AddInheritanceToSubSchema(JsonSchema parent, JsonSchema subSchema) to check subSchema.AllOf for existing reference to parent and add new JsonSchema { Reference = parent } when missing, and create ClearUnionCollections(JsonSchema schema) to Clear() schema.OneOf and schema.AnyOf; update Mutate to call these helpers and to use unionSchemas = schema.OneOf.Concat(schema.AnyOf).ToArray() and loop calling AddInheritanceToSubSchema for each subSchemaRef?.ActualSchema; keep all semantics (use ActualSchema, HasReference, AllOf) unchanged.Source: Linters/SAST tools
src/Refitter.Tests/CSharpClientGeneratorFactoryIntegrationTests.cs (3)
49-56: ⚡ Quick winManually constructing the mutator array duplicates factory logic.
The mutator construction here mirrors
CSharpClientGeneratorFactory.CreateDefaultMutators. If the factory's default mutator order or configuration changes, this test won't detect the drift.♻️ Consider letting the factory create default mutators
Remove the explicit mutators parameter to test the factory's actual default pipeline:
- var mutators = new IOpenApiDocumentMutator[] - { - new DisableAdditionalPropertiesMutator(settings.GenerateDefaultAdditionalProperties), - new OneOfDiscriminatorToAllOfMutator(), - new FixMissingIntegerTypesMutator(), - new CustomIntegerTypeMutator( - settings.CodeGeneratorSettings?.IntegerType ?? IntegerType.Int32), - }; - - var factory = new CSharpClientGeneratorFactory(settings, document, mutators); + var factory = new CSharpClientGeneratorFactory(settings, document);🤖 Prompt for 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. In `@src/Refitter.Tests/CSharpClientGeneratorFactoryIntegrationTests.cs` around lines 49 - 56, The test is duplicating factory logic by constructing the mutators array instead of using the factory's defaults; update the test in CSharpClientGeneratorFactoryIntegrationTests to stop manually creating that IOpenApiDocumentMutator[] and instead use the factory's default pipeline (either remove the explicit mutators parameter so the factory constructs defaults, or call CSharpClientGeneratorFactory.CreateDefaultMutators(settings) and pass that result) so the test exercises the actual default mutator order/configuration defined by CSharpClientGeneratorFactory.
13-65: ⚡ Quick winTest name suggests order verification but only checks namespace.
The method name
Create_AppliesMutatorsInOrder_BeforeBuildingGeneratorimplies that mutator application order is verified, yet the test only asserts the namespace was set. Consider adding assertions that confirm the mutations actually executed (e.g., verifyVehicle.OneOfis empty,Car.idtype is set,Car.countformat is "int64").🤖 Prompt for 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. In `@src/Refitter.Tests/CSharpClientGeneratorFactoryIntegrationTests.cs` around lines 13 - 65, The test Create_AppliesMutatorsInOrder_BeforeBuildingGenerator currently only checks the generator namespace but should also assert that the mutators actually ran: after constructing the factory and calling Create(), add FluentAssertions checks against the mutated OpenAPI document (the local document variable) to verify mutator effects—e.g., that document.Components.Schemas["Vehicle"].OneOf is null or empty (OneOf moved/removed), that document.Components.Schemas["Car"].Properties["id"] has the expected integer type/format set by FixMissingIntegerTypesMutator/CustomIntegerTypeMutator (format "int64" or IntegerType mapping), and that document.Components.Schemas["Car"].Properties["count"].Type/Format reflects "integer" with format "int64"; use the existing mutator names (DisableAdditionalPropertiesMutator, OneOfDiscriminatorToAllOfMutator, FixMissingIntegerTypesMutator, CustomIntegerTypeMutator) as guidance when locating where to assert these changes and use FluentAssertions in the same style as the existing assertions.
15-38: 💤 Low valueConsider extracting inline OpenAPI JSON strings.
The large inline JSON strings are repeated across tests with minor variations. Extracting them into helper methods or constants could improve maintainability and reduce duplication.
♻️ Example refactoring approach
private static string CreateMinimalOpenApiDocument() => """ { "openapi": "3.0.1", "info": { "title": "Test", "version": "1.0" }, "paths": {} } """; private static string CreateOpenApiDocumentWithSchema(string schemasJson) => $$""" { "openapi": "3.0.1", "info": { "title": "Test", "version": "1.0" }, "paths": {}, "components": { "schemas": {{schemasJson}} } } """;Also applies to: 70-87, 116-136, 157-163, 190-196
🤖 Prompt for 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. In `@src/Refitter.Tests/CSharpClientGeneratorFactoryIntegrationTests.cs` around lines 15 - 38, Extract the repeated inline OpenAPI JSON literals into private static helper methods in the CSharpClientGeneratorFactoryIntegrationTests test class (e.g., CreateMinimalOpenApiDocument() returning the base document string and CreateOpenApiDocumentWithSchema(string schemasJson) that injects schemas), then replace each large inline JSON block (the document variable creation and the other similar blocks noted in the review) with calls to these helpers, passing only the varying schema fragment (e.g., the "Vehicle"/"Car" schema JSON) so tests reuse the shared base document and reduce duplication.
🤖 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.Tests/CSharpClientGeneratorFactoryIntegrationTests.cs`:
- Line 155: Rename the test method
Create_WithCodeGeneratorSettings_AppliesExplictPropertyCopies to
Create_WithCodeGeneratorSettings_AppliesExplicitPropertyCopies to fix the typo;
update the method declaration and any references (test attributes, usages, or
string-based names in test runners) that refer to
Create_WithCodeGeneratorSettings_AppliesExplictPropertyCopies so they point to
Create_WithCodeGeneratorSettings_AppliesExplicitPropertyCopies.
---
Nitpick comments:
In `@src/Refitter.Core/OneOfDiscriminatorToAllOfMutator.cs`:
- Around line 8-47: The Mutate method in OneOfDiscriminatorToAllOfMutator is too
complex; extract small helpers to make intent explicit and reduce cognitive
complexity: create ShouldTransformSchema(JsonSchema schema) to encapsulate the
early checks (schema != null, schema.DiscriminatorObject != null,
unionSchemas.Length>0 and ensure schema.Type is set to JsonObjectType.Object
when needed), create AddInheritanceToSubSchema(JsonSchema parent, JsonSchema
subSchema) to check subSchema.AllOf for existing reference to parent and add new
JsonSchema { Reference = parent } when missing, and create
ClearUnionCollections(JsonSchema schema) to Clear() schema.OneOf and
schema.AnyOf; update Mutate to call these helpers and to use unionSchemas =
schema.OneOf.Concat(schema.AnyOf).ToArray() and loop calling
AddInheritanceToSubSchema for each subSchemaRef?.ActualSchema; keep all
semantics (use ActualSchema, HasReference, AllOf) unchanged.
In `@src/Refitter.Core/SchemaWalker.cs`:
- Around line 37-107: The EnumerateDocumentSchemaRoots method has high cognitive
complexity due to nested loops and conditionals; refactor by extracting the
nested enumeration blocks into three helper iterator methods:
EnumerateComponentSchemas(OpenApiDocument), EnumeratePathItemSchemas(PathItem),
and EnumerateOperationSchemas(OpenApiOperation) (or similar names) and have
EnumerateDocumentSchemaRoots call them; move the logic that yields
document.Components.Schemas into EnumerateComponentSchemas, the per-path logic
(including yielding pathItem.Parameters and iterating pathItem.Values) into
EnumeratePathItemSchemas which calls EnumerateOperationSchemas for each
operation, and move the per-operation logic (yielding
operation.ActualParameters, requestBody content schemas, response headers and
response content schemas) into EnumerateOperationSchemas so the main method
simply iterates document.Paths and delegates to these helpers, preserving all
yielded types and null checks.
In `@src/Refitter.Tests/CSharpClientGeneratorFactoryIntegrationTests.cs`:
- Around line 49-56: The test is duplicating factory logic by constructing the
mutators array instead of using the factory's defaults; update the test in
CSharpClientGeneratorFactoryIntegrationTests to stop manually creating that
IOpenApiDocumentMutator[] and instead use the factory's default pipeline (either
remove the explicit mutators parameter so the factory constructs defaults, or
call CSharpClientGeneratorFactory.CreateDefaultMutators(settings) and pass that
result) so the test exercises the actual default mutator order/configuration
defined by CSharpClientGeneratorFactory.
- Around line 13-65: The test
Create_AppliesMutatorsInOrder_BeforeBuildingGenerator currently only checks the
generator namespace but should also assert that the mutators actually ran: after
constructing the factory and calling Create(), add FluentAssertions checks
against the mutated OpenAPI document (the local document variable) to verify
mutator effects—e.g., that document.Components.Schemas["Vehicle"].OneOf is null
or empty (OneOf moved/removed), that
document.Components.Schemas["Car"].Properties["id"] has the expected integer
type/format set by FixMissingIntegerTypesMutator/CustomIntegerTypeMutator
(format "int64" or IntegerType mapping), and that
document.Components.Schemas["Car"].Properties["count"].Type/Format reflects
"integer" with format "int64"; use the existing mutator names
(DisableAdditionalPropertiesMutator, OneOfDiscriminatorToAllOfMutator,
FixMissingIntegerTypesMutator, CustomIntegerTypeMutator) as guidance when
locating where to assert these changes and use FluentAssertions in the same
style as the existing assertions.
- Around line 15-38: Extract the repeated inline OpenAPI JSON literals into
private static helper methods in the
CSharpClientGeneratorFactoryIntegrationTests test class (e.g.,
CreateMinimalOpenApiDocument() returning the base document string and
CreateOpenApiDocumentWithSchema(string schemasJson) that injects schemas), then
replace each large inline JSON block (the document variable creation and the
other similar blocks noted in the review) with calls to these helpers, passing
only the varying schema fragment (e.g., the "Vehicle"/"Car" schema JSON) so
tests reuse the shared base document and reduce duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c60947d6-cd49-4e00-b3f2-4c969e0aba36
📒 Files selected for processing (15)
src/Refitter.Core/CSharpClientGeneratorFactory.cssrc/Refitter.Core/CustomIntegerTypeMutator.cssrc/Refitter.Core/CustomTemplateFactory.cssrc/Refitter.Core/DisableAdditionalPropertiesMutator.cssrc/Refitter.Core/FixMissingIntegerTypesMutator.cssrc/Refitter.Core/IOpenApiDocumentMutator.cssrc/Refitter.Core/JsonSchemaReferenceComparer.cssrc/Refitter.Core/OneOfDiscriminatorToAllOfMutator.cssrc/Refitter.Core/SafeSchemaTypeNameGenerator.cssrc/Refitter.Core/SchemaWalker.cssrc/Refitter.Tests/CSharpClientGeneratorFactoryIntegrationTests.cssrc/Refitter.Tests/CustomIntegerTypeMutatorTests.cssrc/Refitter.Tests/DisableAdditionalPropertiesMutatorTests.cssrc/Refitter.Tests/FixMissingIntegerTypesMutatorTests.cssrc/Refitter.Tests/OneOfDiscriminatorToAllOfMutatorTests.cs
| } | ||
|
|
||
| [Test] | ||
| public async Task Create_WithCodeGeneratorSettings_AppliesExplictPropertyCopies() |
Contributor
There was a problem hiding this comment.
Typo in method name: "Explict" should be "Explicit".
The method name contains a spelling error that should be corrected for consistency and readability.
📝 Proposed fix
- public async Task Create_WithCodeGeneratorSettings_AppliesExplictPropertyCopies()
+ public async Task Create_WithCodeGeneratorSettings_AppliesExplicitPropertyCopies()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public async Task Create_WithCodeGeneratorSettings_AppliesExplictPropertyCopies() | |
| public async Task Create_WithCodeGeneratorSettings_AppliesExplicitPropertyCopies() |
🤖 Prompt for 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.
In `@src/Refitter.Tests/CSharpClientGeneratorFactoryIntegrationTests.cs` at line
155, Rename the test method
Create_WithCodeGeneratorSettings_AppliesExplictPropertyCopies to
Create_WithCodeGeneratorSettings_AppliesExplicitPropertyCopies to fix the typo;
update the method declaration and any references (test attributes, usages, or
string-based names in test runners) that refer to
Create_WithCodeGeneratorSettings_AppliesExplictPropertyCopies so they point to
Create_WithCodeGeneratorSettings_AppliesExplicitPropertyCopies.
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
Implements PRD #3: Decomposes four responsibilities fused into \CSharpClientGeneratorFactory\ into extracted \IOpenApiDocumentMutator\ adapters.
Changes
New files
Refactored
Testing
Summary by CodeRabbit
Release Notes
Refactor
New Features
Tests