Skip to content
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

IS-04 Sender "manifest_href" for IS-07 Senders #38

Closed
garethsb opened this issue May 8, 2019 · 15 comments · Fixed by #45
Closed

IS-04 Sender "manifest_href" for IS-07 Senders #38

garethsb opened this issue May 8, 2019 · 15 comments · Fixed by #45
Labels
dependency issue This issue depends on an issue in an 'upstream' specification has PR Pull requests covering this entire issue have been opened

Comments

@garethsb
Copy link
Contributor

garethsb commented May 8, 2019

Related to #36. Like #37, possibly this is a question for IS-04.

The example in Core models - IS-04 highlights - Senders shows "manifest_href": "". If JSON schema validation is applied to this example (using IS-04 schemas/sender.json), it should fail validation due to the empty string not being a valid uri.

@mjeras
Copy link
Contributor

mjeras commented May 8, 2019

If I am right in saying this field is used only for old IS-04 connection management, this might be discussed together with the deprecation of that feature. Unlike transportfile in #36, the name of this field could allow us to publish maybe the type definition/schema here, it probably wouldn't be a good idea, as it could get confused with the old use of this field.
As far as the schema goes, you are right, this should be addressed, probably best in IS-04 as it is extended to allow for those new transports. At the moment I don't have a better idea than to put an empty string or null or not put the field at all, all of which violate the schema. (In case of schema changes it would be best to omit it, although I am not sure if there is an easy way of doing it just for non rt/dash transports - it would probably require "subclassing" the sender schema and making it quite complicated and potentially humanly unreadable)

@garethsb
Copy link
Contributor Author

garethsb commented May 8, 2019

I think I agree, better not to repurpose this.

