Date   
Re: Core Contract Model Validators

Trevor.Conn@...
 

Another update on this –

 

This week in the #core channel on Slack Ian Johnson asked me why the approach below didn’t involve creating a Validator interface and using reflection to go through the members of a type, calling Validate() on those that implemented the interface. As those on the last Core WG call will remember, this was the original idea I proposed. So since Ian effectively +1’ed my original proposal I decided to implement it for a side by side comparison.

 

The changes only needed to be made to the core-contracts module and can be found here:

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

 

This is the first Go-based reflection code I’ve written, so there could be other ways to do this. However, I used the JSON Unmarshaler implementation from the standard library as an example.

 

With the current structure of our requests, it’s hard to know where to invoke the validation from. In this example, the DeviceService is the top-level struct so I start the process here. However one could argue that an Addressable should be able to validate independently since we can add/update an Addressable on its own. That would mean the Addressable would end up being validated twice in this case. Since we pass a deeply nested multii-purpose model around instead of Request types specific to the API endpoint that could be used for this kind of control, it probably is what it is unless we refactor.

 

Also, w/r/t recursion, I’d look to accomplish that through the Validate() functions at each node in the graph. So DeviceService calls Validate() on all its sub-types where applicable, then those sub-types would do the same followed by their sub-types and so on.

 

Let me know your thoughts. We’ll probably be talking about this at this week’s meeting.

 

Trevor

 

 

From: Conn, Trevor
Sent: Saturday, March 30, 2019 4:27 PM
To: EdgeX-GoLang@...; edgex-tsc-core@...
Subject: Core Contract Model Validators

 

Hi all -- I wanted to follow up on this discussion we've been having in the Core Working Group about validating our contract models and where that responsibility lies. This past week we talked about two options.

 

1.) Using a JSON Schema-based validation

2.) Creating a validator implementation that can be called as part of the UnmarshalJSON operation found in some of our models.

 

I explored the JSON Schema solution a bit on Thursday afternoon with help from Intel, but I did not find it palatable. The schema has to be created and maintained then loaded at runtime from either URL, file or inline string. Child types have their schema, but that schema must be either be repeated in the schema of the parent type or referenced via a pointer. This all seemed like too much work so before getting seriously into it, I decided to work on the validator idea.

 

I created a couple feature branches which you can review/pull at the links below. This involves changes to both core-contracts and edgex-go.

 

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

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

 

Rather than ProvisionWatcher I have used DeviceService in my example above because it already had an UnmarshalJSON() method in place, and also with the ProvisionWatcher I'd have to account for DeviceProfile as well. I just wanted to get something up and going to try and prove the concept, so the scope was more reduced by using DeviceService.

 

Here's a sample request I'm using to test:

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

{"_class":"org.edgexfoundry.domain.meta.DeviceService","adminState":"unlocked","operatingState":"disabled1","name":"ds_test1","description":"manage home variable speed motors","lastConnected":0,"lastReported":0,"labels":["hvac","variable speed motor"],"origin":1471806386919}

 

In the request body above there are two issues. First, the operatingState value is invalid. Second, the Addressable is missing. If you execute the above locally, you'll receive a 400 (Bad Request) with the error message "invalid OperatingState 'disabled1'".

If you fix the OperatingState value, then you'll still receive a 400 with a new error message "Addressable ID and Name are both blank". This is coming from the validator inside of the Addressable model within core-contracts. I noticed that we'll have the opportunity to slim our controller logic by moving the server-side validation we do today into the models. We'll also have consistent state validation (example: validation today is different depending on if you're adding a new Device Service versus updating an existing one).

 

The order of operations with regard to unmarshaling and validation looks like this

  • DeviceService
    • UnmarshalJSON
      • OperatingState (if provided in the request)
        • UnmarshalJSON
        • validate()
      • AdminState (if provided in the request)
        • UnmarshalJSON
        • validate()
    • validate()
      • OperatingState
        • validate()
      • AdminState
        • validate()

Herein is the only thing that gives me minor heartburn about the solution. The validate() method of both OperatingState and AdminState are called as part of their respective UnmarshalJSON implementations. It doesn't have to be but I thought it made sense to do so since it allows us to fail faster if we detect a problem.

 

Assuming those pass, then DeviceService calls its own validate() method which takes care of validating all child types. This is necessary in case a child type's key was omitted in the JSON. This means validation for any of the child types can occur twice -- a cheap operation to be sure, but redundant at times.

 

