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

Fix OpenAPI bugs related to path parameters #3132

Merged
merged 2 commits into from
Jan 12, 2022

Conversation

longnguyencircle
Copy link
Contributor

@longnguyencircle longnguyencircle commented Jan 12, 2022

Signed-off-by: Long Nguyen longnguyen@circle.com

Description:

This PR modifies the openapi.yaml spec to fix two bugs.

Fixes

  1. The fromQueryPathParam is not valid at all and breaks generated Java code. This is for the /api/v1/contracts/{contractId}/results endpoint, which does not use such a parameter. See https://docs.hedera.com/guides/docs/mirror-node-api/cryptocurrency-api#contract-results.
  2. It is not possible to use the arbitrarily defined entityIdPathParam ref for everything. OpenAPI does not know how to replace the name argument, and there is no way to override specific fields after using a ref either. You end up having wrong code that doesn't even call out with the proper arguments. We need to duplicate the schema, but with different names (since we can't override names). See [Question] Extend/override properties of a parameter  OAI/OpenAPI-Specification#2026.

Notes for reviewer:

Please be careful with OpenAPI updates in the future.

Signed-off-by: Long Nguyen <longnguyen@circle.com>
@codecov
Copy link

codecov bot commented Jan 12, 2022

Codecov Report

Merging #3132 (f99c419) into main (306f61a) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #3132   +/-   ##
=========================================
  Coverage     88.29%   88.30%           
- Complexity     2392     2393    +1     
=========================================
  Files           485      485           
  Lines         13567    13567           
  Branches       1264     1264           
=========================================
+ Hits          11979    11980    +1     
  Misses         1275     1275           
+ Partials        313      312    -1     
Impacted Files Coverage Δ
...a/mirror/grpc/listener/CompositeTopicListener.java 83.33% <0.00%> (+4.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 306f61a...f99c419. Read the comment docs.

@steven-sheehy
Copy link
Member

steven-sheehy commented Jan 12, 2022

This looks to be mostly a duplicate of #3108 ?

The fromQueryPathParam issue has already been addressed, I believe.

@longnguyencircle
Copy link
Contributor Author

Ah, I didn't notice that! That is a big PR too—we noticed this when trying to update our Circle nodes. Thanks so much! I believe this can be closed then. :)

@SimiHunjan SimiHunjan mentioned this pull request Jan 12, 2022
2 tasks
Copy link
Member

@steven-sheehy steven-sheehy left a comment

Choose a reason for hiding this comment

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

Ah, I didn't notice that! That is a big PR too—we noticed this when trying to update our Circle nodes. Thanks so much! I believe this can be closed then. :)

Oh, looks like that PR is borked since I last checked after a merge from main. We've closed that one in favor of your PR. Just one change needed. Thanks!

@@ -114,8 +114,7 @@ paths:
description: Returns a list of all ContractResults for a contract's function executions.
operationId: listContractResults
parameters:
- $ref: '#/components/parameters/entityIdPathParam'
- $ref: '#/components/parameters/fromQueryParam'
Copy link
Member

@steven-sheehy steven-sheehy Jan 12, 2022

Choose a reason for hiding this comment

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

Revert removal of fromQueryParam. This is a valid parameter that had a wrong name attribute that was fixed in a previous commit. I think that should already address whatever error you were getting if you can confirm.

@steven-sheehy steven-sheehy added bug Type: Something isn't working P1 rest Area: REST API labels Jan 12, 2022
@steven-sheehy steven-sheehy added this to the 0.49.0 milestone Jan 12, 2022
Signed-off-by: Long Nguyen <longnguyen@circle.com>
Copy link
Member

@steven-sheehy steven-sheehy left a comment

Choose a reason for hiding this comment

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

LGTM

@steven-sheehy steven-sheehy merged commit 27cee98 into hashgraph:main Jan 12, 2022
steven-sheehy pushed a commit that referenced this pull request Jan 12, 2022
* Fix OpenAPI bugs related to path parameters

Signed-off-by: Long Nguyen <longnguyen@circle.com>
steven-sheehy added a commit that referenced this pull request Jan 13, 2022
* Fix OpenAPI bugs related to path parameters

Signed-off-by: Long Nguyen <longnguyen@circle.com>
Co-authored-by: Long Nguyen <longnguyen@circle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Type: Something isn't working P1 rest Area: REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants