Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support new UseInlineDefinitionsForObjects flag #2533

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sol-wasserman
Copy link

Problem

When a request/response has a nested object, there are two pieces of information missing from the generated doc file:

  1. The property's summary is not displayed.
  2. The Nullable marker is not shown for nullable properties.

Here's a screenshot of what it looks like now:

image

Here's a screenshot of the source code for the above:

image

As you can see, this property has a nice summary, and is nullable.

Here's a screenshot of what it I would expect:

image

Underlying Cause

It looks to me like the reason it's not generating that properly is because the object is only a reference, and Swagger doesn't support these on references. As the Swagger specification says, regarding $refs:

This object cannot be extended with additional properties and any properties added SHALL be ignored.

Proposed Solution

We have previously introduced an option to UseInlineDefinitionsForEnums. Among other things, this will show the summary of a property that is an enum, and will also show the Nullable marker accordingly.

This PR adds a similar UseInlineDefinitionsForObjects option. When enabled, it'll inline the whole schema, and will properly show the summary and whether it's nullable:

image

Copied from #2384 which was accidentally closed.

@ColKrumpler
Copy link

@domaindrivendev could this be evaluated to be merged in?

@@ -244,7 +244,7 @@ private OpenApiSchema GenerateConcreteSchema(DataContract dataContract, SchemaRe
case DataType.Object:
{
schemaFactory = () => CreateObjectSchema(dataContract, schemaRepository);
returnAsReference = true;
returnAsReference = !_generatorOptions.UseInlineDefinitionsForObjects;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the original PR for this change #2426, you'll want to add support for self-referencing types:

returnAsReference = !_generatorOptions.UseInlineDefinitionsForObjects ||
                                        dataContract.ObjectProperties.Any(prop =>
                                            prop.MemberType.IsAssignableFrom(dataContract.UnderlyingType) ||
                                            prop.MemberType.GenericTypeArguments.Any(type =>
                                                type.IsAssignableFrom(dataContract.UnderlyingType)));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test for this to also validate the change:

        [Fact]
        public void GenerateSchema_SupportsOption_UseReferenceForSelfReferenceTypesWhenUseInlineDefinitionsForObjects()
        {
            var subject = Subject(
                configureGenerator: c => c.UseInlineDefinitionsForObjects = true
            );

            var schema = subject.GenerateSchema(typeof(SelfReferencingType), new SchemaRepository());

            Assert.NotNull(schema.Reference);
            Assert.Equal("SelfReferencingType", schema.Reference.Id);
        }

@martincostello
Copy link
Collaborator

Thanks for contributing - if you'd like to continue with this pull request, please rebase against the default branch to pick up our new CI.

Copy link
Collaborator

@martincostello martincostello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me - I think I've even researched this exact lack of summary (and maybe the nullability) myself before in the past.

If have no specific feedback other than than it looks like there's some comments from jamesmcroft that should be looked at.

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 91.64%. Comparing base (45a2cb7) to head (3f8b012).

Files Patch % Lines
...DependencyInjection/SwaggerGenOptionsExtensions.cs 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2533      +/-   ##
==========================================
- Coverage   91.70%   91.64%   -0.06%     
==========================================
  Files          91       91              
  Lines        3025     3029       +4     
  Branches      519      519              
==========================================
+ Hits         2774     2776       +2     
- Misses        251      253       +2     
Flag Coverage Δ
Linux 91.64% <60.00%> (-0.06%) ⬇️
Windows 91.64% <60.00%> (-0.06%) ⬇️
macOS 91.64% <60.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jamesmcroft
Copy link

This seems reasonable to me - I think I've even researched this exact lack of summary (and maybe the nullability) myself before in the past.

If have no specific feedback other than than it looks like there's some comments from jamesmcroft that should be looked at.

As the author of the original change being presented here, I know what has been included from my PR here will not work correctly for self-referencing types. Please consider bringing these additional changes from the original PR @sol-wasserman

@sol-wasserman

This comment was marked as outdated.

@martincostello
Copy link
Collaborator

Can you add a test that exhibits the above is handled correctly, whether or not the non-test code is changed? If it then isn't, we should fix that somehow.

@sol-wasserman
Copy link
Author

@jamesmcroft I got a little stuck trying to solve this. Your linked piece of code correctly solves for self-references, but I couldn't figure out how to solve for circular references that may be buried deeper into the tree.

Do you have any ideas?

@cervengoc
Copy link

Thank you for this PR, this is exactly what I was investigating. However, I'd like to suggest a little generalization.

I would like to have this option as a callback receiving at least the contract type which is about to be generated. So that way one can configure this in a much more flexible way, for example using this mode only for several types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants