AMWA-TV / is-05

AMWA IS-05 NMOS Device Connection Management Specification (Stable)
https://specs.amwa.tv/is-05
Apache License 2.0
20 stars 15 forks source link

Use PATCH with application/merge-patch+json #103

Closed s13n closed 4 years ago

s13n commented 4 years ago

Wouldn't it be more in the spirit of PATCH as described in RFC 5789 to use a specific patch document type such as application/merge-patch+json (RFC 7396) instead of application/json?

Backwards compatibility could be achieved by allowing the old type as a deprecated alternative.

garethsb commented 4 years ago

Theoretically, yes, but the definition of the 'merge' operation defined in IS-05 for PATCH requests on /staged is not the same as RFC 7396, or RFC 6902, so there isn't an existing defined IANA media type that is appropriate.

s13n commented 4 years ago

Is there a good reason for this semantic difference?

garethsb commented 4 years ago

The main one is that JSON Merge Patch cannot be used with JSON document formats that use null as several of the NMOS APIs do.

s13n commented 4 years ago

Well, the obvious comment would be to design the API so that it can use JSON Merge Patch and doesn't require custom patch semantics. An alternative would be to use RFC 6902, but I don't particularly like that, as it is more complicated.

But I guess my underlying problem is that I haven't yet grasped the reasoning behind the API well enough. I'm somewhat struggling with the separation between staging and active, which means that parts of the data structure change location behind the scenes on activation. I wonder what the point of this was. It isn't idempotent, for starters. Perhaps it was done to "protect" activated streams from modification, but that could have been done in a simpler way, I believe.

garethsb commented 4 years ago

Well, the obvious comment[...]

The API must be able to indicate all the transport parameters that are supported by a Receiver, and some of those parameters need to be able to indicate a null value.

I'd have preferred to see RFC 7396 used in the spec, but it's not quite appropriate. 🤷‍♂️

I'm somewhat struggling with the separation between staging and active, [...]

The /active endpoint must reflect the current active configuration of the underlying interface. If that is changed by other means (e.g. non-NMOS interface), the values on this endpoint should reflect that.

The /staged endpoint allows an NMOS Client to prepare the parameters and schedule when these staged parameters should be transitioned to active.

s13n commented 4 years ago

some of those parameters need to be able to indicate a null value

Yes, I appreciate that, but what I meant was that the design of data structures is a part of the API design, so it should be possible to avoid using a null value in a way that prevents RFC 7396.

But admittedly I am arguing post factum.

Regarding active vs. staged:

I was assuming that whether a stream is active or not could be reflected by a boolean parameter. Setting it to true would activate the stream. Setting it to false would do the reverse. Whether a stream is active or staged is part of its state in my opinion.

Having "activeness" of a stream represented in the current way seems to me to expose aspects of what you call "the underlying interface". And I was wondering if there's any benefit in that. Maybe the aim was to make it impossible to activate the same stream twice, since the first activation would have removed it from /staged, leading to an error when the second attempt is made. In my preferred scheme, setting the activation parameter to true when it already is true is a noop, and doesn't cause an error, which reflects the spirit of idempotence.

garethsb commented 4 years ago

I think you've missed that a Sender or Receiver can have both a current active state and a (staged) scheduled change to the active state.

s13n commented 4 years ago

Doesn't that apply to all other parameters that make up the state? IOW you have what may be called a "scheduled patch" to the (potentially entire) state of a stream. With this I mean a patch that you can send now, and it gets enacted at a scheduled point in time.

garethsb commented 4 years ago

I think the answer to that is yes, and that's then the state that is maintained at /staged, whereas /active is the currently active state.

s13n commented 4 years ago

Does that mean that every stream that appears in the /active hierarchy has to be present in the /staged hierarchy, too? Even when there are no changes scheduled?

garethsb commented 4 years ago

Absolutely, in response to a GET. (Hence the point about nulls.)

(If by 'stream' you meant JSON attribute? Every Sender/Receiver always has both /staged and /active endpoints.)

s13n commented 4 years ago

OK, I was indeed mistaken about that. I assumed that activation meant to effectively move the corresponding data structure from staged to active. I wasn't aware that it remains accessible under staged.

Thanks for clarifying this!

I think that clears it up for me, and I'm closing the issue.