Remove MvcOptions from SchemaGenerator #3242
Conversation
remove any usages of MvcOptions in 'SchemaGenerator'. It should not be a breaking change since bool expression is 'true' by definition if 'SuppressImplicitRequiredAttributeForNonNullableReferenceTypes=false'
| @@ -202,7 +202,6 @@ Swashbuckle.AspNetCore.SwaggerGen.SchemaFilterContext.Type.get -> System.Type | |||
| Swashbuckle.AspNetCore.SwaggerGen.SchemaGenerator | |||
| Swashbuckle.AspNetCore.SwaggerGen.SchemaGenerator.GenerateSchema(System.Type modelType, Swashbuckle.AspNetCore.SwaggerGen.SchemaRepository schemaRepository, System.Reflection.MemberInfo memberInfo = null, System.Reflection.ParameterInfo parameterInfo = null, Microsoft.AspNetCore.Mvc.ApiExplorer.ApiParameterRouteInfo routeInfo = null) -> Microsoft.OpenApi.Models.OpenApiSchema | |||
| Swashbuckle.AspNetCore.SwaggerGen.SchemaGenerator.SchemaGenerator(Swashbuckle.AspNetCore.SwaggerGen.SchemaGeneratorOptions generatorOptions, Swashbuckle.AspNetCore.SwaggerGen.ISerializerDataContractResolver serializerDataContractResolver) -> void | |||
| Swashbuckle.AspNetCore.SwaggerGen.SchemaGenerator.SchemaGenerator(Swashbuckle.AspNetCore.SwaggerGen.SchemaGeneratorOptions generatorOptions, Swashbuckle.AspNetCore.SwaggerGen.ISerializerDataContractResolver serializerDataContractResolver, Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Mvc.MvcOptions> mvcOptions) -> void | |||
There was a problem hiding this comment.
it seems that I'm breaking backward compatibility by removing this ctor since SchemaGenerator is a public class, but imho it will be misleading to accept 'MvcOptions' ctor parameter and not use it at all...
There was a problem hiding this comment.
Yep it is - the better approach would be to mark it as obsolete. We could then remove it in some future major version.
There was a problem hiding this comment.
got it, applied suggestion
| @@ -984,29 +984,6 @@ public void GenerateSchema_SupportsOption_NonNullableReferenceTypesAsRequired_Re | |||
| Assert.Equal(required, propertyIsRequired); | |||
| } | |||
|
|
|||
| [Theory] | |||
| [InlineData(typeof(TypeWithNullableContextAnnotated), nameof(TypeWithNullableContextAnnotated.SubTypeWithOneNonNullableContent), nameof(TypeWithNullableContextAnnotated.NonNullableString), false)] | |||
There was a problem hiding this comment.
I removed the test entirely since these InlineData test inputs are already present in tests above
There was a problem hiding this comment.
I would keep the tests to verify that the obsolete constructor behaves as expected (i.e. ignoring the MvcOptions value). Marking the test as obsolete would remove any warnings about obsolete usage, and then we'd delete those tests when the constructor is removed at a future date.
There was a problem hiding this comment.
are you OK if I revert tests in this file only and keep others cleaned up?
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3242 +/- ##
==========================================
+ Coverage 83.62% 83.66% +0.03%
==========================================
Files 76 76
Lines 3182 3177 -5
Branches 538 538
==========================================
- Hits 2661 2658 -3
+ Misses 521 519 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -4,6 +4,7 @@ | |||
| using System.Reflection; | |||
| using System.Xml.XPath; | |||
| using Microsoft.AspNetCore.Authentication; | |||
| using Microsoft.AspNetCore.Mvc; | |||
There was a problem hiding this comment.
What is this addition for?
| @@ -202,7 +202,6 @@ Swashbuckle.AspNetCore.SwaggerGen.SchemaFilterContext.Type.get -> System.Type | |||
| Swashbuckle.AspNetCore.SwaggerGen.SchemaGenerator | |||
| Swashbuckle.AspNetCore.SwaggerGen.SchemaGenerator.GenerateSchema(System.Type modelType, Swashbuckle.AspNetCore.SwaggerGen.SchemaRepository schemaRepository, System.Reflection.MemberInfo memberInfo = null, System.Reflection.ParameterInfo parameterInfo = null, Microsoft.AspNetCore.Mvc.ApiExplorer.ApiParameterRouteInfo routeInfo = null) -> Microsoft.OpenApi.Models.OpenApiSchema | |||
| Swashbuckle.AspNetCore.SwaggerGen.SchemaGenerator.SchemaGenerator(Swashbuckle.AspNetCore.SwaggerGen.SchemaGeneratorOptions generatorOptions, Swashbuckle.AspNetCore.SwaggerGen.ISerializerDataContractResolver serializerDataContractResolver) -> void | |||
| Swashbuckle.AspNetCore.SwaggerGen.SchemaGenerator.SchemaGenerator(Swashbuckle.AspNetCore.SwaggerGen.SchemaGeneratorOptions generatorOptions, Swashbuckle.AspNetCore.SwaggerGen.ISerializerDataContractResolver serializerDataContractResolver, Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Mvc.MvcOptions> mvcOptions) -> void | |||
There was a problem hiding this comment.
Yep it is - the better approach would be to mark it as obsolete. We could then remove it in some future major version.
| @@ -984,29 +984,6 @@ public void GenerateSchema_SupportsOption_NonNullableReferenceTypesAsRequired_Re | |||
| Assert.Equal(required, propertyIsRequired); | |||
| } | |||
|
|
|||
| [Theory] | |||
| [InlineData(typeof(TypeWithNullableContextAnnotated), nameof(TypeWithNullableContextAnnotated.SubTypeWithOneNonNullableContent), nameof(TypeWithNullableContextAnnotated.NonNullableString), false)] | |||
There was a problem hiding this comment.
I would keep the tests to verify that the obsolete constructor behaves as expected (i.e. ignoring the MvcOptions value). Marking the test as obsolete would remove any warnings about obsolete usage, and then we'd delete those tests when the constructor is removed at a future date.
|
|
||
| namespace Swashbuckle.AspNetCore.SwaggerGen | ||
| { | ||
| public class SchemaGenerator : ISchemaGenerator | ||
| { | ||
| private readonly SchemaGeneratorOptions _generatorOptions; | ||
| private readonly ISerializerDataContractResolver _serializerDataContractResolver; | ||
| private readonly IOptions<MvcOptions> _mvcOptions; |
There was a problem hiding this comment.
We can remove the field and usage of the constructor argument, but we need to keep the signature the same but mark it as obsolete instead.
There was a problem hiding this comment.
applied suggestion
bring back tests
The issue or feature being addressed
Fixes #3203
Details on the issue fix or feature implementation
remove any usages of MvcOptions in 'SchemaGenerator'. It should not be a breaking change since bool expression is 'true' by definition if 'SuppressImplicitRequiredAttributeForNonNullableReferenceTypes=false'