fix(feed): Properly get reactions from Posts endpoint #1396
Conversation
WaDadidou
added
bug
WaDadidou
requested review from
hthieu1110,
n0izn0iz,
omarsy,
Villaquiranm and
MikaelVallenet
✅ Deploy Preview for teritori-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for testitori ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
go/pkg/feed/service.go
Outdated
| reactionsMap := make(map[string]interface{}) | ||
| if err := json.Unmarshal(dbPost.UserReactions, &reactionsMap); err != nil { | ||
| s.conf.Logger.Error("failed to unmarshal UserReactions", zap.String("data", string(dbPost.UserReactions)), zap.Error(err)) | ||
| continue | ||
| } | ||
|
|
||
| dbPost.UserReactions.Scan(&reactionsMap) |
There was a problem hiding this comment.
we shouldn't keep both Unmarshal and Scan
this scan is also used in the indexer so it's weird that it doesn't work here
There was a problem hiding this comment.
It doesn't work in indexer too :/
This post => https://app.teritori.com/feed/post/testori-caecb9c3-1a35-429b-be30-54c0578c8ec4
Service
Indexer
There was a problem hiding this comment.
@n0izn0iz should I make the same fix in the indexer ?
And removing the Scan usage
There was a problem hiding this comment.
This code seems to not working correctly: https://github.com/TERITORI/teritori-dapp/blob/fix-post-reactions-from-feed/go/internal/indexerhandler/feed.go#L107-L141
go/pkg/feed/service.go
Outdated
| reactionsMap := make(map[string]interface{}) | ||
| if err := json.Unmarshal(dbPost.UserReactions, &reactionsMap); err != nil { | ||
| s.conf.Logger.Error("failed to unmarshal UserReactions", zap.String("data", string(dbPost.UserReactions)), zap.Error(err)) | ||
| continue | ||
| } | ||
|
|
||
| dbPost.UserReactions.Scan(&reactionsMap) |
There was a problem hiding this comment.
We should handle the returned error here: dbPost.UserReactions.Scan(&reactionsMap)
if Scan does not work I think there is an error in the data (seems that empty map causes the error), if there data is corrupt then we should check from the source, if not then maybe .Scan does not work for empty data. We should use either unmarshal or scan but not both.
There was a problem hiding this comment.
I think the problem with scan comes from the change of type of from the custom jsonb encoder to the datatypes.JSON gorm type, we should use the gorm type IMO
Also the scan error should have been checked in the first place, I will add golangcilint checks to avoid that in the future
There was a problem hiding this comment.
Ok, so is it ok ? f1bdc82
There was a problem hiding this comment.
I think json.Unmarshal will just unmarshal the data with defined struct so as long as it's compatible, it will work, that's the reason why the code works with json.Unmarshal (Scan might do some type check also so it does not work) because the root cause might be related to this:
so IMHO, the fix is valid, it can work but for the proper solution you should check the types change.
There was a problem hiding this comment.
@hthieu1110 I don't know what to do when I see these 2 types changes and this TODO:
https://github.com/TERITORI/teritori-dapp/blob/fix-post-reactions-from-feed/go/internal/indexerdb/jsonb.go#L10
There was a problem hiding this comment.
Btw, look at this TODO: https://github.com/TERITORI/teritori-dapp/blob/fix-post-reactions-from-feed/go/internal/indexerdb/jsonb.go#L10
If we fix the TODO, UserReactions gets the type datatypes.JSON finally
|
LGTM |
Closes #1374