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.

Sign up for GitHub

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

fix(feed): Properly get reactions from Posts endpoint #1396

Merged
merged 2 commits into from
Nov 21, 2024
Merged

fix(feed): Properly get reactions from Posts endpoint #1396

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97c83c0
fix(feed): Properly get reactions from Posts endpoint
WaDadidou Nov 18, 2024
f1bdc82
fix(feed): Unmarshal instead of Scan in handleExecuteReactPost in ind…
WaDadidou Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion go/pkg/feed/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,13 @@ func (s *FeedService) Posts(ctx context.Context, req *feedpb.PostsRequest) (*fee
posts := make([]*feedpb.Post, len(dbPostWithExtras))
for idx, dbPost := range dbPostWithExtras {
var reactions []*feedpb.Reaction
reactionsMap := map[string]interface{}{}

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)
Copy link
Collaborator

@n0izn0iz n0izn0iz Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@WaDadidou WaDadidou Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't work in indexer too :/

This post => https://app.teritori.com/feed/post/testori-caecb9c3-1a35-429b-be30-54c0578c8ec4

Service

image

Indexer

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@n0izn0iz should I make the same fix in the indexer ?
And removing the Scan usage

Copy link
Collaborator Author

@WaDadidou WaDadidou Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@n0izn0iz n0izn0iz Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, so is it ok ? f1bdc82

Copy link
Collaborator

Choose a reason for hiding this comment

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

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:
Screenshot 2024-11-20 at 10 14 15 PM

so IMHO, the fix is valid, it can work but for the proper solution you should check the types change.

Copy link
Collaborator Author

@WaDadidou WaDadidou Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

for icon, users := range reactionsMap {
ownState := false
Expand Down
Loading