NicoNex / echotron

An elegant and concurrent library for the Telegram bot API in Go.
https://t.me/s/echotronnews
GNU Lesser General Public License v3.0
363 stars 26 forks source link

fix(chatreactiontype): proper interface unmarshalling #48

Closed heffcodex closed 3 months ago

heffcodex commented 3 months ago

Hi. There is another bug showed up after #47 fix. The fact is that ChatReactionType couldn't be unmarshalled from JSON as it is an interface. So all api responses that use this field type would fail to decode. I came up with this workaround with minimal changes to the existing design. IDK if it is acceptable, but I'd share this anyway ;)

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.

Project coverage is 75.38%. Comparing base (2d056a6) to head (bc2c847).

Files Patch % Lines
types.go 0.00% 19 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #48 +/- ## ========================================== - Coverage 76.45% 75.38% -1.08% ========================================== Files 17 17 Lines 1338 1357 +19 ========================================== Hits 1023 1023 - Misses 265 284 +19 Partials 50 50 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

NicoNex commented 3 months ago

Thank you for the bug report, I think it would be best to make ReactionType implement the interface json.Unmarshaler by composition and thus implementing the required method UnmarshalJSON([]byte) error. https://pkg.go.dev/encoding/json#Unmarshaler

type ReactionType interface {
    ImplementsReactionType()
    json.Unmarshaler
}
heffcodex commented 3 months ago

But it would still require using a wrapper struct for the ReactionType interface as interfaces in Go can't be decoded directly: https://go.dev/play/p/Cbnkzfho1dw

As an alternative, we could replace the ReactionType interface in responses by an all-in-one struct containing either all possible fields:

type ReactionType struct {
  Type string `json:"type"`
  Emoji string `json:"emoji"`
  CustomEmoji string `json:"custom_emoji"`
}

Or embed all possible subtypes in it:

type ReactionType struct {
  Type string `json:"type"`
  *ReactionTypeEmoji
  *ReactionTypeCustomEmoji
}

Obviously it could be also done with reflection, but I believe that is not the case where it is wanted.

NicoNex commented 3 months ago

Sorry for the late response, you're right we can't use the interface in the way I suggested, I think at this point the best way is the embedding you suggested:

type ReactionType struct {
  Type string `json:"type"`
  Emoji string `json:"emoji"`
  CustomEmoji string `json:"custom_emoji"`
}
heffcodex commented 3 months ago

@NicoNex I see that you have implemented the fix, could we close this PR then? And thank you :)

NicoNex commented 3 months ago

@NicoNex I see that you have implemented the fix, could we close this PR then? And thank you :)

Thank you for the bug report and PR, sure this can be closed. :) We'll release the new changes asap.