One precedent we have is the issue of "flow_id" in the sender schema (see AMWA-TV/is-04#67). In IS-04 v1.1 this started to allow null as well as string. This presents a theoretical problem when browsing such senders through the Query API v1.0, because in v1.0, null isn't permitted, so the v1.0 rendition of the resource isn't valid according to the schema.

In our case, we really want a similar relaxation, to make "manifest_href" optional, or allow null like "flow_id" above, or allow empty string as in the current examples in IS-07. This could be done in IS-04 v1.3. It would result in the same issue with the Query API as described above, but in practice, maybe that is OK.

@garethsb
Copy link
Contributor Author

garethsb commented May 8, 2019

Actually, I had forgotten that @andrewbonney has already added some good documentation to IS-04 v1.3-dev on this topic, since the problem of JSON values from v1.x that don't conform to earlier versions now includes "transport" for example. It seems to me that adding "manifest_href" to this list (by adding null as a valid value) doesn't make things any worse! See AMWA-TV/is-04#83.

@andrewbonney
Copy link

I hadn't clocked that an empty string would evaluate as a schema error. That's unfortunate as whilst 'null' is certainly a better value for 'nothing', it does create a replica of the 'flow_id' issue we've previously had which Gareth mentioned.

Whilst adding support for 'null' doesn't break with existing precedents, the 'flow_id' case was an accident and a similar fix here has the potential to break older clients. I'd suggest this may be a little different to the case of introducing new transports, as a well-written client may be checking the transport type already, where it may not expect a value to change type.

This does all add further weight to a feeling I've had that we ought to be removing the automatic Query API key stripping functionality and depending upon the query downgrade mechanism for multi-version support instead, but that's very much an IS-04 discussion point.

@garethsb
Copy link
Contributor Author

While it would be nice to solve the problem of whether Event & Tally senders automatically stripped in Query API v1.0, v1.1, v1.2 were valid according to the respective schemas, the fundamental thing is we need them to be valid according to the IS-04 v1.3 schemas.

Either we need a placeholder URL (like browsers' "about:blank"), or we need to modify the IS-04 schema for "manifest_href":

  • explicitly allow empty string in addition to URI format
  • make not required so it can be omitted
  • allow null
  • change from "format": "uri" to "format": "uri-reference" in v1.3, which would allow relative URLs and thus can be empty... on the other hand, a relative URL isn't what we want here, a missing or null transport file is

Since none of these would validate according to v1.2 or earlier schemas, I'd go with whichever of the above is best for v1.3, and I think that's probably null.

This does all add further weight to a feeling I've had that we ought to be removing the automatic Query API key stripping functionality and depending upon the query downgrade mechanism for multi-version support instead, but that's very much an IS-04 discussion point.

So v1.3 resources would never appear in a response to a v1.2 API?

@andrewbonney
Copy link

Since none of these would validate according to v1.2 or earlier schemas, I'd go with whichever of the above is best for v1.3, and I think that's probably null.

I agree that the 'best' choice for v1.3 is to allow 'null'.

So v1.3 resources would never appear in a response to a v1.2 API?

Yes, that's the thinking. You could as a result only cause mixed version responses by using /v1.3/resources/?query.downgrade=v1.2 etc, ensuring that only clients which are explicitly written to support multiple versions are exposed to it.

@mjeras
Copy link
Contributor

mjeras commented May 14, 2019

I am not even trying to add IS-07 resources if the registry isn't v1.3, so I would personally support changing the schema to allow nulls. I would also agree that IS-07 resources shouldn't be downgraded as critical information would be lost - or better said, there is no way to adjust the transports to match v1.2 schemas.

@garethsb
Copy link
Contributor Author

So v1.3 resources would never appear in a response to a v1.2 API?

Yes, that's the thinking. You could as a result only cause mixed version responses by using /v1.3/resources/?query.downgrade=v1.2 etc, ensuring that only clients which are explicitly written to support multiple versions are exposed to it.

That makes sense but loses us some of the flexibility discussed in IS-04 Upgrade Path - Performing Upgrades.

I would also agree that IS-07 resources shouldn't be downgraded as critical information would be lost - or better said, there is no way to adjust the transports to match v1.2 schemas.

That may be true for IS-07 resources, but it's not true for IS-04 v1.3 resources in general.

Good idea to take this into an IS-04 issue, and focus on the IS-04 v1.3 vs. IS-07 issue here?

@andrewbonney
Copy link

Yes, I'd suggest moving it to an IS-04 issue.

@mjeras
Copy link
Contributor

mjeras commented May 14, 2019

That may be true for IS-07 resources, but it's not true for IS-04 v1.3 resources in general.

Exactly, that's why I was referring to them as IS-07 resources rather than v1.3 resources. I would add a firm statement that those could only appear in v1.3+ responses

Yes, I'd suggest moving it to an IS-04 issue.

Yes, it would be great if an IS-04 issue could be opened and this one closed

@garethsb
Copy link
Contributor Author

garethsb commented May 14, 2019

So... an IS-04 PR for v1.3-dev sender.json to allow null (and probably some changes in the docs as a result), and a PR for this repo to change IS-07 examples from empty string to null, and an issue in IS-04 regarding automatic stripping... (All now done...)

@garethsb
Copy link
Contributor Author

I would add a firm statement that those could only appear in v1.3+ responses

I really don't think that logic should have to exist in a Registry. So unless/until query stripping is completely removed from IS-04, I think we are going to see stripped/downgraded resources in earlier version responses, as the IS-04 documentation makes clear.

@mjeras
Copy link
Contributor

mjeras commented May 14, 2019

But how would you strip/downgrade e.g. the example sender from the IS-07 docs?:
{ "tags": {}, "interface_bindings": [ "eth0" ], "subscription": { "receiver_id": null, "active": false }, "description": "Virtual Button 1", "transport": "urn:x-nmos:transport:mqtt", "label": "Virtual Button 1", "id": "68f519a3-5523-4b2c-b72d-ec23cc80207d", "device_id": "58f6b536-ca4c-43fd-880a-9df2501fc125", "flow_id": "0554b43a-ea7c-418d-a39e-1205ee281af3", "manifest_href": "", "version": "1529676926:000000000" }
The problem is "transport": "urn:x-nmos:transport:mqtt" - You wouldn't be able to remove this as that would break the old schema and the allowed formats there (basically RTP and DASH) are very inappropriate. I guess we would have to come up with a URN that doesn't start with "urn:x-nmos:" which would be allowed, but I am not sure that would be nice either...

@garethsb
Copy link
Contributor Author

That's specifically covered by Version Translations and Requirements for Query API clients which says:

"Implementers should be aware that when operating a client against a Query API version lower than the maximum supported version by that Query API instance, they may be exposed to resource data beyond the scope of that API version's associated schemas. Whilst the Version Translation mechanism will remove access to invalid additional keys, new values for existing keys are not excluded by this mechanism."

@mjeras
Copy link
Contributor

mjeras commented May 14, 2019

OK, thanks for the clarification

@garethsb garethsb added has PR Pull requests covering this entire issue have been opened dependency issue This issue depends on an issue in an 'upstream' specification labels May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency issue This issue depends on an issue in an 'upstream' specification has PR Pull requests covering this entire issue have been opened
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants