Media Post Implementation #72

leobragaz · 2020-01-08T17:03:52Z

Description

This PR implements MediaPost and relative methods.

TODO:

Checklist

Targeted PR against correct branch.
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests.
Added an entry to the CHANGELOG.md file.
Re-reviewed Files changed in the Github PR explorer.

…nd media post to codec.go

updated codecs for post and media post implemented keeper methods for media post implemented strings type and some useful methods

codecov-io · 2020-01-09T17:10:32Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6f45ccb). Click here to learn what that means.
The diff coverage is 63.55%.

@@            Coverage Diff            @@
##             master      #72   +/-   ##
=========================================
  Coverage          ?   75.99%           
=========================================
  Files             ?       21           
  Lines             ?     1083           
  Branches          ?        0           
=========================================
  Hits              ?      823           
  Misses            ?      238           
  Partials          ?       22

Impacted Files	Coverage Δ
x/posts/internal/types/codec.go	`100% <ø> (ø)`
x/posts/internal/types/post_optional_data.go	`0% <0%> (ø)`
x/posts/internal/types/post.go	`0% <0%> (ø)`
x/posts/internal/types/querier.go	`0% <0%> (ø)`
x/posts/internal/keeper/handler.go	`95.61% <100%> (ø)`
x/posts/internal/keeper/keeper.go	`98.23% <100%> (ø)`
x/posts/internal/types/media_post.go	`81.41% <100%> (ø)`
x/posts/internal/types/post_response.go	`53.84% <100%> (ø)`
x/posts/internal/types/reaction.go	`82.85% <100%> (ø)`
x/posts/internal/types/text_post.go	`89.04% <78.12%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f45ccb...54c4795. Read the comment docs.

RiccardoM · 2020-01-10T08:16:51Z

@bragaz I've noticed that currently you've implemented the Provider support and MimeType checks.

I personally think we should follow what @kwunyeung suggested inside his comment on #72:

We cannot and not necessary to verify the mime types on-chain. It depends on how the applications work and specify it as the uploading and rendering parts won't be done on-chain.

And

[...] Storing the mime types are for the applications to know how the files should be rendered. So I don't think the Provider is necessary in any cases.

This means that there is no need for on-chain verification of mime types verification nor provider support at all.

Do you think they should be kept? If so, why?

leobragaz · 2020-01-10T08:34:44Z

Yes I was referring to your comment in #36 :

Such lists can be used as reference to know what providers and MIME types are supported.
They can also be used by the governance to add a new provider or a new MIME type to the supported list

I Miss @kwunyeung 's one and he's totally right, the apps should take care of that. This will also keep chain cleaner and clients could implement as many providers they want.

edited all method in keeper, handler and querier to fit the new models todo: Fix tests

- edited keeper_test, handler_test, querier_test to fit the new Post Interface - edited Post response to fit the new Post Interface - updated codec - added new tests for media posts TODO: - complete MsgCreateMediaPost tests - edit CLI command to let user make media posts

todo: - fix bugs in marshalJSON, unmarshalJSON

golangcibot · 2020-01-14T17:57:47Z

x/posts/internal/types/media_post_test.go

+package types_test
+
+import (
+ "fmt"


File is not goimports-ed (from goimports)

Suggested change

"fmt"

"fmt"

"testing"

leobragaz · 2020-01-14T18:00:02Z

I've been struggling all day with MarshalJSON() UnmarshalJSON() methods of MediaPost and I wasn't able to find any good implementation, @RiccardoM do you have any suggestions on how to do these method starting by String()?

RiccardoM · 2020-01-14T18:44:29Z

I've been struggling all day with MarshalJSON() UnmarshalJSON() methods of MediaPost and I wasn't able to find any good implementation, @RiccardoM do you have any suggestions on how to do these method starting by String()?

You are doing the exact opposite of what should be done. If you take a look at the TextPost, I have:

Overridden the MashalJson with a custom implementation
Overridden the String method with a custom implementation that calls the MarshalJson method

You are overriding the MarshalJson calling the String method, which will not work at all as the String method returns just the class name by default.

So the good implementation should be something like

func (mp MediaPost) MarshalJson() ([]byte, err) {
  type mediaPostJson MediaPost
  return json.Marshal(mediaPostJson(mp))
}

func (mp MediaPost) String() string {
  json, err := json.Marshal(&mp) 
  if err != nil {  
    panic(err)
  }
  return string(json)
}

Actually, it's not needed to use the json.Marshal from the String method and is actually not the best option to be honest.

We should change it inside TextPost too, so that the String method returns less data in a plain format, much like it does for the Posts type. It should be better to open a new issue for this.

Edit

For future reference, you can look at existing code. For example, you could have found the implementation for:

MarshalJson inside the post_response.go file
String inside the post.go file

leobragaz · 2020-01-14T21:06:03Z

I was doing that way since I discovered through the String() test that somehow, it was serializing the media post without the Medias, so just the first object of MediaPost. After that, I thought that it was because it was an array and needed to be handled differently...and so i tried to implement everything like you saw.

RiccardoM · 2020-01-15T06:10:38Z

I was doing that way since I discovered through the String() test that somehow, it was serializing the media post without the Medias, so just the first object of MediaPost. After that, I thought that it was because it was an array and needed to be handled differently...and so i tried to implement everything like you saw.

I would prefer we stick to an implementation like the one I've suggested, and we later discuss any problem that might raise individually once we have the tests setup and failing.

Please don't change the implementations completely just because tests are failing, and do not remove the tests too. Keep them as it might be useful to understand what's wrong.

leobragaz · 2020-01-15T13:37:33Z

Besides the conflicts, we need to understand why the marshalling of MediaPost don't include/overwrite the medias part. All failing tests are about it.
MediaPost TextPost value is marshalled correctly, but it's the only one.
E.g.
With the String() method implemented as follow:

// String implements fmt.Stringer
func (pm PostMedia) String() string {
	bytes, err := json.Marshal(&pm)
	if err != nil {
		panic(err)
	}

	return string(bytes)
}

The result of the function's test is this:

{
  "id": "2",
  "parent_id": "0",
  "message": "media Post",
  "created": "0",
  "last_edited": "0",
  "allows_comments": false,
  "subspace": "desmos",
  "creator": "cosmos1cjf97gpzwmaf30pzvaargfgr884mpp5ak8f7ns"
}

And it's wrong because I think we need to get something like:

{
  "post": {
    "id": "2",
    "parent_id": "0",
    "message": "media Post",
    "created": "0",
    "last_edited": "0",
    "allows_comments": false,
    "subspace": "desmos",
    "creator": "cosmos1cjf97gpzwmaf30pzvaargfgr884mpp5ak8f7ns"
  },
  "medias": [
    {
      "provider": "ipfs",
      "uri": "uri",
      "mime_Type": "text/plain"
    }
  ]
}

I've tried to implement String() as follow:

func (mp MediaPost) String() string {
	bytes, err := mp.TextPost.MarshalJSON()
	bytes2, err := json.Marshal(&mp.Medias)
	bb := append(bytes[:], bytes2[:]...)
	if err != nil {
		panic(err)
	}
	return string(bb)
}

It isn't printing a beautiful result but at least print medias...I really don't understand why this is happening..maybe I'm missing something! Let me know.

This problem is the same in PostQueryResponse.

RiccardoM · 2020-01-15T13:55:56Z

@bragaz Please pull from master to solve the existing conflicts

…eonardo/media-posts-impl � Conflicts: � x/posts/client/cli/tx.go � x/posts/client/rest/tx.go � x/posts/internal/keeper/common_test.go � x/posts/internal/keeper/handler.go � x/posts/internal/keeper/handler_test.go � x/posts/internal/keeper/keeper.go � x/posts/internal/keeper/keeper_test.go � x/posts/internal/keeper/querier_test.go � x/posts/internal/types/msgs.go � x/posts/internal/types/msgs_test.go � x/posts/internal/types/post.go � x/posts/internal/types/post_response_test.go � x/posts/internal/types/text_post_test.go � x/posts/legacy/v0.2.0/migrate.go � x/posts/legacy/v0.2.0/types.go

RiccardoM · 2020-01-16T09:02:20Z

@bragaz I had the time to look deeper into this, and I have found something really interesting.

How JSON encoding works and how to fix the problem

Currently we have the following type inheritances:

  Post 
   ^
TextPost
   ^
MediaPost

Both TextPost and MediaPost implement MarshalJSON. What happens is that during the call of json.Marshal(m), Go performs a lookup of the MarshalJson method in a top-down fashion starting from Post.

What does this mean? It means that it stops as soon as one type implements MarshalJson and uses that implementation. In our cases, it stopped at TextPost, which resulted in every MediaPost being serialized as a TextPost object instead.

By removing the implementation of both MarshalJSON and UnmarshalJSON from TextPost, it starts working properly again. The results are:

var m MediaPost = ...
json.Marshal(m)

// {"id":"2","parent_id":"0","message":"media Post","created":"2020-01-01T15:15:00Z","last_edited":"0001-01-01T00:00:00Z","allows_comments":false,"subspace":"desmos","creator":"cosmos1cjf97gpzwmaf30pzvaargfgr884mpp5ak8f7ns","medias":[{"uri":"uri","provider":"provider","mime_Type":"text/plain"}]}

It is completely fine to remove both MarshalJSON and UnmarshalJSON from TextPost as it has no custom implementation that Amino does not handle properly.

However, we need to implement those methods into MediaPost as it encapsulates TextPost and Amino does not handle very well encapsulation.

Changes in the serialization fashion

Looking at this, it made me think about the best way we can handle all of this properly. What we could do is the following.

JSON serialized posts that extend TextPost should add to it the new fields. This means overriding the MarshalJSON and UnmarshalJSON for each extending post type.

This will result in MediaPosts being serialized as follows:

{
  "id": "2",
  "parent_id": "0",
  "message": "media Post",
  "created": "2020-01-01T15:15:00Z",
  "last_edited": "0001-01-01T00:00:00Z",
  "allows_comments": false,
  "subspace": "desmos",
  "creator": "cosmos1cjf97gpzwmaf30pzvaargfgr884mpp5ak8f7ns",
  "medias": [
    {
      "uri": "uri",
      "provider": "provider",
      "mime_Type": "text/plain"
    }
  ]
}

As you can see, we do not encapsulate the TextPost into a "post" object, but we still have the new "medias" array.

This results in a more clean serialization overall.

Problems with this approach

Thinking ahead of time, this solution might however bring some new problems when we will start to implement poll posts (#14) too.

What would happen if a users wants to create a poll having an image inside?

This should result in a hybrid type that inherits from both MediaPost and PollPost, but as they will have both overridden the MarshalJSON method, one of the two will be chosen instead of the new one.

However, we can think this thought later, but it is still important to now it now.

- fixed tests

- implemented stringer String() method for types.Posts

RiccardoM

I've reviewed just the implementations and not the tests. Will review the latter once the issues and comments on the first have been completely solved.

CHANGELOG.md

x/posts/client/cli/tx.go

x/posts/legacy/v0.1.0/types.go

x/posts/alias.go

x/posts/internal/types/codec.go

x/posts/client/cli/tx.go

x/posts/client/rest/tx.go

x/posts/internal/types/media_post.go

x/posts/internal/types/codec.go

leobragaz · 2020-01-17T16:30:15Z

Removing

cdc.RegisterInterface((*Post)(nil), nil)

Will result in some errors in tests and handler because it's an unregistered interface.

- waiting for removing provider

RiccardoM · 2020-01-20T06:37:18Z

x/posts/client/cli/tx.go

+ arg := strings.Split(args[i], ",")
+ if len(arg) == 3 {
+ media := types.NewPostMedia(arg[0], arg[1], arg[2])
+ medias = append(medias, media)


What would happen if the user specifies two times the same media data? Should we use something like appendIfMissing instead of append here?

I was thinking about that when I was implementing it...it is good to forbid user to post the same media more times? I mean, I can post the same GIF two times, but that will not be the same for a video or a PDF...so yeah, we should use appendIfMissing instead.

@bragaz Using appendIfMissing here means that we are not allowing to use the same media inside the same post. E.g., the user will not be able to create a post having the same exact GIF, PDF, or file in it.

This is correct, and does not refer to creating two posts with the same media inside. This means

Alice creating post P1 with media M1 will be allowed

Alice creating post P1 with media M1 and M2 will be allowed

Alice creating post P1 with media M1 and M1 (again) will not be allowed

Alice creating post P1 with media M1 and post P2 with the same media M1 will be allowed

RiccardoM · 2020-01-20T06:39:00Z

x/posts/client/cli/tx.go

+ return sdk.ErrUnknownRequest("If medias are present, you should specify uri, provider and mime type, if you are confused, please use the --help flag")
+ }
+ }
+ if textMsg, ok := msg.(types.MsgCreateTextPost); ok {


This check will always succeed, can we simply delete it and replace it with something like the following?

msg = types.NewMsgCreateMediaPost(MsgCreateTextPost(msg), medias))

RiccardoM · 2020-01-20T06:43:35Z

x/posts/client/rest/tx.go

+ addr, req.CreationTime)
+
+ if len(req.Medias) != 0 {
+ if textMsg, ok := msg.(types.MsgCreateTextPost); ok {


This check will always succeed. Can we replace it with something like the following?

msg = types.NewMsgCreateMediaPost(MsgCreateTextPost(msg), req.Medias)

Co-Authored-By: Riccardo Montagnin <riccardo.montagnin@gmail.com>

leobragaz · 2020-01-20T14:51:03Z

Following the discussion made about Interfaces and types, I'm closing this PR in order to open another one with the following changes:

Remove all the interfaces
A single Post implementation as follow:

type Post struct {
  // All TextPost's field
  Medias []Media
}
type MsgCreatePost {
  // All TextPost's field
  Medias []Media
}

implemented media post type and methods, added registration of post a…

2ed386e

…nd media post to codec.go

leobragaz self-assigned this Jan 8, 2020

Bragaz added 2 commits January 9, 2020 11:26

fixed regEx

b942423

implemented message for media post

f5f2d4c

updated codecs for post and media post implemented keeper methods for media post implemented strings type and some useful methods

Bragaz added 4 commits January 10, 2020 17:00

implemented Interface Post inherited by TextPost and MediaPost

92d9c57

edited all method in keeper, handler and querier to fit the new models todo: Fix tests

correct things after make all runs

279ed34

- almost completed tests

5976a15

todo: - fix bugs in marshalJSON, unmarshalJSON

golangcibot reviewed Jan 14, 2020

View reviewed changes

Bragaz added 2 commits January 15, 2020 11:08

- completed tests

632fa58

- updated CLI command

e19ebc5

leobragaz marked this pull request as ready for review January 15, 2020 13:26

Bragaz added 3 commits January 15, 2020 15:01

updated aliases

6f45ccb

- solved all conflicts with master

e19cfa2

leobragaz requested a review from RiccardoM January 16, 2020 08:50

Bragaz added 4 commits January 16, 2020 11:03

- solved marshal, unmarshal problems

6c0c842

- fixed tests

- fixed errors in handler test

28350e5

- updated CHANGELOG.md

64c3fdf

- fixed query matchingPosts type that has to be an interface

fef8612

- implemented stringer String() method for types.Posts

Bragaz added 4 commits January 16, 2020 11:35

- fixed lint error

5521156

- updated rest tx to handle the media

1af4ed7

- fixed lint errors

1ddf419

- fixed some minor errors after reviewing changed files

050056e

RiccardoM suggested changes Jan 17, 2020

View reviewed changes

RiccardoM added the kind/new-feature Propose the addition of a new feature that does not yet exist label Jan 17, 2020

RiccardoM added this to the v0.3.0 milestone Jan 17, 2020

- made requested corrections

cb560ca

leobragaz requested a review from RiccardoM January 17, 2020 14:17

RiccardoM suggested changes Jan 17, 2020

View reviewed changes

- made requested corrections

f50fb53

- waiting for removing provider

RiccardoM suggested changes Jan 20, 2020

View reviewed changes

leobragaz and others added 3 commits January 20, 2020 09:32

Update x/posts/internal/types/media_post.go

108a006

Co-Authored-By: Riccardo Montagnin <riccardo.montagnin@gmail.com>

Update x/posts/internal/types/post_response.go

d54ec25

Co-Authored-By: Riccardo Montagnin <riccardo.montagnin@gmail.com>

- fixed requested corrections

54c4795

leobragaz closed this Jan 20, 2020

leobragaz mentioned this pull request Jan 21, 2020

Media post implementation #81

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media Post Implementation #72

Media Post Implementation #72

leobragaz commented Jan 8, 2020 •

edited

Loading

codecov-io commented Jan 9, 2020 •

edited

Loading

RiccardoM commented Jan 10, 2020

leobragaz commented Jan 10, 2020 •

edited

Loading

golangcibot Jan 14, 2020

leobragaz commented Jan 14, 2020 •

edited

Loading

RiccardoM commented Jan 14, 2020 •

edited

Loading

leobragaz commented Jan 14, 2020

RiccardoM commented Jan 15, 2020

leobragaz commented Jan 15, 2020

RiccardoM commented Jan 15, 2020

RiccardoM commented Jan 16, 2020

RiccardoM left a comment

leobragaz commented Jan 17, 2020

RiccardoM Jan 20, 2020

leobragaz Jan 20, 2020

RiccardoM Jan 20, 2020

RiccardoM Jan 20, 2020

RiccardoM Jan 20, 2020

leobragaz commented Jan 20, 2020

- "fmt"
+ "fmt"
+ "testing"

Media Post Implementation #72

Media Post Implementation #72

Conversation

leobragaz commented Jan 8, 2020 • edited Loading

Description

Checklist

codecov-io commented Jan 9, 2020 • edited Loading

Codecov Report

RiccardoM commented Jan 10, 2020

leobragaz commented Jan 10, 2020 • edited Loading

golangcibot Jan 14, 2020

Choose a reason for hiding this comment

leobragaz commented Jan 14, 2020 • edited Loading

RiccardoM commented Jan 14, 2020 • edited Loading

Edit

leobragaz commented Jan 14, 2020

RiccardoM commented Jan 15, 2020

leobragaz commented Jan 15, 2020

RiccardoM commented Jan 15, 2020

RiccardoM commented Jan 16, 2020

How JSON encoding works and how to fix the problem

Changes in the serialization fashion

Problems with this approach

RiccardoM left a comment

Choose a reason for hiding this comment

leobragaz commented Jan 17, 2020

RiccardoM Jan 20, 2020

Choose a reason for hiding this comment

leobragaz Jan 20, 2020

Choose a reason for hiding this comment

RiccardoM Jan 20, 2020

Choose a reason for hiding this comment

RiccardoM Jan 20, 2020

Choose a reason for hiding this comment

RiccardoM Jan 20, 2020

Choose a reason for hiding this comment

leobragaz commented Jan 20, 2020

leobragaz commented Jan 8, 2020 •

edited

Loading

codecov-io commented Jan 9, 2020 •

edited

Loading

leobragaz commented Jan 10, 2020 •

edited

Loading

leobragaz commented Jan 14, 2020 •

edited

Loading

RiccardoM commented Jan 14, 2020 •

edited

Loading