feat(rest-api-store): new rest api to retrieve store waku messages (#1611) #1630

Ivansete-status · 2023-03-30T10:47:35Z

Issue

feat: HTTP REST API: Store Client API #1611

Comments

New rest api based on the current store json-rpc api and following the same structure as the current relay rest api.
The store api attend GET requests to retrieve historical messages.
Unit tests added.
Allow to return descriptive error message to rest-client in case of error (4XX or 5XX.).
Always allow to call the store api endpoints (json-rpc & rest) without explicit storenode (chore: allow to call store api endpoints without a storenode #1575.).

Possible enhancements

Implement basic Rust rest-client with OpenApi generator.
Reduce the test size by using 'asyncSetup' and 'asyncTeardown'.

How to test it

Unit tests

Run the next commands from the repo root folder:

. env.sh

nim c -r -d:chronicles_log_level=WARN --verbosity=0 --hints=off --outdir=build ./tests/v2/wakunode_rest/test_rest_store.nim

Manually

All commands from nwaku root folder.

Build the wakunode2:
```
make wakunode2
```

Start a node "A".

Open a new terminal from the nwaku root folder.
Create "cfg_node_a.txt" file with the next content:

ports-shift = 0 
topics = "my-waku-topic"
staticnode = [ "/ip4/127.0.0.1/tcp/60001/p2p/16Uiu2HAmVFXtAfSj4EiR7mL2KvL4EE2wztuQgUSBoj2Jx2KeXFLN" ]
log-level = "DEBUG"
nodekey = "364d111d729a6eb6d2e6113e163f017b5ef03a6f94c9b5b7bb1bb36fa5cb07a9"
#storenode = "/ip4/127.0.0.1/tcp/60001/p2p/16Uiu2HAmVFXtAfSj4EiR7mL2KvL4EE2wztuQgUSBoj2Jx2KeXFLN"
rest = true
metrics-logging = false

Run the next from nwaku root folder:

./build/wakunode2 --config-file=cfg_node_a.txt

Start a store node "B". This will store the messages.

Open a new terminal from the nwaku root folder.
Create "cfg_node_b.txt" file with the next content:

ports-shift = 1
topics = "my-waku-topic"
staticnode = [ "/ip4/127.0.0.1/tcp/60000/p2p/16Uiu2HAm2eqzqp6xn32fzgGi8K4BuF88W4Xy6yxsmDcW8h1gj6ie" ]
log-level = "DEBUG"
nodekey = "0d714a1fada214dead6dc9c7274585eca0ff292451866e7d6d677dc818e8ccd2"
store = true
store-message-db-url = "sqlite://sqlite_folder/store.sqlite3"
store-message-retention-policy = "capacity:150000"

Run the next from nwaku root folder:

./build/wakunode2 --config-file=cfg_node_b.txt

Send a few messages to node "A".

Run the next command a few times, that will post a message through json-rpc:

curl -d "{\"jsonrpc\":\"2.0\",\"id\":"$(date +%s)",\"method\":\"post_waku_v2_relay_v1_message\", \"params\":[\"my-waku-topic-ivan\", {\"timestamp\":"$(date +%s)", \"payload\":\"VGhpcyBpcyBhIG1lc3NhZ2UK\"}]}" --header "Content-Type: application/json" http://localhost:8545

Once both nodes are running, now send Store-Rest requests to node "A".
All the GET parameters should be passed URL-encoded.

Getting all stored messages:

curl -X GET localhost:8645/store/v1/messages?peerAddr=%2Fip4%2F127.0.0.1%2Ftcp%2F60001%2Fp2p%2F16Uiu2HAmVFXtAfSj4EiR7mL2KvL4EE2wztuQgUSBoj2Jx2KeXFLN

Filtering by a particular pubsub topic:

curl -X GET localhost:8645/store/v1/messages?peerAddr=%2Fip4%2F127.0.0.1%2Ftcp%2F60001%2Fp2p%2F16Uiu2HAmVFXtAfSj4EiR7mL2KvL4EE2wztuQgUSBoj2Jx2KeXFLN\&pubsubtopic=my-waku-topic

Filtering by a particular pubsub topic but without setting 'peerAdd' parameter.

curl -X GET localhost:8645/store/v1/messages?pubsubtopic=my-waku-topic

waku/v2/node/rest/store/openapi.yaml

alrevuelta

unsolicitated review, but hope it helps :)

waku/v2/node/rest/store/handlers.nim

Ivansete-status · 2023-04-03T14:03:53Z

unsolicitated review, but hope it helps :)

