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

Receiver cannot easily validate Sender "event_type" #61

Open
garethsb opened this issue Jul 15, 2019 · 12 comments
Open

Receiver cannot easily validate Sender "event_type" #61

garethsb opened this issue Jul 15, 2019 · 12 comments

Comments

@garethsb
Copy link
Contributor

Since "event_type" is only available via the Sender's IS-07 Events API /type endpoint or a request for the IS-04 Source/Flow, or once the first event message is received, a Receiver cannot easily reject a Sender of the wrong event type during a Connection API request to PATCH /staged.

The current design therefore puts the onus firmly on the controller/client to ensure it does not connect incompatible senders and receivers. I think using the "transport_file" has been discussed but rejected.

This issue is just added to confirm my understanding is correct, and note this area for possible enhancement in the future.

@garethsb
Copy link
Contributor Author

Moreover, although the number event type has a "unit", this is not carried in the event state. Does this mean that the Receiver MUST make an HTTP GET request to the Sender?

A unit may also be indicated in the measurement event type like number/temperature/C that is carried in the event state, but that doesn't seem to be required either. Should it be?

@mjeras
Copy link
Contributor

mjeras commented Jul 18, 2019

Your understanding is correct, at least for the time being (i.e. v1.0). Sending the Type JSON in the IS-05 parameters was discussed, but that would be a major change and it should really go into v1.1, probably best dealt with together with the entire complex object type definition.

A unit may also be indicated in the measurement event type like number/temperature/C that is carried in the event state, but that doesn't seem to be required either. Should it be?

I am not sure if you really meant "event state" or maybe "event_type"? If it was about event type, that is the recommendation, not mandatory. As far as temperature goes, it would be straight forward, but I am not certain there would be other examples (measurement types) where this might not be appropriate.

@garethsb
Copy link
Contributor Author

I am not sure if you really meant "event state" or maybe "event_type"?

I meant the "event_type" is required in the event message and /state endpoint, as well as being required in the /type endpoint. Are the "event_type" in each place required to be identical? (I assume so.)

I think I was confused partly by https://github.com/AMWA-TV/nmos-event-tally/blob/v1.0.x/examples/eventsapi-type-number-measurement-get-200.json which says it is an example of a measurement but has "event_type": "number" rather than "number/temperature/C".

I think we should fix that, and add a corresponding eventsapi-state-number-measurement-get-200.json to examples?

@garethsb
Copy link
Contributor Author

Your understanding is correct, at least for the time being (i.e. v1.0). Sending the Type JSON in the IS-05 parameters was discussed, but that would be a major change and it should really go into v1.1, probably best dealt with together with the entire complex object type definition.

OK, makes sense. This part is somewhat related to AMWA-TV/is-05#96.

@mjeras
Copy link
Contributor

mjeras commented Jul 18, 2019

That type in the type definition JSON should state which JSON (JS) type is the underlying type. So this should basically say: Take the "number" type in JSON, apply the range -20 to 100 wit 0.1 step and treat it as degrees Celsius. I would therefore say the example is correct. (the "event_type" in the various resources is a single sting which should be comparable using wildcards and carry the essential type information, while the type definition is the entire JSON file where individual type properties are broken down and presented in more detail, therefore the units of measure are separated from the base type)

@garethsb
Copy link
Contributor Author

OK... The /type endpoint does not contain the measurement name anywhere. So actually a Receiver has to instead look at the Sender /state endpoint or examine the first event (state) message to confirm that it is connected to an appropriate Sender.

@mjeras
Copy link
Contributor

mjeras commented Jul 18, 2019

I meant the "event_type" is required in the event message and /state endpoint, as well as being required in the /type endpoint. Are the "event_type" in each place required to be identical? (I assume so.)

Just to give you a direct answer, the first two should be identical, but not the third one. Please note that the first two are called "event_type" and the third is just "type"

@garethsb
Copy link
Contributor Author

Please note that the first two are called "event_type" and the third is just "type"

Yes, sorry for my confusion, the /type "type" is clearly documented as the base type.

@garethsb
Copy link
Contributor Author

