feat: Replace DIDCommMsg type with map[string]interface{} #1054

soluchok · 2020-01-10T17:43:07Z

The intention of this PR is to generalize all messages in the system/framework. Because our messages are JSON based messages the structure map[string]interface is enough to make messages with the same type. The benefit of this can be:

we cannot send an array/string etc.
the new message type provides common functions ID(),Type() etc.
message can be modified directly by key (without knowing the structure)

Since the message is a map, it can be modified by some other package/client. To prevent this we need to expose only the message interface. The interface provides Type and Scan functions. Having these functions any package/client may convert a message into a specific structure.

type DIDMsg interface {
	ID() string
	Type() string

	ThreadID() (string, error)
	Scan(v interface{}) error
}

The usage:
In our current logic, we send events to the client/user. DIDCommAction and StateMsg accordingly. In that case, we cannot expose the message to the client. Because we are using DIDCommMsg message in our services (background handling e.g Continue function). So, instead of exposing DIDCommMsg we should expose an interface DIDMsg.

Closes #1065 #1039

codecov · 2020-01-14T17:38:54Z

Codecov Report

Merging #1054 into master will decrease coverage by 0.06%.
The diff coverage is 94.8%.

@@            Coverage Diff             @@
##           master    #1054      +/-   ##
==========================================
- Coverage   91.14%   91.08%   -0.07%     
==========================================
  Files         102      102              
  Lines        6446     6469      +23     
==========================================
+ Hits         5875     5892      +17     
- Misses        311      314       +3     
- Partials      260      263       +3

Impacted Files	Coverage Δ
pkg/didcomm/protocol/introduce/models.go	`100% <ø> (ø)`	⬆️
pkg/didcomm/common/service/errors.go	`100% <ø> (ø)`	⬆️
pkg/didcomm/common/service/event.go	`100% <ø> (ø)`	⬆️
pkg/restapi/operation/common/msgservice.go	`92.59% <100%> (ø)`	⬆️
pkg/didcomm/protocol/didexchange/states.go	`94.87% <100%> (ø)`	⬆️
pkg/didcomm/protocol/route/service.go	`89.22% <100%> (ø)`	⬆️
pkg/framework/context/context.go	`98.26% <71.42%> (-1.74%)`	⬇️
pkg/didcomm/protocol/introduce/service.go	`91.48% <90%> (-0.66%)`	⬇️
pkg/didcomm/protocol/introduce/states.go	`95.6% <91.66%> (ø)`	⬆️
pkg/didcomm/common/service/did_comm_msg.go	`95.74% <95.74%> (ø)`
... and 2 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 f2b36a0...7f3d63b. Read the comment docs.

rolsonquadras · 2020-01-15T15:28:16Z

@soluchok It would be nice if we can describe the reason for this change. Currently, I don't see any in PR or Issue description.

pkg/didcomm/common/service/did_comm_msg.go

pkg/didcomm/protocol/didexchange/service.go

pkg/didcomm/common/service/did_comm_msg.go

sudeshrshetty · 2020-01-15T17:13:27Z

pkg/didcomm/common/service/did_comm_msg.go