Hey thanks ! I really appreciate it of course :)
I didn't add you yet because I wanted to make it incremental. Once I apply @LNSD's suggestions I will extend the PR's reviewers

jm-clius

Congratulations on your first PR! :D Approach here looks really good, but I'd like to spend some more time on a review tomorrow morning. Will leave a detailed review then.

docs/api/v2/rest-api.md

jm-clius · 2023-04-04T16:00:41Z

docs/api/v2/rest-api.md


 ### API Specification

-The HTTP REST API has been designed following the OpenAPI 3.0.3 standard specification format. The OpenAPI specification file can be found here: [TBD]()
+The HTTP REST API has been designed following the OpenAPI 3.0.3 standard specification format. The OpenAPI specification files can be found here:


For version-controlled documentation we generally use semantic breaks: generally this just means each sentence (or full clause) in a separate line. These breaks aren't visible in markdown renders, but does help with reviewing and maintaining docs.

jm-clius · 2023-04-04T16:03:24Z

docs/operators/how-to/configure-rest-api.md

@@ -0,0 +1,20 @@
+# Node configuration


Thanks for including operator documentation in the PR! It's important to keep this up to date.

In general, we want every new page in the operator guide to be reachable from the landing page with a few clicks. In other words, it will be useful if this page is linked from somewhere else - I'd suggest just adding this to the configuration tutorials list in the configuration landing page

LNSD

In general, looks great to me 😁 ✅

Please check my comments.

LNSD · 2023-04-05T06:41:53Z

docs/operators/how-to/configure-rest-api.md

+|`--rest-admin` | Enable access to REST HTTP Admin API. | `false` |
+|`--rest-private` | Enable access to REST HTTP Private API. | `false` |


Seeing these two command line options remembers me a question:

@jm-clius Shall we put each "namespaced" group of REST API endpoints behind a configuration flag (e.g., in this case, --rest-store-client) for configuration granularity purposes?

Yes, I think that's a good idea. We may just want to start by doing these as sub-commands (see: https://github.com/status-im/nim-confutils#using-sub-commands) which are only available when --rest is enabled, in order to keep things a bit more simplified. We want to do this with other groups of config items as well, but now is a good time as ever to do this for the REST API.

Morning guys @LNSD , @jm-clius . Shall we open a separate issue to tackle that?

Makes sense to me. Let's tackle it in a follow-up PR :)

I issued the next one:
#1652

LNSD · 2023-04-05T06:45:25Z

waku/v2/node/rest/store/handlers.nim

+  try:
+    return Timestamp(parseInt(input))
+  except CatchableError as e:
+    error "Error converting to Timestamp ", error=e.msg
+    return Timestamp(0)


I think this should be a ValueError, it is more specific. Additionally, we are not using the pythonic as ex, we use: https://nim-lang.org/docs/system.html#getCurrentExceptionMsg

Suggested change

try:

return Timestamp(parseInt(input))

except CatchableError as e:

error "Error converting to Timestamp ", error=e.msg

return Timestamp(0)

try:

return Timestamp(parseInt(input))

except ValueError:

error "Error converting to Timestamp ", error= getCurrentExceptionMsg()

return Timestamp(0)

This also applies to the rest of try/except from this PR 🙂

Thanks for the comment.
Actually, I realized that the management of time-parameters should be normalized, i.e. we have four possible time-parameters and the user needs a valid feedback for each of them.

LNSD · 2023-04-05T06:46:27Z

waku/v2/node/rest/store/handlers.nim

+# The param must be in the format `(ip4|ip6)/tcp/p2p/$peerId` but URL-encoded
+proc parsePeerAddr(peerAddr: string): Result[RemotePeerInfo, string] =
+  let parsedAddr = decodeUrl(peerAddr)
+  debug "parsePeerAddr ", parsedAddr


Logging types directly without converting them to string, via $, may lead to breaking the structured logging/json logging.

Suggested change

debug "parsePeerAddr ", parsedAddr

debug "parsePeerAddr ", parsed_addr = $parsedAddr

LNSD · 2023-04-05T06:52:29Z

waku/v2/node/rest/store/handlers.nim

+  try:
+    return Timestamp(parseInt(input))
+  except CatchableError as e:
+    error "Error converting to Timestamp ", error=e.msg


I am not entirely convinced with this error log level. We should be very selective when using the error log level (e.g., to avoid alarm fatigue: if everything is an error, nothing is an actual error).

If we return a default value, I don't see it as critical to being logged with an error level. I think I wouldn't log anything, but if you think this should be logged, then pick a level like debug or trace.

