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.

Sign up for GitHub

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

chore: Refactored openapi sepcifications and build doc #2124

Closed
wants to merge 1 commit into from
Closed

chore: Refactored openapi sepcifications and build doc #2124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 0 additions & 74 deletions waku/waku_api/rest/admin/openapi.yaml
View file
Open in desktop

This file was deleted.

64 changes: 0 additions & 64 deletions waku/waku_api/rest/debug/openapi.yaml
View file
Open in desktop

This file was deleted.

37 changes: 37 additions & 0 deletions waku/waku_api/rest/doc/adminapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
get:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the API documentation rather the API specification.
It would make sense to call this folder spec(wherever we decide to keep it), because doc is what can be generated from this API spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I accept it!

summary: Get connected peers info
description: Retrieve information about connected peers.
operationId: getPeerInfo
tags:
- admin
responses:
'200':
description: Information about a Waku v2 node.
content:
application/json:
schema:
type: array
items:
$ref: "./schemas/apitypes.yaml#/WakuPeer"
'5XX':
description: Unexpected error.
post:
summary: Adds new peer(s) to connect with
description: Adds new peer(s) to connect with.
operationId: postPeerInfo
tags:
- admin
requestBody:
content:
application/json:
schema:
type: array
items:
type: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Having string here is ambigous, better to have a comment or a type indicating what info is required to be passed.
A user of the API will not know what to be passed, is it just a peerID? If so, how about multiAddress ?

responses:
'200':
description: Ok
'400':
description: Cannot connect to one or more peers.
'5XX':
description: Unexpected error.
17 changes: 17 additions & 0 deletions waku/waku_api/rest/doc/debugapi_info.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
get:
summary: Get node info
description: Retrieve information about a Waku v2 node.
operationId: getNodeInfo
tags:
- debug
responses:
'200':
description: Information about a Waku v2 node.
content:
application/json:
schema:
$ref: './schemas/apitypes.yaml#/WakuInfo'
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit: but maybe we can rename WakuInfo to LocalNodeInfo/WakuNodeInfo

'4XX':
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you see a scenario where 4xx would be returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, nice catch!

description: Bad request error.
'5XX':
description: Unexpected error.
18 changes: 18 additions & 0 deletions waku/waku_api/rest/doc/debugapi_version.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
get:
summary: Get node version
description: Retrieve the Waku v2 node version.
operationId: getNodeVersion
tags:
- debug
responses:
'200':
description: The version of a Waku v2 node.
content:
text/plain:
schema:
type: string
'4XX':
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above, I don't see when 4xx would be returned for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, not!

description: Bad request error.
'5XX':
description: Unexpected error.

41 changes: 41 additions & 0 deletions waku/waku_api/rest/doc/filterapi_v1_messages.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Legacy support for v1 waku filter
# /filter/v1/messages/{contentTopic}:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a good practice for URL paths. As per https://swagger.io/docs/specification/describing-parameters/#path-parameters, path params are used to refer to a specific resource within a collection. Not sure contentTopic fits into this for filter.

Better practice for the path would be defined as /filter/v1/messages with contentTopic as query parameter.

get: # get_waku_v2_filter_v1_messages
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to merge all filter_v1 APis in one file. Similarly for filter_v2 as well.
Any specific reason only filter API's have been split across multiple files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is the only way... at least that I know ... open api specification allows referencing files. So there is no way, that I would prefer more either, to move the paths also to the ref files.
I do not like it much, but - imho - still better maintainable separated than in one big.

Copy link
Contributor

Choose a reason for hiding this comment

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

One of my reason for suggesting this kind of organization is a user who wants to use a protocol has to load/work with only 1 file, hence suggesting to merge filterv1 related API's into 1.
It doesn't cause that big of an impact, just a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chaitanyaprem what do you mean by load only 1 file?
This setup now follows the only possible file split in open-api structure.
User will face only one file: openapi.yaml
All sub yamls are referenced from there. Through that root openapi.yaml it is possible to generate reference doc/webpage, also can be imported to Postman if one needs it.

