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

Add option to detect unmapped error #1086

Merged
merged 2 commits into from Jul 21, 2022

Conversation

mszostok
Copy link
Contributor

@mszostok mszostok commented Jul 4, 2022

Description

This pull request adds:

  • dedicated type error for UnmappedError
  • unit tests coverage

Reason

The library uses the UnmarshallingErrorEvent error in two cases:

  1. when there was a real json.Unmarshal
  2. or when there was no corresponding Go struct for unmarshal process

We want to be able to check what kind of error we got. As a result, we will be able to do the assertion whether we care about this unmarshal error or not. Example snippet:

// handlingEvents holds event names for which we have corresponding business logic.
var handlingEvents = sets.NewString("message", "connected")

// ...trimmed...

switch ev := msg.Data.(type) {
case *slack.UnmarshallingErrorEvent:
  err, ok := ev.ErrorObj.(slack.UnmappedError)
  if ok && !handlingEvents.Has(err.EventType()) { // it's not an err for us, as we don't care about this event type.
    b.log.Debugf("Slack unmarshalling error: %+v", ev.Error())
    continue
  }

  b.log.Errorf("Slack unmarshalling error: %+v", ev.Error())
}

Currently, we have in logs:

ERRO[2022-06-15T11:30:02Z] Slack unmarshalling error: RTM Error: Received unmapped event "user_status_changed"

We could do the error string assertion but it's not a good practice, so we want to avoid that.

Implementation
I decided to leave the UnmarshallingErrorEvent as it is, but instead change

err := fmt.Errorf("RTM Error: Received unmapped event %q", typeStr)

to typed error. As a result, is not a breaking change.

Copy link
Member

@kanata2 kanata2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented on one point, but LGTM.

websocket_managed_conn.go Outdated Show resolved Hide resolved
Copy link
Member

@kanata2 kanata2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@kanata2 kanata2 merged commit 5a6b1b0 into slack-go:master Jul 21, 2022
@mszostok mszostok deleted the unmapped-typed-err branch July 25, 2022 10:22
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.

None yet

2 participants