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 bugs on and add missing fields to OpenAPI spec #2457

Conversation

longnguyencircle
Copy link
Contributor

@longnguyencircle longnguyencircle commented Aug 27, 2021

Signed-off-by: Long Nguyen [email protected]

Description:

This PR modifies our OpenAPI spec YAML file to fix some glaring bugs that pass client generation, but do not pass actual usage when writing real code that attempts to talk to the REST API.

Related issue(s): #2447

Fixes #

Notes for reviewer:

  • tokenid -> tokenId The API spec had this case typo, which trickles down into the generated Jersey client and causes us to fail to send the right query parameter.
  • Supply fields had the wrong type; screenshots included.
  • Three unknown fields were added, which should have been added at the time of their implementation in the mirror node: transaction.bytes, transaction.entity_id, and token.fee_schedule_key. From now on, if you add any new fields to any objects in the API, please add them to the spec as well. OpenAPI Generator does not have an option to inject the generated client files with @IgnoreUnknownProperties, and likely never will, so any time the generated client sees an unknown property, it fails fast and breaks downstream code.
  • api/v1/transactions can actually take multiple timestamp constraints to provide a clear search window with left and right boundaries, ex. https://mainnet-public.mirrornode.hedera.com/api/v1/transactions?timestamp=lte:2000000000.711927001&&timestamp=gte:1500000000.711927001. I updated the spec to reflect this, and it does work as expected (Jersey will send all parameters).

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@@ -1080,19 +1080,21 @@ components:
expiry_timestamp:
example: null
nullable: true
fee_schedule_key:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2021-08-27 at 10 45 02 AM

type: number
example: 9223372036854775807
type: string
example: "9223372036854775807"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type: number
example: 1000000
type: string
example: "1000000"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type: string
format: byte
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a byte string; it's literally a regular string with Base64 encoding.
Screen Shot 2021-08-27 at 10 46 55 AM

Copy link
Member

Choose a reason for hiding this comment

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

The type: string specifies that it is string. The optional format: byte indicates that it is base64 encoded.. See the data types section in the spec.

Copy link
Contributor Author

@longnguyencircle longnguyencircle Aug 27, 2021

Choose a reason for hiding this comment

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

Thanks, learned something new! :) Let me revert this locally and get it working as it was originally.

Comment on lines 1212 to 1215
entity_id:
type: string
bytes:
type: string
Copy link
Contributor Author

@longnguyencircle longnguyencircle Aug 27, 2021

Choose a reason for hiding this comment

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

These fields were missing, which causes any generated client code to fail when invoked (fee_schedule_key as well).

@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #2457 (fd73c98) into main (3556c8b) will not change coverage.
The diff coverage is n/a.

❗ Current head fd73c98 differs from pull request most recent head 652e3c5. Consider uploading reports for the commit 652e3c5 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##               main    #2457   +/-   ##
=========================================
  Coverage     91.58%   91.58%           
  Complexity     2534     2534           
=========================================
  Files           420      420           
  Lines         11591    11591           
  Branches       1016     1016           
=========================================
  Hits          10616    10616           
  Misses          644      644           
  Partials        331      331           
Impacted Files Coverage Δ
tokens.js 96.21% <0.00%> (ø)

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 3556c8b...652e3c5. Read the comment docs.

hedera-mirror-rest/api/v1/openapi.yml Outdated Show resolved Hide resolved
hedera-mirror-rest/api/v1/openapi.yml Show resolved Hide resolved
hedera-mirror-rest/api/v1/openapi.yml Show resolved Hide resolved
@steven-sheehy steven-sheehy added bug Type: Something isn't working P2 rest Area: REST API labels Aug 27, 2021
@steven-sheehy steven-sheehy added this to the Mirror 0.40.0 milestone Aug 27, 2021
@longnguyencircle longnguyencircle force-pushed the fix-missing-params-and-more-openapi-spec branch 2 times, most recently from 2902355 to 7def82f Compare August 27, 2021 20:13
Signed-off-by: Long Nguyen <[email protected]>

Undo changes and add format: bytes

Signed-off-by: Long Nguyen <[email protected]>

Byte, not bytes

Change supply fields back to strings

Signed-off-by: Long Nguyen <[email protected]>
@longnguyencircle longnguyencircle force-pushed the fix-missing-params-and-more-openapi-spec branch from 7def82f to 652e3c5 Compare August 27, 2021 20:14
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. Thanks!

@steven-sheehy steven-sheehy merged commit bc19911 into hashgraph:main Aug 27, 2021
@longnguyencircle longnguyencircle deleted the fix-missing-params-and-more-openapi-spec branch August 30, 2021 12:39
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 P2 rest Area: REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants