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

bug: REST relay publish returns HTTP 500 Internal Server Error instead of 4XX for invalid requests #914

Closed
fbarbu15 opened this issue Nov 17, 2023 · 5 comments
Assignees
Labels
bug Something isn't working E:REST API service node See https://github.com/waku-org/pm/issues/82 for details

Comments

@fbarbu15
Copy link

Describe the bug
For invalid requests it's better to have some meaningful error codes/messages. HTTP 500 Internal Server Error is misleading and inconsistent with RPC protocol or REST on NWAKU.

To Reproduce

  1. Have 2 nodes connected between them (I used discv5)
  2. Subscribe both nodes to pubsub_topic X
  3. Try to publish a valid message on pubsub_topic Y using the REST endpoint relay/v1/messages/
    => HTTP 500 instead of 4XX
  4. Try to publish a invalid message without payload on pubsub_topic X using the REST endpoint relay/v1/messages/
    => HTTP 500 instead of 4XX

Comparisons between REST/RCP and NWAKU to see inconsistency
Added comments for those that don't seem correct. However the main problem is HTTP 500

For the 1st scenario:
GOWAKU

- min-relay-peers-to-publish=0:

RPC: HTTP 400 (Bad Request): "error":"no subscription found for content topic" (should be pubsub topic but this is minor)
REST: HTTP 404 (Not Found): not subscribed to topic (would look better: not subscribed to pubsub topic. Also should we return 400 instead of 404 to be consistent with RPC and NWAKU?)

- min-relay-peers-to-publish=1:

RPC:HTTP 400 (Bad Request): "error":"not enough peers to publish" (doesn't seem correct as there is 1 peer, problem here is that the node is not subscribed to this pubsub topic )
REST:HTTP 500 (Internal Server Error) with no content (definitely not right, I would expect 4XX with Failed to publish: Node not subscribed to pubsub topic: /waku/2/rs/18/3)

NWAKU

REST: HTTP 400 (Bad Request): "Failed to publish: Node not subscribed to topic: /waku/2/rs/18/3"

For the 2nd scenario:
GOWAKU

RPC:HTTP 400 (Bad Request): "error":"missing Payload field"
REST:HTTP 500 (Internal Server Error) with no content (definitely not right, I would expect 4XX with missing Payload field)

NWAKU(REST)

HTTP 400 (Bad Request): "error":"Invalid content body, could not decode. Unable to deserialize data"

go-waku version/commit hash

wakuorg/go-waku:latest

@fbarbu15 fbarbu15 added bug Something isn't working E:REST API service node See https://github.com/waku-org/pm/issues/82 for details labels Nov 17, 2023
@chaitanyaprem
Copy link
Collaborator

RPC:HTTP 400 (Bad Request): "error":"not enough peers to publish" (doesn't seem correct as there is 1 peer, problem here is that the node is not subscribed to this pubsub topic )

If you are getting this error, it must be because there is no peer connected for the pubsubTopic that you are trying to publish the messages. Can you share logs for this test?

chaitanyaprem added a commit that referenced this issue Nov 22, 2023
@chaitanyaprem chaitanyaprem mentioned this issue Nov 22, 2023
2 tasks
@fbarbu15
Copy link
Author

RPC:HTTP 400 (Bad Request): "error":"not enough peers to publish" (doesn't seem correct as there is 1 peer, problem here is that the node is not subscribed to this pubsub topic )

If you are getting this error, it must be because there is no peer connected for the pubsubTopic that you are trying to publish the messages. Can you share logs for this test?

Yes, the peers are connected on a different pubsubTopic.
Node1 and Node2 are connected on pubsub_topic X and Node1 tries to publish a message on pubsub_topic Y.
A better message IMO would be not subscribed to pubsub topic

The one I get now, not enough peers to publish would suggest that Node1 is subscribed to pubsub_topic Y, but no other peer is subscribed to this pubsub_topic Y
Here's the log
Thanks
node1_2023-11-22_10-30-35__a4207863-490d-4f54-bb91-8e97a8d7870f__wakuorg_go-wakulatest.log

@chaitanyaprem
Copy link
Collaborator

RPC:HTTP 400 (Bad Request): "error":"not enough peers to publish" (doesn't seem correct as there is 1 peer, problem here is that the node is not subscribed to this pubsub topic )
If you are getting this error, it must be because there is no peer connected for the pubsubTopic that you are trying to publish the messages. Can you share logs for this test?

Yes, the peers are connected on a different pubsubTopic. Node1 and Node2 are connected on pubsub_topic X and Node1 tries to publish a message on pubsub_topic Y. A better message IMO would be not subscribed to pubsub topic

The one I get now, not enough peers to publish would suggest that Node1 is subscribed to pubsub_topic Y, but no other peer is subscribed to this pubsub_topic Y Here's the log Thanks node1_2023-11-22_10-30-35__a4207863-490d-4f54-bb91-8e97a8d7870f__wakuorg_go-wakulatest.log

Got it, Thanks.

Anyways added a check to verify is pubsubTopic subscription is there or not in the linked PR.

@chaitanyaprem
Copy link
Collaborator

As per discussion here https://discord.com/channels/1110799176264056863/1176811954598330378 , there is no requirement of topic subscription in order to publish to it.

Which means, as long as there are enough peers connected to a pubsubTopic we should be able to publish to it without subscribing. Will revert related changs in #919 accordingly.

@chaitanyaprem
Copy link
Collaborator

@fbarbu15 , closing this issue as the respective PR is merged.

Please retest and reopen if required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working E:REST API service node See https://github.com/waku-org/pm/issues/82 for details
Projects
Archived in project
Development

No branches or pull requests

2 participants