As an aside, you will see changes in the feature branches related to the elimination of models.Service from core-contracts. Nothing else was composed from this model other than the DeviceService and I felt that eliminating it would simplify the solution.

 

Comments are welcome. If this looks like a viable approach to folks, then there might be some bandwidth soon to undertake adding UnmarshalJSON to the models that do not have them, and implementing the necessary validate() handlers.

 

Trevor Conn
Technical Staff Engineer
Dell Technologies | IoT DellTech
Trevor_Conn@...
Round Rock, TX  USA

Core Contract Model Validators

Trevor.Conn@...
 

Hi all -- I wanted to follow up on this discussion we've been having in the Core Working Group about validating our contract models and where that responsibility lies. This past week we talked about two options.


1.) Using a JSON Schema-based validation

2.) Creating a validator implementation that can be called as part of the UnmarshalJSON operation found in some of our models.


I explored the JSON Schema solution a bit on Thursday afternoon with help from Intel, but I did not find it palatable. The schema has to be created and maintained then loaded at runtime from either URL, file or inline string. Child types have their schema, but that schema must be either be repeated in the schema of the parent type or referenced via a pointer. This all seemed like too much work so before getting seriously into it, I decided to work on the validator idea.


I created a couple feature branches which you can review/pull at the links below. This involves changes to both core-contracts and edgex-go.


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

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


Rather than ProvisionWatcher I have used DeviceService in my example above because it already had an UnmarshalJSON() method in place, and also with the ProvisionWatcher I'd have to account for DeviceProfile as well. I just wanted to get something up and going to try and prove the concept, so the scope was more reduced by using DeviceService.


Here's a sample request I'm using to test:

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

{"_class":"org.edgexfoundry.domain.meta.DeviceService","adminState":"unlocked","operatingState":"disabled1","name":"ds_test1","description":"manage home variable speed motors","lastConnected":0,"lastReported":0,"labels":["hvac","variable speed motor"],"origin":1471806386919}


In the request body above there are two issues. First, the operatingState value is invalid. Second, the Addressable is missing. If you execute the above locally, you'll receive a 400 (Bad Request) with the error message "invalid OperatingState 'disabled1'".

If you fix the OperatingState value, then you'll still receive a 400 with a new error message "Addressable ID and Name are both blank". This is coming from the validator inside of the Addressable model within core-contracts. I noticed that we'll have the opportunity to slim our controller logic by moving the server-side validation we do today into the models. We'll also have consistent state validation (example: validation today is different depending on if you're adding a new Device Service versus updating an existing one).


The order of operations with regard to unmarshaling and validation looks like this

  • DeviceService
    • UnmarshalJSON
      • OperatingState (if provided in the request)
        • UnmarshalJSON
        • validate()
      • AdminState (if provided in the request)
        • UnmarshalJSON
        • validate()
    • validate()
      • OperatingState
        • validate()
      • AdminState
        • validate()
Herein is the only thing that gives me minor heartburn about the solution. The validate() method of both OperatingState and AdminState are called as part of their respective UnmarshalJSON implementations. It doesn't have to be but I thought it made sense to do so since it allows us to fail faster if we detect a problem.

Assuming those pass, then DeviceService calls its own validate() method which takes care of validating all child types. This is necessary in case a child type's key was omitted in the JSON. This means validation for any of the child types can occur twice -- a cheap operation to be sure, but redundant at times.

As an aside, you will see changes in the feature branches related to the elimination of models.Service from core-contracts. Nothing else was composed from this model other than the DeviceService and I felt that eliminating it would simplify the solution.

Comments are welcome. If this looks like a viable approach to folks, then there might be some bandwidth soon to undertake adding UnmarshalJSON to the models that do not have them, and implementing the necessary validate() handlers.


Trevor Conn
Technical Staff Engineer
Dell Technologies | IoT DellTech
Trevor_Conn@...
Round Rock, TX  USA

Re: JSON Marshal vs Decode

Goodell, Leonard
 

That method looks to be working off raw JSON where the method I suggest is on the object, not the JSON.

 

But this isn’t needed if you can get the “remove the omitEmpty tag” working. BTW, I looked at the structs and didn’t see any omitEmpty tag in use. Where is that specified?

 

Thanks!

   Lenny

 

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

 

Well, we kind of have a method like that already.

 

https://github.com/edgexfoundry/go-mod-core-contracts/blob/4425df9b51ed055c673fdafb1a97590b6cd0f324/models/operatingstate.go#L53

 

Again, the problem is that everywhere operating state is used, we have to add this check.

 

Trevor

 

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

 

