Re: [Edgex-golang] JSON Marshal vs Decode

Goodell, Leonard
 

Could add a ValidateIsSet() func to OperatingState that the parents calls so it is sill responsibility/encapsulated in OperatingState.

 

From: Trevor.Conn@... <Trevor.Conn@...>
Sent: Friday, March 08, 2019 9:40 AM
To: Goodell, Leonard <leonard.goodell@...>; ian.johnson@...
Cc: edgex-tsc-core@...; EdgeX-GoLang@...
Subject: RE: [Edgex-golang] JSON Marshal vs Decode

 

Lenny – Yes, it does appear that is the mechanism preventing the UnmarshalJSON from being called. But I have two follow ups:

 

  1. Why does it work when calling Marshal()? I suspect it’s because there isn’t a pointer in the receiver and OperatingState has an empty string value on the parent type whereas during Unmarshal there isn’t a pointer to reference.
  2. I’m not a fan of putting the knowledge of “check for empty string” in all the types that have OperatingState as a member when the whole point of making OperatingState a composite literal type is to encapsulate that check. It may as well be a regular string in that case.

 

Good discussion.

 

Trevor

 

From: EdgeX-GoLang@... <EdgeX-GoLang@...> On Behalf Of Goodell, Leonard
Sent: Friday, March 8, 2019 10:20 AM
To: Conn, Trevor; ian.johnson@...
Cc: edgex-tsc-core@...; EdgeX-GoLang@...
Subject: Re: [Edgex-golang] JSON Marshal vs Decode

 

[EXTERNAL EMAIL]

So the issue is the custom validation can’t run if the incoming JSON does exist for that object, which makes sense. Seems to me it is then the responsibility of the parent object’s custom validation to handle this case, i.e. simple empty value check.

 

In this case have UnmarshalJSON() in models/provisionwatcher.go should check a.OperatingState for empty string or constant representing ‘empty’ before setting it.

 

Lenny

 

From: EdgeX-GoLang@... <EdgeX-GoLang@...> On Behalf Of Trevor.Conn@...
Sent: Thursday, March 07, 2019 4:58 PM
To: ian.johnson@...
Cc: edgex-tsc-core@...; EdgeX-GoLang@...
Subject: Re: [Edgex-golang] JSON Marshal vs Decode

 

We may be moving towards a better solution with Ian’s suggestion.

 

I have created a follow up branch here:

https://github.com/tsconn23/go-mod-core-contracts/tree/marshal_bug_ian

 

Only the contracts require changes in this case and, as Ian says below, the change is to add the Unmarshal function to the ProvisionWatcher where the receiver is a pointer. There’s still one problem though, suggestions are welcome.

 

This still doesn’t fix the bug. I can now step into the Unmarshal function, so we’re closer. But the related Unmarshal in the OperatingState is never called because it’s not supplied on the request – in either the ProvisionWatcher itself or the related DeviceService.

 

Here is the example request I am using to test. This comes from the core-metadata-importer collection from the blackbox tests.

 

POST http://localhost:48081/api/v1/provisionwatcher

{"_class":"org.edgexfoundry.domain.meta.ProvisionWatcher","name":"lonworks watcher test","identifiers":{"MAC":"00-05-1C-C1-99-99","HTTP":"10.0.0.4"},"origin":1471806386919,"profile":{"name":"variable speed motor profile"},"service":{"name":"home variable speed motor device service"}}

 

If you’re going to test this locally you will need to run the core-metadata-importer collection in your local environment first due to the Device Service relationship.

 

Trevor

 

P.S. Even with this, I still think having a more streamlined request makes sense. But if we can work this out, it could be a good middling step that we can implement for Edinburgh.

 

From: Ian Johnson <ian.johnson@...>
Sent: Thursday, March 7, 2019 5:04 PM
To: Conn, Trevor
Cc: edgex-tsc-core@...; EdgeX-GoLang@...
Subject: Re: [Edgex-golang] JSON Marshal vs Decode

 

[EXTERNAL EMAIL]

Hi Trevor, sorry for missing the call today so perhaps I'm missing some context here, but why doesn't the custom unmarshaller method get called for the type you are using? 

 

A problem I see with the solution you have in your branch is that you're unmarshalling the json and then you re-marshal it again just to get the custom error checking logic in your marshaller to run. This seems both inefficent and confusing.

 

I think that a better way to handle this is to re-define your custom Unmarshaller for a pointer to the type as that will cause it to run and be used. As you have it right now, it's I don't think it's being used because you call Decode you're using a pointer, and as such unmarshalling into a pointer, but the custom method you've defined is a method on a object directly and thus is ignored. However you will find if you try the trivial implementation for this Unmarshaller which is just to call json.Unmarshal on the data passed in to your custom unmarshaller method that you will enter an infinite loop. A good trick I've seen before is to use a type alias and an "anonymous struct" with struct embedding to run the generic unmarshaller on the data you're provided but in a way that it doesn't call the custom method you've defined. You can see an example of this here: https://gist.github.com/anonymouse64/bd0321890ce34ba39b92c536bc9080d0

I recommend this approach over the re-encoding method you have today.

 

Thanks,

Ian 

On Thu, Mar 7, 2019 at 12:43 PM <Trevor.Conn@...> wrote:

Hi all – I’m sending this email out as a follow up to today’s Core WG call. The recording of the call can be found on the wiki page and discussion pertinent to this topic starts at about the 30 minute mark.

 

https://wiki.edgexfoundry.org/display/FA/Core+Working+Group

 

The issue in a nutshell is that in large measure throughout the EdgeX Go services, we ingest and deserialize JSON requests in the following manner:

 

var e models.Event

dec := json.NewDecoder(r.Body)

err := dec.Decode(&e)

 

Or, alternatively stated

 

var pw models.ProvisionWatcher
json.NewDecoder(r.Body).Decode(&pw)

 

The mechanism used here to decode the request body from a stream containing JSON into a given type does NOT cause the custom unmarshaling logic present in many contracts to be called. This was proven and demo’ed on the call. In essence, this means we have an issue guaranteeing the integrity of requests received by all services in edgex-go that utilize the above pattern.

 

I presented a solution to this problem on the call. The depth to which we embrace the solution and the associated timeframe is TBD. If you wish to more closely review the code that I showed during the call, I have made that available via the following feature branches.

 

https://github.com/tsconn23/edgex-go/tree/marshal_bug

https://github.com/tsconn23/go-mod-core-contracts/tree/marshal_bug

 

If you are interested in this topic, I am happy to take any questions/comments via this email thread or Slack. Thanks.

 

Trevor Conn

Senior Principal Software Engineer

Dell Technologies | IoT DellTech

Trevor.Conn@...

Round Rock, TX USA

 

Join EdgeX-TSC-Core@lists.edgexfoundry.org to automatically receive all group messages.