Adding error message to the History Responses #660

staheri14 · 2021-07-08T20:19:07Z

This closes vacp2p/rfc#248 and is incremental progress towards vacp2p/rfc#406.

I also utilized this PR to do some clean-up on the pagination logic as part of #484. Removed the private proc paginateWithoutIndex and shifted its code inside the findMessage proc for the sake of better code readability. Moreover, the paginateWithoutIndex had simple logic i.e., peeling off the Index from IndexedWakuMessages, so having it as a separate proc would only complicate the code. Following this change, I have renamed another existing proc i.e., paginateWithIndex to paginate.

Also, replaced the use of Result with the explicit return call (as suggested in track 1 checklist of the refactor week https://hackmd.io/1imOGULZRsed2HpgmzGleA).

… logic

status-im-auto · 2021-07-08T20:33:32Z

Jenkins Builds

Click to see older builds (3)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`b09a0b6`	#1	2021-07-08 20:33:32	~14 min	`macos`	📦`bin`
✔️	`b09a0b6`	#1	2021-07-08 20:37:45	~18 min	`linux`	📦`bin`
✔️	`b09a0b6`	#1	2021-07-09 10:06:16	~13 hr	`windows`	📦`exe`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`eef1567`	#2	2021-07-09 17:19:48	~10 min	`macos`	📦`bin`
✔️	`eef1567`	#2	2021-07-09 17:22:27	~12 min	`linux`	📦`bin`
✔️	`eef1567`	#2	2021-07-09 17:43:37	~33 min	`windows`	📦`exe`

✔️	`975dcb3`	#3	2021-07-13 16:12:35	~10 min	`macos`	📦`bin`
✔️	`975dcb3`	#3	2021-07-13 16:15:47	~13 min	`linux`	📦`bin`
✔️	`975dcb3`	#3	2021-07-13 17:09:01	~1 hr 6 min	`windows`	📦`exe`

oskarth · 2021-07-09T04:34:07Z

waku/v2/protocol/waku_store/waku_store.nim

@@ -299,20 +305,13 @@ proc paginateWithIndex*(list: seq[IndexedWakuMessage], pinfo: PagingInfo): (seq[
      newCursor = msgList[s].index # the new cursor points to the begining of the page

  if (retrievedPageSize == 0):
-    return (@[], PagingInfo(pageSize: 0, cursor:pinfo.cursor, direction: pinfo.direction))
+    return (@[], PagingInfo(pageSize: 0, cursor:pinfo.cursor, direction: pinfo.direction), HistoryResponseError.OK)


What is HistoryResponseError.OK? Seems like an oxymoron, isn't 0 pages retrieved, if correct, an OK response?

The goal of the error message is to distinguish between two cases that both result in 0 message history. 1) when the cursor is invalid 2) the cursor is valid but there is no match for the submitted query
When an error message is OK, it means the second scenario happened. Having zero messages in the response is not by itself an error, but if it is because of an invalid cursor then it is an error (as the input is invalid).

Following @jm-clius comment #660 (comment), I have renamed OK to NONE to resolve the naming ambiguity.

oskarth · 2021-07-09T04:34:28Z

waku/v2/protocol/waku_store/waku_store_types.nim

@@ -52,9 +52,15 @@ type
    startTime*: float64 # used for time-window query
    endTime*: float64 # used for time-window query

+  HistoryResponseError* {.pure.} = enum
+    ## HistoryResponseError contains error message to inform  the querying node about the state of its request
+    OK = uint32(0)


Why is this an error?

I'd suggest making this NONE and the default. That way we're not mixing "Success" and "Error" codes (which should be avoided) and don't have to use it other than for error conditions.

Sure, that makes sense. I will change its name to none!

OK means no error, and I agree that NONE makes more sense! @oskarth

oskarth · 2021-07-09T04:36:21Z

I might be missing some context, but to me:

In the current implementation of pagination, a history query with an invalid cursor (as part of the paging info) gets responded by an empty set of waku messages. Likewise, if the topics field of the history query does not match any of the historical messages, the returned set of messages is empty. The preceding cases are indistinguishable in the current implementation of store protocol thus the querying node has no clue as to why no waku messages is retrieved. (From vacp2p/rfc#248)

means we need give error for invalid cursors. If topic field doesn't match, then seems like an OK response and not an error? Perhaps informational, as in "no messages for topic". What do other standard APIs do in this scenario?

jm-clius · 2021-07-09T14:28:11Z

waku/v2/protocol/waku_store/waku_store.nim

@@ -245,7 +251,7 @@ proc findIndex*(msgList: seq[IndexedWakuMessage], index: Index): Option[int] =
      return some(i)
  return none(int)

-proc paginateWithIndex*(list: seq[IndexedWakuMessage], pinfo: PagingInfo): (seq[IndexedWakuMessage], PagingInfo) =
+proc paginate*(list: seq[IndexedWakuMessage], pinfo: PagingInfo): (seq[IndexedWakuMessage], PagingInfo, HistoryResponseError) =


Perhaps if this defaults to OK I imagine you wouldn't have to explicitly set HistoryResponseError.OK in so many places?

The default is OK, however, as part of the unit testing, I need to check that the different execution paths of paginate that yield OK behave as expected and do not produce an error.
Please let me know if there is a specific part of the code that the use of OK is unnecessary and can be avoided.

BTW, as I am using return to terminate the proc, I cannot leave the third output value unspecified.

waku/v2/protocol/waku_store/waku_store.nim

staheri14 · 2021-07-09T17:36:03Z

means we need give error for invalid cursors. If topic field doesn't match, then seems like an OK response and not an error? Perhaps informational, as in "no messages for topic". What do other standard APIs do in this scenario?

@oskarth
Our pagination method is similar to the one used in Slack https://api.slack.com/docs/pagination#cursors
The only error message associated with the pagination on the Slack is the same as ours i.e., invalid_cursor

I also checked Twitter pagination, it appears they do not consider any error message for invalid cursors/tokens https://developer.twitter.com/en/docs/twitter-api/pagination

staheri14 · 2021-07-09T17:37:46Z

@oskarth
I initially did not think of the query mismatch as an error, I can still add further error messages related to other fields of queries
1- the pubsub topic was not found
2- the time range was not found
3- the content topic was not found

Let me know if further error messages is desired

oskarth · 2021-07-10T05:28:40Z

waku/v2/protocol/waku_store/waku_store_types.nim

  HistoryResponse* = object
    messages*: seq[WakuMessage]
    pagingInfo*: PagingInfo # used for pagination
+    error*: HistoryResponseError


Maybe I'm still missing something, but if HistoryResponseError is NONE, couldn't that intent be communicated with error*: Option[HistoryResponsError]? Right now, the way I'm reading it, it seems like an ad-hoc implementation of Option type.

That way in case of empty result, error would simply be omitted. What am I missing with this approach?

In protobuf terms, that field would be emitted, so the equivalent thing (0) would go over the wire, I believe.

Maybe I'm still missing something, but if HistoryResponseError is NONE, couldn't that intent be communicated with error*: Option[HistoryResponsError]? Right now, the way I'm reading it, it seems like an ad-hoc implementation of Option type.

If we want the have error codes for a successful response, then the error field should not be optional. Otherwise, this can be converted to an optional field.

I suggest not to make the error field optional (although in protobuf fields are optional by default). The reason is to make sure that the error is not omitted by mistake rather the response has indeed been concluded successfully.

Let me know whether this makes sense or shall I convert it to optional (or make it a simple string error). I'll follow your discretion.

oskarth · 2021-07-10T05:34:01Z

See comment above re possibly using Option type, to me that seems cleaner but I might still be missing something here.

Let me know if further error messages is desired

Generally an extensible way of dealing with this is to have an optional error string, that way things can be elaborated on without a protocol upgrade. This helps with logs and debugging, for example if further down the line you want errors like "permission denied" "rate limiting reached (300/500)" or the "foobar not found" you suggested. It can be complemented with an error code, like 404, 500 etc in HTTP, for more automatic error handling but for now I think knowing 1) It is an error 2) Invalid cursor is good enough!