+
+// ID returns the message id
+func (m DIDCommMsg) ID() string {
+	if m == nil || m[jsonID] == nil {


what if ID is ""?

The ID function will return the default value. Default is "".

Shouldn't it be invalid error if ID is empty?

Since the message is generic, it doesn't make sense to return an error. The empty type should be returned. We may provide the IsValid function to check the message if it is needed. For the functions which have some logic, I agree there should be an error. e.g ThreadID function. When we just want to get the direct value, it does not make sense to return an error.

sudeshrshetty · 2020-01-15T17:16:13Z

pkg/didcomm/common/service/did_comm_msg.go

+}
+
+// ToStruct converts msh message to  struct
+func (m DIDCommMsg) ToStruct(v interface{}) (err error) {


Just curious, why can't we use "https://github.com/mitchellh/mapstructure"?

mapstructure not maintainable anymore mitchellh/mapstructure#145 (comment)

I still think it's better than rolling our own code until we have a good reason to.

@soluchok @llorllale I believe the issue is that we need omit:empty and that feature hasn't been merged into mapstructure. correct?

yeah, but I think it is not a big problem

sudeshrshetty · 2020-01-15T17:21:28Z

pkg/framework/context/context.go

+				return err
+			}
+
+			if svc.Accept(h) {


Passing whole message may be risky to check acceptance criteria, for now add a TODO to pass subset of DIDComm msg map.

Accept may use an interface e.g

type Typer interface { Type() string }

We are also going to check it based on ~purpose.

@Moopli ^^^

pkg/didcomm/common/service/did_comm_msg.go

llorllale · 2020-01-15T18:27:12Z

pkg/didcomm/common/service/did_comm_msg.go

+	if !ok {
+		return ""
+	}


We're hiding an error condition here. Likewise in ID().

Not sure what to do. Adding an error return parameter would make the usage of Type() clumsy.

I think the best way is to return a default value.

NewDidCommMsg() should guarantee these base values exist.

#1054 (comment)

pkg/didcomm/common/service/did_comm_msg.go

llorllale · 2020-01-15T18:42:39Z

pkg/didcomm/protocol/didexchange/service.go

@@ -275,7 +273,7 @@ func (s *Service) handle(msg *message, aEvent chan<- service.DIDCommAction) erro
 		s.sendMsgEvents(&service.StateMsg{
 			ProtocolName: DIDExchange,
 			Type:         service.PostState,
-			Msg:          msg.Msg.Clone(),


It's interesting that we now lose this aspect (no longer exporting a copy).

I thought about it, I think instead of copy we may provide didcommmsg interface.

type IDIDCommMsg interface{ ID() Type() ToStrunct() }

copy a message is not a cheap operation. My suggestion is to provide an interface. I have not seen that we really use this message. Anyway providing an interface looks better and cheaper.

An interface or a struct is fine by me personally. Gophers would probably prefer it be a struct though.

An interface should only be returned if that is need to satisfy the signature of another interface.
Otherwise return a struct. The consuming code should accept the struct using an interface, of course.

Two public artifacts is overkill in this case. Either make it a public struct or a public interface + private struct. It's immutable and the method set [ID(), Type(), ThreadID(), Scan()] to me strikes the ideal balance between size, cohesion, and generality/usefulness.

pkg/didcomm/protocol/introduce/service.go

llorllale · 2020-01-15T18:51:05Z

@soluchok we lost the Clone() method. Rather than recreating it, I propose we make DIDCommMsg a struct and hide the map inside it. Leave ID(), ThreadID(), and ToStruct()/Scan() as they are. We get immutability for free this way + clients can scan to their specific struct with ToStruct()/Scan()

pkg/didcomm/protocol/introduce/models.go

troyronda · 2020-01-17T15:46:29Z

pkg/didcomm/common/service/did_comm_msg.go

+}
+
+// DIDCommMsg did comm msg
+type DIDCommMsg map[string]interface{}


This should probably not be in common/service anymore.

It is an implementation rather than a common interface being consumed by other packages.

Give me a hint of where to move it. pkg/common/didcommmsg ?

pkg/didcomm/common/service/event.go

pkg/didcomm/common/service/did_comm_msg.go

Signed-off-by: Andrii Soluk <isoluchok@gmail.com>

sudeshrshetty mentioned this pull request Jan 13, 2020

MessageService to use messenger api #1058

Open

2 tasks

soluchok changed the title ~~WIP: feat: Changed DIDCommMsg type to map[string]interface{}~~ Replace DIDCommMsg type with map[string]interface{} Jan 15, 2020

soluchok changed the title ~~Replace DIDCommMsg type with map[string]interface{}~~ feat: Replace DIDCommMsg type with map[string]interface{} Jan 15, 2020

rolsonquadras reviewed Jan 15, 2020

View reviewed changes

pkg/didcomm/common/service/did_comm_msg.go Show resolved Hide resolved

pkg/didcomm/protocol/didexchange/service.go Show resolved Hide resolved

sudeshrshetty reviewed Jan 15, 2020

View reviewed changes

troyronda requested review from llorllale and fqutishat January 15, 2020 17:32

llorllale reviewed Jan 15, 2020

View reviewed changes

pkg/didcomm/common/service/did_comm_msg.go Outdated Show resolved Hide resolved

llorllale reviewed Jan 15, 2020

View reviewed changes

pkg/didcomm/common/service/did_comm_msg.go Outdated Show resolved Hide resolved

llorllale reviewed Jan 15, 2020

View reviewed changes

pkg/didcomm/protocol/introduce/service.go Outdated Show resolved Hide resolved

troyronda reviewed Jan 17, 2020

View reviewed changes

pkg/didcomm/protocol/introduce/models.go Show resolved Hide resolved

troyronda reviewed Jan 17, 2020

View reviewed changes

pkg/didcomm/common/service/event.go Outdated Show resolved Hide resolved

baha-ai reviewed Jan 17, 2020

View reviewed changes

pkg/didcomm/common/service/did_comm_msg.go Outdated Show resolved Hide resolved

feat: Replace DIDCommMsg type with map[string]interface{}

7f3d63b

Signed-off-by: Andrii Soluk <isoluchok@gmail.com>

troyronda approved these changes Jan 17, 2020

View reviewed changes

sudeshrshetty approved these changes Jan 17, 2020

View reviewed changes

troyronda merged commit 9d11c5d into hyperledger-archives:master Jan 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Replace DIDCommMsg type with map[string]interface{} #1054

feat: Replace DIDCommMsg type with map[string]interface{} #1054

soluchok commented Jan 10, 2020 •

edited

codecov bot commented Jan 14, 2020 •

edited

rolsonquadras commented Jan 15, 2020 •

edited

sudeshrshetty Jan 15, 2020

soluchok Jan 16, 2020

sudeshrshetty Jan 16, 2020

soluchok Jan 17, 2020

sudeshrshetty Jan 15, 2020

soluchok Jan 16, 2020

llorllale Jan 16, 2020

troyronda Jan 16, 2020

soluchok Jan 17, 2020

sudeshrshetty Jan 15, 2020

soluchok Jan 16, 2020

sudeshrshetty Jan 16, 2020

troyronda Jan 17, 2020

llorllale Jan 15, 2020 •

edited

soluchok Jan 16, 2020

llorllale Jan 17, 2020

soluchok Jan 17, 2020

llorllale Jan 15, 2020

soluchok Jan 16, 2020

llorllale Jan 16, 2020

troyronda Jan 16, 2020 •

edited

llorllale Jan 17, 2020

llorllale commented Jan 15, 2020

troyronda Jan 17, 2020

soluchok Jan 17, 2020

feat: Replace DIDCommMsg type with map[string]interface{} #1054

feat: Replace DIDCommMsg type with map[string]interface{} #1054

Conversation

soluchok commented Jan 10, 2020 • edited

codecov bot commented Jan 14, 2020 • edited

Codecov Report

rolsonquadras commented Jan 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale Jan 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Jan 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soluchok commented Jan 10, 2020 •

edited

codecov bot commented Jan 14, 2020 •

edited

rolsonquadras commented Jan 15, 2020 •

edited

llorllale Jan 15, 2020 •

edited

troyronda Jan 16, 2020 •

edited