chore: Refactored openapi sepcifications and build doc #2124
Conversation
…/doc Separated paths and types while having one common doc for the whole api Upon this base we are able to generate api documentation on the fly to the web.
NagyZoltanPeter
added
the
E:REST API service node
NagyZoltanPeter
requested review from
SionoiS,
alrevuelta,
chaitanyaprem,
harsh-98,
fbarbu15,
fryorcraken and
Ivansete-status
|
You can find the image built from this PR at Built from 0e1be7c |
|
Good job! This facilitates the creation of automated tests based on specs, utilizing OpenAPI tools. From schema validators to automated test generation |
chair28980
removed
the
E:REST API service node
I think we should place this spec in a separate repo under wak-org similar to how we are storing proto files at https://github.com/waku-org/waku-proto or place it under rfc repo(with the REST API rfc). |
| @@ -0,0 +1,37 @@ | |||
| get: | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I accept it!
| schema: | ||
| type: array | ||
| items: | ||
| type: string |
There was a problem hiding this comment.
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 ?
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: './schemas/apitypes.yaml#/WakuInfo' |
There was a problem hiding this comment.
Small nit: but maybe we can rename WakuInfo to LocalNodeInfo/WakuNodeInfo
| application/json: | ||
| schema: | ||
| $ref: './schemas/apitypes.yaml#/WakuInfo' | ||
| '4XX': |
There was a problem hiding this comment.
Do you see a scenario where 4xx would be returned?
There was a problem hiding this comment.
Hehe, nice catch!
| text/plain: | ||
| schema: | ||
| type: string | ||
| '4XX': |
There was a problem hiding this comment.
Same comment as above, I don't see when 4xx would be returned for this.
| only WakuMessages that are linked to any of the given | ||
| content topics will be delivered in the get response. | ||
| It should be a URL-encoded-comma-separated string. | ||
| example: 'my%20first%20content%20topic%2Cmy%20second%20content%20topic%2Cmy%20third%20content%20topic' |
There was a problem hiding this comment.
Also, not sure why the example has spaces.
Content-topics as i understand don't have spaces in them rather of the format "toychat/1/abc/proto". Example should reflect the same.
| it can be part of the next page request. | ||
| example: '1680590947000000000' | ||
|
|
||
| - name: storeTime |
There was a problem hiding this comment.
Not sure if this and senderTime should be used.
Because each store node in the network may store the message at a different time, whereas senderTime would be same.
But maybe this is also more of a store protocol change.
| it can be part of the next page request. | ||
| example: '1680590945000000000' | ||
|
|
||
| - name: digest |
There was a problem hiding this comment.
What would happen if i specify multiple cursor fields?
better to document that behaviour so that users of the API are clear as well.
| it can be part of the next page request. | ||
| example: 'Gc4ACThW5t2QQO82huq3WnDv%2FapPPJpD%2FwJfxDxAnR0%3D' | ||
|
|
||
| - name: pageSize |
There was a problem hiding this comment.
If messageSize is too big to fit page size messages in a response, does the server truncate and chunk or send less than requested pageSize?
| example: 'Gc4ACThW5t2QQO82huq3WnDv%2FapPPJpD%2FwJfxDxAnR0%3D' | ||
|
|
||
| - name: pageSize | ||
| in: query |
There was a problem hiding this comment.
Also curious to know how paging works, does the user need to keep querying until empty response is returned?
| type: object | ||
| required: | ||
| - protocol | ||
| - connected |
There was a problem hiding this comment.
Connected need not be per protocol, this is per peer.
I am assuming this indicates whether node is connected with this peer or not and not per protocol.
There was a problem hiding this comment.
Our idea was to group protocol and info under peers. Of course having a peer with no protocol does not make sense, hence the required.
There was a problem hiding this comment.
I do agree that protocol is required, but what i meant is connected status need not be at same level with protocol.
What i am suggesting is that response should be in the following format. If you noticed the peer connection status is shown outside protocols. Also, one more query on this point, does this endpoint list all peers in the peerStore or only peers that we are currently connected to?
{
"multiaddr":[
"/ip4/10.0.0.133/tcp/60000/p2p/16Uiu2HAkymj3jUpK1eGaymZbGftwffVYtoMRgnd8sngC8sSJzabo",
"/ip4/127.0.0.1/tcp/60000/p2p/16Uiu2HAkymj3jUpK1eGaymZbGftwffVYtoMRgnd8sngC8sSJzabo"
],
"connected": true,
"protocols":[
"/vac/waku/filter-push/2.0.0-beta1",
"/vac/waku/relay/2.0.0"
]
}
There was a problem hiding this comment.
My understanding we list all peers we know and their supported protocols we maybe connected at query time.
Suppose that we can have several peers that supports filtering but currently we are using only one of them. (or same with store protocol).
@Ivansete-status WDYT?
| @@ -0,0 +1,25 @@ | |||
| # /relay/v1/auto/messages/{contentTopic}: # Note the plural in messages | |||
There was a problem hiding this comment.
Another reason for not having contentTopic as a URL param is since contentTopic format recommends having /, a user will have to URLencode the contentTopic before invoking the API. It is non-standard but if we include contentTopic in requestBody then this issue can be avoided.
While testing with postman for example with contentTopic /toychat/1/huilong/proto, i had to pass %2Ftoychat%2F1%2Fhuilong%2Fproto in order for this to work.
There was a problem hiding this comment.
We had exactly this conversation in nwaku.
So far our conclusion was, that even though it requires url encoding, having GET request in content body is against rest convention and Http recommendation. But I could live with it having to do so.
Major issue was that the nim-presto implementation we use is - in align with the http spec - forbids having content body for a GET request.
So in order to do so we need to modify our HttpServer implementation.
I'm not sure how it is in your go impl. Does that allows it?
There was a problem hiding this comment.
In golang, the way we are implementing REST methods i don't see any limitation with this.
Will test once to confirm though.
There was a problem hiding this comment.
It is even more extensive in store api. But as I see its only a problem if one would issue such and api call by hand, in all other programmatic case url encoding shall not be an issue.
| - protocols | ||
| properties: | ||
| multiaddr: | ||
| type: string |
There was a problem hiding this comment.
Also, this multiaddr should be an array of multiaddresses for the peer...as a peer can have more than 1 multiaddress advertising.
We should also include peeerID
There was a problem hiding this comment.
Implemented structure here for reference
https://github.com/waku-org/go-waku/blob/9000314447e3450754dbc667a2e2049accb758d1/cmd/waku/server/rest/admin_api.yaml#L55-L74
Haven't added shardInfo yet, but will do.
|
|
||
| It is allowed to refresh or add new content topic to an existing subscription. | ||
|
|
||
| Fields pubsubTopic and contentFilters must be filled. |
There was a problem hiding this comment.
With autosharding, pubsubTopic is not mandatory.
Only contentTopics can be filled and using autosharding algorithm, pubsubTopic is derived.
cc @chaitanyaprem , we need to change documentation for all APIs accordingly.
|
@chaitanyaprem @harsh-98, @fryorcraken , @chair28980 New repository: https://github.com/waku-org/waku-rest-api |
Description
This PR is an effort towards a slim and comprehensive Rest API specification and documentation.
Please take it as a reasoning on choices we can have as well as a source of proposal. (In this manner please feel free to invite others to give opinions)
Current Waku node interface specifications are living next to each endpoint implementation and not really used to provide a real help for waku users on how they can access their node running.
To reach this I think best move is to merge these separate specifications and use as single source of truth.
Although from maintenance point of view i think it is better to have it well splitted up yet being able to organized in hierarchy.
I found unfortunate the OpenApi specification rules are a bit too restrictive on this but was able to reorganize under one openapi.yaml - as a root of the specification - while having endpoints defined alone.
Schema specification was also aimed to get common and eliminate duplications.
This approach is presented here.
There are few things to get decided.
For getting an impression I created a generated api doc under my gh pages:
https://nagyzoltanpeter.github.io/nwaku-api/index.html
Changes
Issue
#2120