oskarth · 2021-07-10T05:37:57Z

As for what error are needed now, might make sense to get @richard-ramos @siphiuel POV in terms of what Core/Desktop needs

staheri14 · 2021-07-12T18:36:42Z

See comment above re possibly using Option type, to me that seems cleaner but I might still be missing something here.

Let me know if further error messages is desired

Generally an extensible way of dealing with this is to have an optional error string, that way things can be elaborated on without a protocol upgrade. This helps with logs and debugging, for example if further down the line you want errors like "permission denied" "rate limiting reached (300/500)" or the "foobar not found" you suggested. It can be complemented with an error code, like 404, 500 etc in HTTP, for more automatic error handling but for now I think knowing 1) It is an error 2) Invalid cursor is good enough!

Thanks for the comments @oskarth!

As in the case of the HTTP response status codes, there are plenty of options covering a successful response like 200 OK.
The reason I used error codes instead of error strings was to have a well-defined and precise set of error codes with exact definitions that can be consistently perceived by the two ends of the communication, I found error strings error-prone and a bit ad-hoc in this sense.
I agree that the error strings would be more extensible.

oskarth

Let's get this merged, if we want to tweak it at a later point it shouldn't be an issue. Thanks for the patience!

staheri14 · 2021-07-13T15:59:56Z

Let's get this merged, if we want to tweak it at a later point it shouldn't be an issue. Thanks for the patience!

Sure! thanks for the comments @oskarth , definitely please let me know if further updates would be required to make the error handling more effective and useful! :)

staheri14 added 5 commits July 8, 2021 12:14

adds error field to history response

0278413

unittest for protobuf enc/dec

d27fd56

renames HistoryResponseError to Store Error and simplifies pagination…

1676f66

… logic

reverts the naming of StoreError to HistoryResponseError

5ead320

fixes a unittest for invalid cursor

b09a0b6

staheri14 requested review from oskarth, jm-clius and EbubeUd July 8, 2021 20:36

oskarth reviewed Jul 9, 2021

View reviewed changes

jm-clius reviewed Jul 9, 2021

View reviewed changes

waku/v2/protocol/waku_store/waku_store.nim Outdated Show resolved Hide resolved

renames OK to NONE

eef1567

staheri14 requested review from oskarth and jm-clius July 9, 2021 17:18

oskarth reviewed Jul 10, 2021

View reviewed changes

staheri14 self-assigned this Jul 12, 2021

oskarth approved these changes Jul 13, 2021

View reviewed changes

jm-clius approved these changes Jul 13, 2021

View reviewed changes

Merge branch 'master' into history-queries-error-message

975dcb3

staheri14 merged commit 0758615 into master Jul 13, 2021

staheri14 deleted the history-queries-error-message branch July 13, 2021 19:01

staheri14 mentioned this pull request Jul 16, 2021

Adds error field to history response vacp2p/rfc#433

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding error message to the History Responses #660

Adding error message to the History Responses #660

staheri14 commented Jul 8, 2021 •

edited

Loading

status-im-auto commented Jul 8, 2021 •

edited

Loading

oskarth Jul 9, 2021

staheri14 Jul 9, 2021

staheri14 Jul 9, 2021

oskarth Jul 9, 2021

jm-clius Jul 9, 2021

staheri14 Jul 9, 2021

staheri14 Jul 9, 2021

oskarth commented Jul 9, 2021

jm-clius Jul 9, 2021

staheri14 Jul 9, 2021

staheri14 Jul 9, 2021

staheri14 commented Jul 9, 2021 •

edited

Loading

staheri14 commented Jul 9, 2021

oskarth Jul 10, 2021 •

edited

Loading

oskarth Jul 10, 2021

staheri14 Jul 12, 2021 •

edited

Loading

oskarth commented Jul 10, 2021 •

edited

Loading

oskarth commented Jul 10, 2021

staheri14 commented Jul 12, 2021 •

edited

Loading

oskarth left a comment

staheri14 commented Jul 13, 2021

Adding error message to the History Responses #660

Adding error message to the History Responses #660

Conversation

staheri14 commented Jul 8, 2021 • edited Loading

status-im-auto commented Jul 8, 2021 • edited Loading

Jenkins Builds

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

oskarth commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 commented Jul 9, 2021 • edited Loading

staheri14 commented Jul 9, 2021

oskarth Jul 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

oskarth commented Jul 10, 2021 • edited Loading

oskarth commented Jul 10, 2021

staheri14 commented Jul 12, 2021 • edited Loading

oskarth left a comment

Choose a reason for hiding this comment

staheri14 commented Jul 13, 2021

staheri14 commented Jul 8, 2021 •

edited

Loading

status-im-auto commented Jul 8, 2021 •

edited

Loading

staheri14 commented Jul 9, 2021 •

edited

Loading

oskarth Jul 10, 2021 •

edited

Loading

staheri14 Jul 12, 2021 •

edited

Loading

oskarth commented Jul 10, 2021 •

edited

Loading

staheri14 commented Jul 12, 2021 •

edited

Loading