summary: Get the latest messages on the polled content topic
description: Get a list of messages that were received on a subscribed content topic after the last time this method was called.
operationId: getMessagesByTopic
tags:
- filter_legacy
parameters:
- in: path
name: contentTopic # Note the name is the same as in the path
required: true
schema:
type: string
description: Content topic of message
responses:
'200':
description: The latest messages on the polled topic.
content:
application/json:
schema:
$ref: './schemas/apitypes.yaml#/FilterGetMessagesResponse'
# TODO: Review the possible errors of this endpoint
'400':
description: Bad request.
content:
text/plain:
schema:
type: string
'404':
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be returned when i tested with local nwaku node.
Ideally if contentTopic is not found or node is not subscribed, we should be returning this error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I need to test it, although I think we should check if we have subscription for the given contentTopic only.
Than its a bad request.
This can be an issue for implementation rather. But here I will give a better explanation!

description: Not found.
content:
text/plain:
schema:
type: string
'5XX':
description: Unexpected error.
content:
text/plain:
schema:
type: string
71 changes: 71 additions & 0 deletions waku/waku_api/rest/doc/filterapi_v1_subscriptions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# Legacy support for v1 waku filter
# /filter/v1/subscriptions:
post: # post_waku_v2_filter_v1_subscription
chaitanyaprem marked this conversation as resolved.
Show resolved Hide resolved
summary: Subscribe a node to an array of topics
description: Subscribe a node to an array of content topics.
operationId: postSubscriptions
tags:
- filter_legacy
requestBody:
content:
application/json:
schema:
$ref: './schemas/apitypes.yaml#/FilterLegacySubscribeRequest'
responses:
'200':
description: OK
content:
text/plain:
schema:
type: string
# TODO: Review the possible errors of this endpoint
'400':
description: Bad request.
content:
text/plain:
schema:
type: string
'5XX':
description: Unexpected error.
content:
text/plain:
schema:
type: string

delete: # delete_waku_v2_filter_v1_subscription
summary: Unsubscribe a node from an array of topics
description: Unsubscribe a node from an array of content topics.
operationId: deleteSubscriptions
tags:
- filter
requestBody:
content:
application/json:
schema:
$ref: './schemas/apitypes.yaml#/FilterLegacySubscribeRequest'
responses:
'200':
description: OK
content:
text/plain:
schema:
type: string
# TODO: Review the possible errors of this endpoint
'400':
description: Bad request.
content:
text/plain:
schema:
type: string
'404':
description: Not found.
content:
text/plain:
schema:
type: string
'5XX':
description: Unexpected error.
content:
text/plain:
schema:
type: string
40 changes: 40 additions & 0 deletions waku/waku_api/rest/doc/filterapi_v2_messages.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# /filter/v2/messages/{contentTopic}:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment for contentTopic being a URL param as above.

get: # get_waku_v2_filter_v2_messages
summary: Get the latest messages on the polled content topic
description: Get a list of messages that were received on a subscribed content topic after the last time this method was called.
operationId: getMessagesByTopic
tags:
- filter
parameters:
- in: path
name: contentTopic # Note the name is the same as in the path
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if we should also have an optional pubsubTopic mentioned in case the API is to be used by someone who is not following autosharding.
In such a case they would specify both pubsub and contentTopic.

Actually a better way would be to specify topicFilter with type as autosharding/StaticOrNamedSharding.
In autosharding type only contentTopic can be specified, whereas for StaticOrNamedSharding the user has to specify both pubsubTopic and contentTopic.

This same topicFilter can be used for filterSubscribe, Unsubscribe and fetching messages.

required: true
schema:
type: string
description: Content topic of message
responses:
'200':
description: The latest messages on the polled topic.
content:
application/json:
schema:
$ref: './schemas/apitypes.yaml#/FilterGetMessagesResponse'
# TODO: Review the possible errors of this endpoint
'400':
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to include an error message in 400 response which indicates detailed issue.
In this case, it could be that contentTopic format is not proper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, although I'm not pretty sure if there is an exact contentTopic format spec or just recommendation?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a recommendation, and when autosharding is used the format has to follow the recommendation otherwise an error is thrown.

description: Bad request.
content:
text/plain:
schema:
type: string
'404':
Copy link
Contributor

Choose a reason for hiding this comment

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

This error can actually mean that filter subscription is not there for this contentTopic.
Maybe description can be changed to indicate the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, description shall be more explanatory.

description: Not found.
content:
text/plain:
schema:
type: string
'5XX':
description: Unexpected error.
content:
text/plain:
schema:
type: string
Loading
Loading