[EXTERNAL EMAIL]

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

 

Re: JSON Marshal vs Decode

Trevor.Conn@...
 

Well, we kind of have a method like that already.

 

https://github.com/edgexfoundry/go-mod-core-contracts/blob/4425df9b51ed055c673fdafb1a97590b6cd0f324/models/operatingstate.go#L53

 

Again, the problem is that everywhere operating state is used, we have to add this check.

 

Trevor

 

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

 

[EXTERNAL EMAIL]

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

 

Re: 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

 

Re: JSON Marshal vs Decode

Trevor.Conn@...
 

I think my hunch on #1 is correct. If I alter the request JSON to include an empty OperatingState, I can step into the unmarshalling of the type now and receive the error I’m looking for.

{"_class":"org.edgexfoundry.domain.meta.ProvisionWatcher","name":"lonworks watcher test2","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"}, "operatingState":""}

 

So that’s good --- It means we would probably need to remove the omitEmpty tag from any members like this.

 

Trevor

 

From: Conn, Trevor
Sent: Friday, March 8, 2019 10:40 AM
To: Goodell, Leonard; 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

 

Re: JSON Marshal vs Decode

Trevor.Conn@...
 

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

 

Re: JSON Marshal vs Decode

Goodell, Leonard
 

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

 

Re: JSON Marshal vs Decode

Trevor.Conn@...
 

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

 

Re: JSON Marshal vs Decode

Trevor.Conn@...
 

Hi Ian – See below in blue

 

Trevor

 

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? 

 

We covered this pretty extensively in the call. If you’ve watched that already and still have a question, we should talk 1x1.

 

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.

 

Please listen to the call. I did this, actually, because of some feedback you provided some time ago on one of the SMA PRs. I would like to find a better solution.

 

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.

 

I will take a look

 

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

 

Re: JSON Marshal vs Decode

Ian Johnson
 

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

 

Re: JSON Marshal vs Decode

Goodell, Leonard <leonard.goodell@...>
 

Hi Trevor,

  Not seeing your changes. Did you push them to your branches. i.e. “This branch is even with edgexfoundry:master.” 😉

 

Thanks!

   Lenny

 

From: EdgeX-TSC-Core@... <EdgeX-TSC-Core@...> On Behalf Of Trevor.Conn@...
Sent: Thursday, March 07, 2019 11:43 AM
To: edgex-tsc-core@...; EdgeX-GoLang@...
Subject: [Edgex-tsc-core] JSON Marshal vs Decode

 

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

 

Re: JSON Marshal vs Decode

Trevor.Conn@...
 

Whoops – It’s always good to commit one’s changes, not simply add them. They’re there now. Thanks for the catch, Lenny.

 

Trevor

 

From: Goodell, Leonard <leonard.goodell@...>
Sent: Thursday, March 7, 2019 2:01 PM
To: Conn, Trevor; edgex-tsc-core@...; EdgeX-GoLang@...
Subject: RE: JSON Marshal vs Decode

 

[EXTERNAL EMAIL]

Hi Trevor,

  Not seeing your changes. Did you push them to your branches. i.e. “This branch is even with edgexfoundry:master.” 😉

 

Thanks!

   Lenny

 

From: EdgeX-TSC-Core@... <EdgeX-TSC-Core@...> On Behalf Of Trevor.Conn@...
Sent: Thursday, March 07, 2019 11:43 AM
To: edgex-tsc-core@...; EdgeX-GoLang@...
Subject: [Edgex-tsc-core] JSON Marshal vs Decode

 

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

 

JSON Marshal vs Decode

Trevor.Conn@...
 

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

 

Re: [Edgex-tsc-devops] Preliminary Doc on Moving to Go Modules

Trevor.Conn@...
 

Hi all -- I'm going to piggy back on the thread below and loop in the Go email distro for more discussion about modules.

I've attached a V2 of the previous document that was sent. It includes two more bullet points w/r/t DevOps considerations, the final one being reference to a larger section about Go modules and how their adoption may affect the structure of our code repos as well as how we manage tags.


As I said below, I'm offering this proposal in the spirit of discussion. It represents the state of my thinking at the moment. There are some questions that I'd like to get feedback on from both the DevOps team and our Go experts. In addition to any feedback here, I imagine we'll be talking about this in the Working Group calls this week.


Trevor Conn
Senior Principal Software Engineer
Dell Technologies | IoT DellTech
Trevor_Conn@...
Round Rock, TX  USA


