Skip to content

Conversation

teogeb
Copy link
Contributor

@teogeb teogeb commented Dec 3, 2024

This is a breaking change

Use StreamrClientError instead of StreamMessageError:

  • added streamMessage parameter to StreamrClientErrorCode
  • custom InvalidJsonError removed, using INVALID_MESSAGE_CONTENT error code instead
  • add and use new error codes:
    • INVALID_SIGNATURE
    • INVALID_PARTITION
    • ASSERTION_FAILED
      • This should be used when there is an internal mismatch about some pre-condition. E.g. when use use StreamMessage object, we can assume that StreamMessage instance has a valid value for the messageType property as all instances are created by our internal StreamMessageTranslator utility. We have a switch statement for handling different message types, and there we can throw an error with ASSERTION_FAILED code if messageType is not any of the valid message types.
  • use MISSING_PERMISSION error code

Removed StreamMessageError class.

The StreamrClientError class has an optional constructor parameter for specifying related StreamMessage. When that is used, the error message contains a string representation of the MessageID. The MessageID is also stored to the error object as a field to make it accessible programmatically.

  • StreamMessagError stored the whole StreamMessage object. Therefore e.g. the whole message content was exposed if the error was logged. This change resolves issue NET-1378.

Also code is now included in the error message. It is good to duplicate the code and messageId info in the error message as custom error fields are not shown e.g. in browser environment outputs.

Output in logs

In the NodeJS an error is written to the output like this:

StreamrClientError: Foobar (code=MISSING_PERMISSION, messageId={"streamId":"streamId","streamPartition":0,"timestamp":0,"sequenceNumber":0,"publisherId":"0x64e5f13fcc00a854e1d2d9def029aa18654eeff8","msgChainId":"msgChainId"})
    at ...
    at ...
    at ... {
  code: 'MISSING_PERMISSION',
  messageId: MessageID {
    streamId: 'streamId',
    streamPartition: 0,
    timestamp: 0,
    sequenceNumber: 0,
    publisherId: '0x64e5f13fcc00a854e1d2d9def029aa18654eeff8',
    msgChainId: 'msgChainId'
  }
}

In browser environments an error is written to the output like this:

Uncaught StreamrClientError: Foobar (code=MISSING_PERMISSION, messageId={"streamId":"streamId","streamPartition":0,"timestamp":0,"sequenceNumber":0,"publisherId":"0x9d95e67dcbc8db5534300347e8ed0820466c27e1","msgChainId":"msgChainId"})
    at ...
    at ...

Note that e.g. in NodeJS logs the messageId info is duplicated. In some other environments only the error message is visible, it is good to have the info also in the message.

Open questions

Maybe this is not a breaking changes as we've not documented our errors? I.e. this is functionality change but but an API change.

Note

The change implemented in 6c5f4fcc6 is maybe not needed at all? Should improve test coverage and maybe remove the ignoreMessages functionality (if the onError is never called with the StreamMessage parameter?)

Future improvements

  • Remove ValidationError and use StreamrClientError instead of that
  • Replace all anonymous errors (e.g. new Error('streamId required')) with StreamrClientError

@github-actions github-actions bot added the sdk label Dec 3, 2024
@teogeb teogeb changed the title feat(sdk): Use StreamrClientError instead of custom errors feat(sdk): Use StreamrClientError instead of StreamMessagError Dec 10, 2024
@teogeb teogeb marked this pull request as ready for review December 10, 2024 19:45
@teogeb teogeb requested a review from harbu December 10, 2024 19:45
@github-actions github-actions bot added the docs label Dec 12, 2024
@teogeb teogeb changed the title feat(sdk): Use StreamrClientError instead of StreamMessagError feat(sdk)!: Use StreamrClientError instead of StreamMessagError Dec 12, 2024
@teogeb teogeb merged commit b92326d into main Dec 12, 2024
24 checks passed
@teogeb teogeb deleted the sdk-replace-StreamMessageError branch December 12, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants