-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Update IntervalAction to use the common Address #536
Conversation
Since the common Address created, we can add it to the IntervalAction model and DTO. Close edgexfoundry#535 Signed-off-by: weichou <[email protected]>
Id string `json:"id,omitempty" validate:"omitempty,uuid"` | ||
Name string `json:"name" validate:"edgex-dto-none-empty-string,edgex-dto-rfc3986-unreserved-chars"` | ||
IntervalName string `json:"intervalName" validate:"edgex-dto-none-empty-string,edgex-dto-rfc3986-unreserved-chars"` | ||
Address Address `json:"address" validate:"required"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the dive
validation attribute to cause Address to be validated so don't have to manually call validate on Address??
Like we have done here for Readings in and Event:
https://github.com/edgexfoundry/go-mod-core-contracts/blob/master/v2/dtos/event.go#L30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'dive' only works for slice and map, see https://pkg.go.dev/github.com/go-playground/validator/v10#hdr-Dive
In this case, the request DTO looks like:
{
"requestId": "e6e8a2f4-eb14-4649-9e2b-175247911369",
"apiVersion":"v2",
"action": {
"apiVersion":"v2",
"name": "query",
"intervalName": "afternoon",
"address":{
"type": "REST",
"host": "123.123.0.1",
"port": 123,
"httpMethod": "GET"
}
}
}
Actually, I can success to verify the Address without call validation on the Address. But I need to verify the REST or MQTT protocol properties.
Currently, I didn't find any suitable tag for our use case so I wrote the Validate function, see https://github.com/edgexfoundry/go-mod-core-contracts/blob/master/v2/dtos/address.go#L25.
If we don't want to call validate on Address, we can
- Migrate the v1 validator to verify the deeper struct https://github.com/edgexfoundry/go-mod-core-contracts/blob/master/models/validator.go
- Or write a custom validation tag at https://github.com/edgexfoundry/go-mod-core-contracts/blob/master/v2/validator.go
Close edgexfoundry#535 Signed-off-by: weichou <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Close #535
PR Checklist
Please check if your PR fulfills the following requirements:
If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/go-mod-core-contracts/blob/master/.github/Contributing.md.
What is the current behavior?
Issue Number: #535
What is the new behavior?
Since the common Address created, we can add it to the IntervalAction model and DTO.
Does this PR introduce a breaking change?
New Imports
Specific Instructions
Are there any specific instructions or things that should be known prior to reviewing?
Other information