From: EdgeX-TSC-DevOps@... <EdgeX-TSC-DevOps@...> on behalf of Gregg, James R <james.r.gregg@...>
Sent: Monday, January 7, 2019 12:21 PM
To: Conn, Trevor; edgex-tsc-devops@...
Subject: Re: [Edgex-tsc-devops] Preliminary Doc on Moving to Go Modules
 

[EXTERNAL EMAIL]

Thanks Trevor.

I’ll add this to the agenda for this week.

 

~James R.Gregg

 

From: EdgeX-TSC-DevOps@... [mailto:EdgeX-TSC-DevOps@...] On Behalf Of Trevor.Conn@...
Sent: Monday, January 7, 2019 11:19 AM
To: edgex-tsc-devops@...
Subject: [Edgex-tsc-devops] Preliminary Doc on Moving to Go Modules

 

Hi all -- I've attached a document that contains everything I can think of w/r/t accommodating Go Modules in our CI/CD pipeline. Moving to modules for dependency mgmt is a goal for the Edinburgh deliverable.

 

The intent is not to consider the attached doc as a final draft or plan. I'd like to use it as a basis for discussion during this week's upcoming DevOps call. From this we can work toward a plan so that we can have this done well in advance of mid-April.

 

Thanks.

 

Trevor Conn
Senior Principal Software Engineer
Dell Technologies | IoT DellTech
Trevor_Conn@...
Round Rock, TX  USA

Anyone Using VSCode?

Trevor.Conn@...
 

Dell Customer Communication

Is there anyone out there using VSCode as their primary IDE for work on EdgeX-Go that would corroborate the suggestion in this issue?

 

https://github.com/edgexfoundry/edgex-go/issues/981

 

Trevor Conn

Senior Principal Software Engineer

Dell Technologies | IoT DellTech

Trevor.Conn@...

Round Rock, TX USA

 

FYI New GlobalSign Mongo Driver

Trevor.Conn@...
 

Dell - Internal Use - Confidential

Hi all – This is to let you know that we now have a new Mongo driver in the master branch of EdgeX thanks to Lenny Goodell @ Intel.

 

PR #915 was just merged

Information about the new driver can be found here

 

Members of our Go developer community please note that the next time you update/rebase your feature branches from master, you will need to delete (or rename) your glide.lock file and execute a “make prepare” to pull the new driver into your vendor directory. From there you can do a “make build” as normal.

 

Trevor Conn

Senior Principal Software Engineer

Dell Technologies | IoT DellTech

Trevor.Conn@...

Round Rock, TX USA

 

Re: Building containers behind a http proxy

David Urbina
 

I do have the HTTP_PROXY exported in my terminal session.

Digging a little more on the error, I can see that "docker build" does get the proxy from nthe terminal but it do not pass it to any the internal commands in the Dockerfile.* scripts. For example, "RUN apk" fails because it does not get the proxy.

Docker documentation mentions that the "--build-arg" must be used to pass the proxy to the container been built. But this certainly requires to modify the EdgeX's Makefile.

Is there a better way? or should It open an issue?

Thanks.

Re: Building containers behind a http proxy

Trevor.Conn@...
 

Dell Customer Communication

Have you tried exporting those vars in the terminal session itself and then running “make docker”? That should cause anything in the terminal session that initiates an http connection to use the specified proxy settings and you won’t have to edit the file. That said, if the proxy blocks a website that glide tries to pull a dependency from, you won’t have a way around that without talking to your network admin.

 

Trevor Conn

Senior Principal Software Engineer

Dell Technologies | IoT DellTech

Trevor.Conn@...

Round Rock, TX USA

 

 

From: EdgeX-GoLang@... [mailto:EdgeX-GoLang@...] On Behalf Of edgex@...
Sent: Tuesday, November 27, 2018 4:59 PM
To: EdgeX-GoLang@...
Subject: [Edgex-golang] Building containers behind a http proxy

 

[EXTERNAL EMAIL]

Hello! I have been trying to build the containers using "make docker" but given the fact that I am in a Intranet behind a proxy it simply fails. The only solution I could find was to modify the Makefile by adding "--build-arg HTTP_PROXY=$(HTTP_PROXY)" to every "docker build". Is there a better way to do this?

Thanks.

Building containers behind a http proxy

David Urbina
 

Hello! I have been trying to build the containers using "make docker" but given the fact that I am in a Intranet behind a proxy it simply fails. The only solution I could find was to modify the Makefile by adding "--build-arg HTTP_PROXY=$(HTTP_PROXY)" to every "docker build". Is there a better way to do this?

Thanks.