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

docs(api): fix request and response types #4244

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Jun 6, 2024

What this PR changes/adds

  • distinguish method names for different api versions
  • push down all the jakarta.ws.rs annotation from abstract classes to implementations

Why it does that

fix the api docs output (as outlined in #4243)

Further notes

removed all the unnecessary operationId attributes from @Operation swagger annotations

Linked Issue(s)

Closes #4243

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@ndr-brt ndr-brt added documentation Improvements or additions to documentation enhancement New feature or request labels Jun 6, 2024
@ndr-brt ndr-brt requested a review from wolf4ood June 6, 2024 14:12

@Operation(description = "Request all assets according to a particular query",
operationId = "requestAssetV3",
Copy link
Member

Choose a reason for hiding this comment

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

some tools like API gateways use the operationId to generate their internal routing model. removing it may pose a problem.
I guess the safest way is if we have both, distinct method names and operationIDs

Copy link
Member Author

@ndr-brt ndr-brt Jun 6, 2024

Choose a reason for hiding this comment

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

by default the operationId reflects the method name, if the method is already called requestAssetV3, explicitly defining the operationId is not necessary

Copy link
Member

@paullatzelsperger paullatzelsperger Jun 6, 2024

Choose a reason for hiding this comment

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

but doesn't that mean, that if an operationId is present, it would override whatever the method name is?
and if so, naming the methods ...V3... should not be necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is necessary to avoid the generated openapi mess (I debugged it, not sure about the root cause, but the rename thing definitely fixes the issue).
I see two appreciated side effects:

  • it will avoid outdated operationId definitions (because they are sterile strings)
  • defining different method names for different versions makes total sense (think about a method that in 2 different versions has 2 different signatures/behaviors)

Copy link
Member

@paullatzelsperger paullatzelsperger Jun 6, 2024

Choose a reason for hiding this comment

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

so long as it fixes the issue, i'm all for it. we need to be extra careful with method renaming though, as it could cause obscure behaviour in API gateways etc.

think about a method that in 2 different versions has 2 different signatures/behaviors

That shouldn't be a problem, since they'd live in different classes anyway.

@ndr-brt ndr-brt requested a review from paullatzelsperger June 6, 2024 14:21
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 131 lines in your changes missing coverage. Please review.

Project coverage is 70.54%. Comparing base (7f20ba5) to head (0894be0).
Report is 292 commits behind head on main.

Files Patch % Lines
...sferprocess/v2/TransferProcessApiV2Controller.java 0.00% 20 Missing ⚠️
...tiation/v2/ContractNegotiationApiV2Controller.java 0.00% 12 Missing ⚠️
...ent/policy/v3/PolicyDefinitionApiV3Controller.java 0.00% 12 Missing ⚠️
...sferprocess/v3/TransferProcessApiV3Controller.java 0.00% 12 Missing ⚠️
...finition/v2/ContractDefinitionApiV2Controller.java 0.00% 11 Missing ⚠️
...ent/policy/v2/PolicyDefinitionApiV2Controller.java 0.00% 10 Missing ⚠️
...i/management/secret/v1/SecretsApiV1Controller.java 0.00% 8 Missing ⚠️
...tiation/v3/ContractNegotiationApiV3Controller.java 0.00% 7 Missing ⚠️
...agreement/v2/ContractAgreementApiV2Controller.java 0.00% 6 Missing ⚠️
...finition/v3/ContractDefinitionApiV3Controller.java 0.00% 6 Missing ⚠️
... and 6 more

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4244      +/-   ##
==========================================
- Coverage   71.74%   70.54%   -1.20%     
==========================================
  Files         919     1046     +127     
  Lines       18457    20967    +2510     
  Branches     1037     1169     +132     
==========================================
+ Hits        13242    14792    +1550     
- Misses       4756     5680     +924     
- Partials      459      495      +36     

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

@ndr-brt ndr-brt force-pushed the 4243-fix-api-docs branch from 0894be0 to c003c25 Compare June 6, 2024 14:31
@ndr-brt ndr-brt merged commit 031d4b1 into eclipse-edc:main Jun 6, 2024
16 checks passed
@ndr-brt ndr-brt deleted the 4243-fix-api-docs branch June 6, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

management api openapi got strange behavior after /v3 bump
3 participants