Fix downgrade support #343
Conversation
…alue or set to predefined default value for the required property
|
Since this is a significant change to code which is used by both Node and Registry, I recommend unit tests with very good coverage are added. |
| } | ||
| })") | ||
| }, | ||
| { make_schema_id(is04_versions::v1_1, nmos::types::sender, U("tags")), make_schema(R"({ |
There was a problem hiding this comment.
I can't see any difference between v1.0 and v1.1 schemas for tags?
There was a problem hiding this comment.
sender's tags v1.0 is optional and v1.1 is not.
| "description": "URN identifying the type of service", | ||
| "format": "uri" | ||
| }, | ||
| "authorization": { |
There was a problem hiding this comment.
Is it necessary to remove authorization when downgrading before v1.3? The v1.2 schema doesn't consider it invalid as far as I know. Same applies to many other cases.
There was a problem hiding this comment.
Only apply the JSON validation on the device's 'controls' parameter when downgrading to v1.1 or v1.2. If JSON validation fails, the 'controlswill be set to an empty array. btw if that ever happens, our idea of theis_permitted_downgrade` is no longer corrected. I am just extending the original downgrade resources_versions, rather than adding sub-parameters.
| { U("href") }, | ||
| { U("hostname") }, | ||
| { U("caps") }, | ||
| { U("services"), value::array() } |
There was a problem hiding this comment.
This doesn't seem like right strategy to me. I don't think we should remove authorization when considering v1.3 node resource and exposing it via Node API v1.0 or registering via Registration API v1.0. If someone has good reason that we should do that, I don't think we should ever remove all services and replace with empty array... 😕
There was a problem hiding this comment.
as long as the JSON validation is successful, the authorization parameter will not be removed.
| } | ||
|
|
||
| // list of property validators to identify the associated property has been changed since the previous version | ||
| static const web::json::experimental::json_validator& property_changed_validator() |
There was a problem hiding this comment.
Minor nit: property_changed_validator sounds like something that is used to perform validation every time a property is changed (e.g. as a result of nmos::modify_resource). Not sure what is the best name, though!
There was a problem hiding this comment.
maybe just named it as property_validator, using the word changed because I wanted to specify that the validator is only used for those properties which have schema changed between versions.
| return U("/downgrade") + make_api_version(version) + U("-") + type.name + U("-") + property; | ||
| } | ||
|
|
||
| // property schemas are extracted from the resource schemas |
There was a problem hiding this comment.
Since all of these property schemas are already included in the full schemas embedded in nmos-cpp, could this code be refactored to use a json value path to extract the property value schema from those? Not sure...
There was a problem hiding this comment.
If we agree with this PR, this can be followed up.
Yes totally agree |
Fix IS-04 (v1.3) downgrade resource to register to v1.2 Registry.
There are some parameters which have been extended in nmos-parameter-registers for the latest nmos version.
The problem was identified when a downgraded v1.3 video flow with
transfer_characteristicset toUNSPECIFIEDfailed to register to the v1.2 Registry. It is because v1.2 video flow schema used in the Registry does not allowtransfer_characteristicto beUNSPECIFIED. This PR sets the default values for the downgraded resources when an invalid value is used or removes it if the parameter is optional.See https://specs.amwa.tv/nmos-parameter-registers/branches/main/flow-attributes/#transfer-characteristic