Yes I agree, I will pick debug level in this case.
If this exception occurs, the problem comes from how the user sent the parameter, rather than an app error, so it is important to give good feedback to the user and just log in debug.

LNSD · 2023-04-05T06:54:20Z

waku/v2/node/rest/store/handlers.nim

+      ascending: Option[string]
+      ) -> RestApiResponse:
+
+    debug "REST-GET /store/v1/messages"


I would add the "peer address" to the log for debugging purposes.

LNSD · 2023-04-05T06:59:45Z

waku/v2/node/rest/store/client.nim

+              ))
+
+  # If everything goes wrong
+  return err("Unsupported contentType")


We should cover our backs for the unknown. The more debug info that we have, the better 🙂

Suggested change

return err("Unsupported contentType")

return err("Unsupported contentType: " & $contentType)

LNSD · 2023-04-05T07:06:33Z

waku/v2/node/rest/store/handlers.nim

+    let decodedPubsubTopic = decodeUrl(pubsubTopic.get())
+    if decodedPubsubTopic != "":
+      parsedPubsubTopic = some(decodedPubsubTopic)
+    debug "Setting pubsubTopic ", pubsub_topic=parsedPubsubTopic.get()


We need to be consistent with the logged fields name style: sometimes you use the snake_case; others you use camelCase (e.g., parsedAddr). IMO, we should stick with snake_case.

LNSD · 2023-04-05T07:11:04Z

waku/v2/node/rest/store/handlers.nim

+  # Parse ascending field
+  var parsedAscending = true
+  if ascending.isSome() and ascending.get() != "":
+    parsedAscending = ascending.get() == "1"


The ascending field must be true or false (default value is not present). We should avoid the 1/0 on/off yes/no true/false support war.

Completely agree but I couldn't manage to use an Option[bool] given how the nim-presto library parses the route-api code, which only accepts string types:

https://github.com/status-im/nim-presto/blob/2b440a443f3fc29197f267879e16bb8057ccc0ed/presto/route.nim#L322

For now I can only make it to handle "true" or "false" strings.

LNSD · 2023-04-05T07:17:04Z

waku/v2/node/rest/store/openapi.yaml

FWIW. To review the generated documentation, here you can use redocly online to preview this OpenAPI specification file:

https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/waku-org/nwaku/issue-1076-store-rest-api/waku/v2/node/rest/store/openapi.yaml

LNSD · 2023-04-05T07:19:52Z

waku/v2/node/rest/store/openapi.yaml

+        - name: ascending
+          in: query
+          schema:
+            type: string
+          description: >
+            "1" for paging forward, "0" for paging backward
+          example: '1'


I am not convinced with the 1. In my opinion this should be a boolean: true or false.

From the OpenAPI docs: https://swagger.io/docs/specification/data-models/data-types/#boolean

type: boolean represents two values: true and false. Note that truthy and falsy values such as "true", "", 0 or null are not considered boolean values.

Suggested change

- name: ascending

in: query

schema:

type: string

description: >

"1" for paging forward, "0" for paging backward

example: '1'

- name: ascending

in: query

schema:

type: boolean

description: >

"true" for paging forward, "false" for paging backward

example: true

For now I'd use 'string' as per #1630 (comment)

alrevuelta · 2023-04-05T07:32:47Z

tests/v2/wakunode_rest/test_rest_store.nim

@@ -0,0 +1,534 @@
+{.used.}


can't find test_rest_store.nim in https://github.com/waku-org/nwaku/blob/issue-1076-store-rest-api/tests/all_tests_v2.nim so not sure this test is being run with make test2 and consecuently in CI. Mind checking this?

Good catch!

@Ivansete-status For a test suite to be executed. It is required that a test suite file is imported by one of the all_tests_*.nim files (in this case all_tests_v2.nim).

Will get added in next commit. Thanks!

alrevuelta · 2023-04-05T07:45:47Z

tests/v2/wakunode_rest/test_rest_store.nim

+
+    node.peerManager.addServicePeer(remotePeerInfo,
+                                    WakuStoreCodec)
+    peerSwitch.mount(node.wakuRelay)


not sure i understand this. node and peerSwitch are different libp2p peers right? So seems weird to me to mount the relay of one in the other, unless im missing something.

Morning @alrevuelta , you are absolutely right that this code is superfluous. It was inspired by procSuite "Waku v2 JSON-RPC API - Store".
I will remove the wakuRelay references.
Thanks !

alrevuelta · 2023-04-05T07:53:53Z

waku/v2/node/rest/store/handlers.nim