garethsb commented Jul 18, 2019

If not obvious, my concern is that a Receiver of "number/length/m" is connected to a Sender of "number/length/feet" by a poorly written Client, and as a result our rocket doesn't reach the moon.

Or a Receiver of "number/apples/kg" is connected to a Sender of "number/oranges/kg"...

@mjeras
Copy link
Contributor

mjeras commented Jul 18, 2019

OK... The /type endpoint does not contain the measurement name anywhere. So actually a Receiver has to instead look at the Sender /state endpoint or examine the first event (state) message to confirm that it is connected to an appropriate Sender.

The intent when preparing the "event_type" spec was for the control system to use the values on the sender and receive sides to filter out incompatible receivers before they are connected. Now, what should happen if an invalid connection is made? The receiver can pull the type from the sender using the Events API url, but will also get the current state immediately after connecting. He could therefore drop the connection (and maybe park itself?) either if/when it pulls the type or after the first state is received. He could do the same at a later stage if an invalid message should arrive (although, in theory, that shouldn't happen if the sender is designed properly). What remains unclear though, is what would the expected behaviour of the control system be - should it try reconnecting? Should we pass the type info in IS-05 in the future, the receiver would be in a better position to just refuse the IS-05 PATCH command, although I am not sure that IS-05 would allow that at the moment (400 would be the closest one to that, although it might not match the case from the RAML: "...when the request did not meet the schema and constraints.")

As far as the "measurement" part of the "event_type" is concerned, that one is missing in the type definition, but can be inferred from the unit of measure. There could be some ambiguity, although the unit itself should uniquely define the measure type. (I could see someone having "number/width/m" and "number/height/m", but I don't think that would be valid and should be "number/distance/m". The "width" vs. "height" issue should be better dealt with in the role part of grouping (BCP-002-01)). Maybe something to be added in v1.1 for clarity

@garethsb
Copy link
Contributor Author

Our understanding of the issues is very similar.

Now, what should happen if an invalid connection is made? The receiver can pull the type from the sender using the Events API url,

Agreed. This may be a QoI issue however, since it requires a round-trip to the Sender, and PATCH /staged on IS-05 is expected to respond quickly (apart from "mode": "activate_immediate").

but will also get the current state immediately after connecting. He could therefore drop the connection

Agreed.

(and maybe park itself?) either if/when it pulls the type or after the first state is received. He could do the same at a later stage if an invalid message should arrive (although, in theory, that shouldn't happen if the sender is designed properly).

This point, and how to handle ephemeral error conditions, like broken connections, is the purpose of the IS-05 documentation issue, AMWA-TV/is-05#96. There are two points:

(i) whether the Receiver should automatically try re-making the connection after some kinds of error (current understanding: yes, but how often/for how long is QoI issue)

(ii) whether the Receiver should ever indicate some kinds of error by marking itself inactive in IS-05 /active endpoint and IS-04 Receiver "subscription" (current understanding: no)

What remains unclear though, is what would the expected behaviour of the control system be - should it try reconnecting?

First it needs a notification of the error condition, and (based on current understanding), there can be none via IS-04/IS-05.

Should we pass the type info in IS-05 in the future, the receiver would be in a better position to just refuse the IS-05 PATCH command,

Agreed.

although I am not sure that IS-05 would allow that at the moment (400 would be the closest one to that, although it might not match the case from the RAML: "...when the request did not meet the schema and constraints.")

500 is closest: returned when "there is some failure caused by activation which was not caused by a violation of schema or constraints." (https://github.com/AMWA-TV/nmos-device-connection-management/blob/v1.1-dev/APIs/ConnectionAPI.raml#L270-L273)

@garethsb
Copy link
Contributor Author

(ii) whether the Receiver should ever indicate some kinds of error by marking itself inactive in IS-05 /active endpoint and IS-04 Receiver "subscription" (current understanding: no)

Updated proposal that allows connection-oriented Senders/Receivers to mark themselves inactive in the case of unrecoverable errors is now here - AMWA-TV/is-05#97. Thoughts on that would be appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants