-
Notifications
You must be signed in to change notification settings - Fork 79
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
Openapi spec split #447
Openapi spec split #447
Conversation
…ger api integration
-- update swagger-ui resource reading Signed-off-by: Usman Saleem <[email protected]>
…irectory name Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
// adjust the ref (which works with OpenApi3Router) to relative that works with | ||
// swagger-ui | ||
final String adjustedFile = | ||
StringUtils.replace(file, "/openapi/eth2/signing/schemas.yaml", "../schemas.yaml"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quite annoying to have to do this twice, both in the gradle task and here :(
Once I add the new API endpoints I'll have another path to replace too on top of this one. Will be /openapi/eth2/keymanager/schemas.yaml
.
Would be great to make this replace logic generic so I don't have to touch this code once I add the new endpoints. Maybe with a regex? If starts with /
-> replace everything before the last slash with ..
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also realized that this flattening of files into one folder means that all filenames must be unique right? Kind of a bummer. Anyway to preserve some of the folder hierarchy and still use relative paths? Like ../signing/schemas.yaml
and ../keymanager/schemas.yaml
Would also make the string replace logic simpler that way. Would just be replace /openapi/eth2
with ..
and done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am about to re-do this logic slightly ... apparently the Vertx's OpenApi3Router (which internally uses swagger-parser) is not fool-proof (i.e. fails to parse) when it deals with relative paths in $ref
(such as sign.yaml referencing ../schema.yaml#....
where it sits two folder deep. It works if all files are on same level. On the other hand, tools like swagger-ui and redoc (which are javascript based) works fine with such relative paths.
I am adding a bit of logic to modify the relative $ref
and discriminator
entries to absolute paths for OpenApi3Router handling.
The end file structure that should work would look like:
openapi
├── eth1
│ └── web3signer.yaml
├── eth2
│ ├── signing
│ │ ├── paths
│ │ │ ├── public_keys.yaml
│ │ │ ├── reload.yaml
│ │ │ ├── sign.yaml
│ │ │ └── upcheck.yaml
│ │ └── schemas.yaml
│ └── web3signer.yaml
├── filecoin
│ └── web3signer.yaml
└── index.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file structure looks good to me! Def had a hard time making relative paths work with Vertx, if there's a robust way to make it work that would be the cleanest for sure.
…d by OpenApi3Router from file system instead of from within the jar Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge progress! Much more robust solution. I'm still baffled that we have to do this relative/absolute paths gymnastic in the first place. Really wish the $ref standard was more standardized across the different OpenAPI tools.
Getting super close though!
@@ -78,7 +78,7 @@ void web3signerYamlEndPointRespondsWith200() { | |||
given() | |||
.baseUri(signer.getUrl()) | |||
.when() | |||
.get(SWAGGER_UI_ENDPOINT + "/web3signer.yaml") | |||
.get(SWAGGER_UI_ENDPOINT + "/eth2/web3signer.yaml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the failing test. Can't see exactly what's wrong with this though, at first glance seems like it should work 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its Vertx 3.x handling of trailing / in route paths. I've added quick fix and a note in Runner class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see, nice!
btw @usmansaleem feel free to close my PR and do the API split in yours! Probably simpler to test and merge confidently. |
Signed-off-by: Usman Saleem <[email protected]>
Thank you, @joaquim-verges , I'll proceed with split openapi specs in this PR 👍🏼 |
Its the swagger-parser-v3 which has issues with relative paths (specially when involved with multi-level folders), going to create a reproducer and file an issue with them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! I'll let @jframe take the final look, but this should unblock the rest of the tasks in #439
Thank you @usmansaleem !
final Path openApiSpec = | ||
openApiSpecsExtractor | ||
.getSpecFilePathAtDestination(getOpenApiSpecResource()) | ||
.orElseThrow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be good throw with a nice error message
core/src/main/java/tech/pegasys/web3signer/core/util/OpenApiSpecsExtractor.java
Show resolved
Hide resolved
core/src/main/java/tech/pegasys/web3signer/core/util/OpenApiSpecsExtractor.java
Outdated
Show resolved
Hide resolved
core/src/test/java/tech/pegasys/web3signer/core/util/OpenApiSpecsExtractorTest.java
Show resolved
Hide resolved
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Merge #444 into this PR.