+
+# Checks whether the peerAddr parameter represents a valid p2p multiaddress.
+# The param must be in the format `(ip4|ip6)/tcp/p2p/$peerId` but URL-encoded
+proc parsePeerAddr(peerAddr: string): Result[RemotePeerInfo, string] =


perhaps move to utils/peers.nim?

since its very similar to parseRemotePeerInfo?

Yes, will be in utils/peers.nim in the next commit. We are planning to modify the current parseRemotePeerInfo in further PR's so that it doesn't raise any exception.

waku/v2/node/rest/store/handlers.nim

jm-clius

I've now done a detailed read of the code and this LGTM. Well done and thanks (also for your insights here, @LNSD)!

jm-clius · 2023-04-05T08:37:42Z

waku/v2/node/rest/store/handlers.nim

+
+# Queries the store-node with the query parameters and
+# returns a RestApiResponse that is sent back to the api client.
+proc makeHistoryQuery(selfNode: WakuNode,


Nitpick, but perhaps something like performHistoryQuery is a better proc name here? To me, make sounds similar to create. :)

…1611) * feat: new rest api based on the current store json-rpc api and following the same structure as the current relay rest api. * feat: the store api attend GET requests to retrieve historical messages * feat: unit tests. * feat: allow return message to rest-client in case error (4XX or 5XX) * chore: always allow to call the store api endpoints (only rest) without explicit storenode (#1575) * feat: always mounting the current node as storenode client

Ivansete-status · 2023-04-06T09:46:41Z

Thank you all for the reviews! Together we brought a good step forward.
Let me please give a special thank you to @LNSD ! You rock ! and the previous rest structure is just beautiful :)

Ivansete-status added the track:production label Mar 30, 2023

Ivansete-status requested a review from LNSD March 30, 2023 10:47

LNSD reviewed Apr 3, 2023

View reviewed changes

waku/v2/node/rest/store/openapi.yaml Show resolved Hide resolved

alrevuelta reviewed Apr 3, 2023

View reviewed changes

waku/v2/node/rest/store/handlers.nim Outdated Show resolved Hide resolved

waku/v2/node/rest/store/handlers.nim Show resolved Hide resolved

waku/v2/node/rest/store/handlers.nim Outdated Show resolved Hide resolved

waku/v2/node/rest/store/handlers.nim Outdated Show resolved Hide resolved

Ivansete-status self-assigned this Apr 3, 2023

Ivansete-status force-pushed the issue-1076-store-rest-api branch 2 times, most recently from 0ed647a to b9de7a5 Compare April 4, 2023 14:50

Ivansete-status requested review from alrevuelta, LNSD, vpavlin, rymnc and jm-clius April 4, 2023 15:38

Ivansete-status marked this pull request as ready for review April 4, 2023 15:40

jm-clius reviewed Apr 4, 2023

View reviewed changes

jm-clius self-requested a review April 4, 2023 16:11

LNSD approved these changes Apr 5, 2023

View reviewed changes

alrevuelta reviewed Apr 5, 2023

View reviewed changes

jm-clius approved these changes Apr 5, 2023

View reviewed changes

Ivansete-status force-pushed the issue-1076-store-rest-api branch 2 times, most recently from 78df82f to 37edab1 Compare April 6, 2023 08:32

Ivansete-status force-pushed the issue-1076-store-rest-api branch from 37edab1 to bca0731 Compare April 6, 2023 08:59

Ivansete-status merged commit b2acb54 into master Apr 6, 2023
12 checks passed

Ivansete-status deleted the issue-1076-store-rest-api branch April 6, 2023 09:43

		\|`--rest-admin` \| Enable access to REST HTTP Admin API. \| `false` \|
		\|`--rest-private` \| Enable access to REST HTTP Private API. \| `false` \|

	debug "parsePeerAddr ", parsedAddr
	debug "parsePeerAddr ", parsed_addr = $parsedAddr

	return err("Unsupported contentType")
	return err("Unsupported contentType: " & $contentType)

feat(rest-api-store): new rest api to retrieve store waku messages (#1611) #1630

feat(rest-api-store): new rest api to retrieve store waku messages (#1611) #1630

Conversation

Ivansete-status commented Mar 30, 2023 • edited

Issue

Comments

Possible enhancements

How to test it

Unit tests

Manually

alrevuelta left a comment

Choose a reason for hiding this comment

Ivansete-status commented Apr 3, 2023

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LNSD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LNSD Apr 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status commented Apr 6, 2023

Ivansete-status commented Mar 30, 2023 •

edited

LNSD Apr 6, 2023 •

edited