Re: [Edgex-tsc-core] Core Contract Model Validators


On 4/4/19 3:39 PM, Malini Bhandaru wrote:

Thank you Trevor for the links! Shall check them out.

Thanks for the reply Malini!


Am I correct in assuming that the validator will be invoked at the recipient endpoint of a rest call, and only then?

That's what we were discussing during our call today. The original problem was that a received JSON model was missing a required field. Trevor's branches both make the call to Validate in the UnmarshallJSON methods of our models. So when any of our services handle incoming REST calls with JSON payloads, the service calls json.Unmarshall, which in turn causes the UnmarshallJSON method of the top-level model, and all of it's children to be called recursively. The problem is that since Validate is called by each model's UnmarshallJSON method, you can end up with Validate being called more than once for some of the embedded models. My suggestion to call validate *after* the json.Unmarshall call was an alternative to adding a "valid" flag to each model in order to reduce the duplicate calls.

All that said, there's nothing that would prevent our services (or unit tests) from using Validate() in other areas of the code...

Then we could as part of our microservices have a base class that does validation as a first step, kind of like form input validation at the server side.

Unfortunately Go doesn't provide traditional OO inheritance, meaning there's no concept of base classes. Your analogy is correct though, with this addition, our services would validate REST input (CBOR or JSON instead of HTML forms) before doing anything with the resulting models.

Reflection used recursively does makes perfect sense.


There was a comment during today’s core WG call about annotations and min required fields.

One thing that this does not address is “context” based required fields. For example a person may need to share only name and phone in “contact” but

Needs to share name, phone and address for a parcel delivery type flow.

Actually this proposal allows each model to decide what is required in order for the model to be considered valid. The original bug was that a provisionwatcher was received and it lacked an "OperatingState". With this approach, the OperatingState Validate() method would return an error as since one wasn't received in the JSON, the OperatingState isn't set to one of the two allowed values ("Enabled" or "Disabled").

What do people think about using gRPC for inter microservice communication? Not for Edinburg but going forward. gRPC has schemas, efficient.

We had a long discussion about the merits of gRPC during our original design discussions about binary data support. In the end, we decided to go with a new serialization format (CBOR), but keep REST as the default mechanism for cross-service communications.

Note - JSON also supports schemas, and we discussed the merits of using a schema validation vs. an Go interface-based approach, and it was felt the latter was a better solution.


But Trevor may point out that schemas also constrain that we provide more as opposed to just a name.


Was driving, so I was unsuccessful in unmuting, but want to chime in on log levels. I do think the default should not be trace.





From: <EdgeX-GoLang@...> on behalf of "espy via Lists.Edgexfoundry.Org" <>
Reply-To: "espy@..." <espy@...>
Date: Thursday, April 4, 2019 at 12:13 PM
To: "Conn, Trevor (EMC)" <Trevor_Conn@...>, "EdgeX-GoLang@..." <EdgeX-GoLang@...>, "edgex-tsc-core@..." <edgex-tsc-core@...>
Cc: "EdgeX-GoLang@..." <EdgeX-GoLang@...>
Subject: Re: [Edgex-golang] [Edgex-tsc-core] Core Contract Model Validators


On 4/4/19 9:49 AM, Tony Espy wrote:

On 4/2/19 6:38 PM, Trevor.Conn@... wrote:

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:


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.

I did a bit of research on this and there's been at least one formal golang proposal to enhance the JSON annotation language to cover missing fields:

That said, although the proposal was shot down, the use of a Validator interface was discussed as an alternative.

I'm +1 on the overall approach (including use of reflection).


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.

I think always validating when unmarshalling an external request makes sense, and one could argue that validating if/when we construct a new instance of a model makes sense too. I'm not sure I follow the exact flow where we'd validate twice?

An alternate/simpler approach to that solves the duplicate validation calls problem is just to make validation a responsibility of the original caller (ie. the function/method that makes the first json.Unmarshall()) instead of always calling Validate() at the end of a model's UnmarshallJSON().


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.

Seems reasonable



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





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.


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
Round Rock, TX  USA

Join to automatically receive all group messages.