TheThingsArchive / ttn

The Things Network Stack V2
https://www.thethingsnetwork.org
MIT License
462 stars 276 forks source link

Payload Functions: Formalise ability to drop messages in Encoder #248

Closed FokkeZB closed 8 years ago

FokkeZB commented 8 years ago

Unlike uplink, we don't have a separate validator function for downlink to drop invalid messages.

Simply by trying out I found that only if I return an empty array ([]) the message will indeed be dropped.

I'd suggest we:

  1. Expand the behaviour to drop on any falsy return value (e.g. null and false)
  2. Formalise and document this, with false as most verbose example.

I've created https://github.com/TheThingsIndustries/dashboard/issues/77 for 2)

htdvisser commented 8 years ago

Or maybe just throwing/returning an error? Will work for both uplink and downlink.

FokkeZB commented 8 years ago

@htdvisser then there wouldn't be a way anymore to tell the difference between some kind of runtime error (pass the wrong stuff to some JS API e.g.) and the decision to drop the message would there?

I also think we should keep the required knowledge on JS minimal.

htdvisser commented 8 years ago

The behaviour would be the same for both cases: stop processing.

The problem with a falsey value is that 0 is a falsey value, while I might want to switch the LED on my Arduino off.

FokkeZB commented 8 years ago

But when I'm testing the payload function I want to know if it was dropped or an error occurred.

The encoder needs to return an array to be valid so if you want to send 0 you need to return [0] which is not falsy

johanstokking commented 8 years ago

The data comes from the application domain. Let's assume it's correctness. I don't really see a use case when an application wants to publish a message and that the network decides it's invalid based on business logic that is not present in the application.

The reason why there is a validator for uplink is that you might want to filter out measurement outliers from further processing. You can do this in the application as well, in which case you don't use the validator.

I'd rather drop the validator from uplink, than introduce a validator for downlink.

avbentem commented 8 years ago

I'd rather drop the validator from uplink

For uplinks one can authorize others to access the MQTT data. So, I think it's nice to have a generic validator that works for all who are authorized to use the MQTT data.

For downlinks, I assume one can NOT authorize others to send downlinks? If true, then please just discard or delete the rest of this very comment.

However, if one can authorize others for downlinks†:

The data comes from the application domain. Let's assume it's correctness.

If coming from a (trusted) third party, then I guess "assume its correctness" might be false. So then a documented mechanism to drop downlinks will indeed be nice.

†) I hope not, as that could even yield multiple downlinks for one uplink... Chaos.

johanstokking commented 8 years ago

For the record, the reason there is an optional payload function for conversion and validation, separate for decode is:

The reason why there is no converter and validator for downlink is because I want us to support zero calculations and checks on the node to save power. By using payload functions, we allow developers to do this in one place, before making the data available to one or more integrations. It's optional and doesn't apply to applications.

FokkeZB commented 8 years ago

If we don't formalise, then shouldn't we error if the encoder doesn't return what we want it to return?

johanstokking commented 8 years ago

Yes, like we should if payload function errors. I'll open a new issue for this.