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

Remove unreferenced schema, add --trim-unused-schema & --keep-schema #199

Merged

Conversation

kirides
Copy link
Contributor

@kirides kirides commented Oct 23, 2023

fixes #170

This DRAFT implements the aforementioned feature request.

it adds a new commandline switch (--trim-unused-schema) aswell as configuration option (trimUnusedSchema: true) which works well in conjunction with --tag Xyz filters or similar.

The new option removes any Schema found in #/components/... which is not referenced in any way by the operations of the OpenApi document.

This results in much less code being generated for large documents which are filtered to generate only a few endpoints.

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #199 (bd91fce) into main (28d4fbb) will decrease coverage by 0.35%.
Report is 1 commits behind head on main.
The diff coverage is 94.37%.

@@            Coverage Diff             @@
##             main     #199      +/-   ##
==========================================
- Coverage   98.64%   98.30%   -0.35%     
==========================================
  Files          52       55       +3     
  Lines        1842     2002     +160     
==========================================
+ Hits         1817     1968     +151     
- Misses          9       17       +8     
- Partials       16       17       +1     
Flag Coverage Δ
unittests 98.30% <94.37%> (-0.35%) ⬇️

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

Files Coverage Δ
src/Refitter.Core/RefitGenerator.cs 99.40% <100.00%> (+0.04%) ⬆️
...Refitter.Tests/Examples/KeepSchemaPatternsTests.cs 100.00% <100.00%> (ø)
...c/Refitter.Tests/Examples/TrimUnusedSchemaTests.cs 100.00% <100.00%> (ø)
src/Refitter.Core/SchemaCleaner.cs 89.41% <89.41%> (ø)

@christianhelle christianhelle added the enhancement New feature, bug fix, or request label Oct 23, 2023
@christianhelle christianhelle self-assigned this Oct 23, 2023
@christianhelle
Copy link
Owner

@kirides Thanks for this! So far it looks good to me, but I'll go through it in detail later and maybe hunt for a couple of edge cases and test with a couple of other OpenAPI specs that I can find

@christianhelle
Copy link
Owner

@kirides I'm curious why this is just a draft. Aside from missing docs and a naming convention where members are prefixed with _ (not my cup of tea), I think the changes look good. Is there anything here that you have your own doubts on?

type: string
nullable: true
additionalProperties: false
UserComponent:
Copy link
Owner

@christianhelle christianhelle Oct 23, 2023

Choose a reason for hiding this comment

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

@kirides What happens if there is a component called AnotherUserComponent that references UserComponent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's not accessible from any request schema (request, header, parameters, response) it will be trimmed away.

OpenApi doesn't have "inheritance", so any Schema that might be returned, should be in an AnyOf response type, thus won't be removed.

application/json:
schema:
$ref: '#/components/schemas/Warehouse'
'400':
Copy link
Owner

Choose a reason for hiding this comment

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

I'm guilty of defining a ProblemDetails component but never specifying it as a 400 response content 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

at work we return ProblemDetails for anything non success, thus any 400 & 500, don't ask me why.


public class SchemaCleaner
{
private readonly OpenApiDocument _doc;
Copy link
Owner

Choose a reason for hiding this comment

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

@kirides this is just me nit-picking, but can we rename _doc to document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
_doc = doc;
}
public void RemoveUnreferencedSchema()
Copy link
Owner

Choose a reason for hiding this comment

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

@kirides can you please add an empty line between the constructor and the RemoveUnreferencedSchema() method declaration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@kirides
Copy link
Contributor Author

kirides commented Oct 23, 2023

@kirides I'm curious why this is just a draft. Aside from missing docs and a naming convention where members are prefixed with _ (not my cup of tea), I think the changes look good. Is there anything here that you have your own doubts on?

I had some gripes with the way schema references work with NSwag, as they work a bit different than Microsoft's OpenApi libs.

I'll have to check with some more elaborate documents (headers, ref of ref, ...) before I'm done.

As for why I did open this PR while still in the draft, just to show that I do actually implement the feature according to the issue and maybe get some eye on it before it's "complete"

@kirides kirides changed the title [DRAFT] remove unreferenced schema Remove unreferenced schema, add --trim-unused-schema & --keep-schema Nov 3, 2023
@kirides kirides marked this pull request as ready for review November 3, 2023 18:30
Copy link

sonarcloud bot commented Nov 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

No Coverage information No Coverage information
0.5% 0.5% Duplication

Copy link
Owner

@christianhelle christianhelle left a comment

Choose a reason for hiding this comment

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

@kirides Great stuff as usual! Thanks again your contributions

@christianhelle christianhelle merged commit 41e095d into christianhelle:main Nov 5, 2023
423 of 425 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, bug fix, or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unused schema definitions (e.g. --remove-unreferenced-schema )
2 participants