devicehive / devicehive-java-server

DeviceHive Java Server
http://www.devicehive.com
Apache License 2.0
369 stars 139 forks source link

Notifications are not coming from devices of user created after notification subscription #101

Closed demon-xxi closed 9 years ago

demon-xxi commented 9 years ago

Logging the bug as reported by @SorJEF

Steps to reproduce:

  1. Subscribe to all notifications for all devices via websocket
  2. Create a new user
  3. Register a new device for the newly created user
  4. Send notification for the newly created device

Actual Result: Notification is not being sent to the subscription

Expected Result: Notification is being sent to the subscription

Note: The expected behavior is happening if steps 1 and 2 are swapped so that user is created before subscription.

demon-xxi commented 9 years ago

We need to check is this is really expected behavior. Registering to all devices is not always a good scenario. If any system needs ALL notifications then it may be worth creating kafka worker to read everything.

sburr commented 9 years ago

It is true that registering for all devices is not always a good idea. But it is certainly sometimes a good idea. Implementation under the covers with a special-case kafka worker could be useful.

sorjef commented 9 years ago

An update on that:

We thought that it depends on steps related to the new user provisioning, but we verified today that it is not related to the user provisioning flow; notifications for new devices for existing user are not seen either.

zubrabubra commented 9 years ago

Defect is reproducible one have to specify empty array for names field when submitting subscribe request, while omitting names field follows expected behavior - notifications are received for all devices even created after subscription request. Though the case of empty array needs to be discussed, and should be propagated to all possible corner cases within DH.

sorjef commented 9 years ago

@zubrabubra, just a small correction so there is no confusion: this is not the problem with the names field for the subscription request, but with the deviceGuid field for the subscription request.

TLDR:

Let's at least propagate an ability to subscribe to all devices even which do not exist yet no matter whether deviceGuid is set to some value, is null or is omitted.

Overview

Let me reiterate the current situation with the deviceGuid field in the subscription request:

  1. When set to null or undefined - subscribe to all devices even that are not registered yet
  2. When set to [] (empty array) - subscribe to all devices except the devices which are not registered yet
  3. When set to ['some-value', ...] - subscribe to specified devices except the devices which are not registered yet

Subscription to all devices

I think that in any case we need to make this behaviour clear and understandable and, therefore, I would appeal to consistency. There are two approaches in bringing that into the consistent state:

The feature when notifications are received for all devices even created after the subscription request is mandatory. There are different use cases when you need to configure something on new device creation, so you subscribe to $device-add notification. So if we remove this feature it will be impossible to perform some actions on new device creation and the existence of the $device-addnotification will make no sense.

Conclusion: Let's propagate user can subscribe to devices even which are not registered yet for all cases.

What to do with an empty array

Another separate question is what to do when deviceGuid is an empty array. There are some comments on that in the issue #56 related to names field for subscription request. I would personally handle deviceGuid set to empty array the same way as null or undefined just because when an error is returned in this case, practically it is more inconvenient than useful.

demon-xxi commented 9 years ago

@SorJEF, I am ok with doing subscribe to all devices' events when deviceGuids is unset or set as null, basically meaning there is no filter on that field. On the other hand I am strictly against doing number 2 with [] array. That is too tricky business logic and not obvious at all. I would like to avoid that kind of logic in the dh code. Especially as it is currently achievable by getting list of available devices and then doing option 3 by passing them all to subscription. That is more transparent and predictable way.

And empty array should return 400 error as request to subscribe to no devices is useless and so incorrect.

zubrabubra commented 9 years ago

Closing defect, as passing null in guid field works as expected. Lets move discussion on empty array to separate thread.