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

handle points with "" and "0" key #580

Closed
cbrake opened this issue Jul 26, 2023 · 9 comments
Closed

handle points with "" and "0" key #580

cbrake opened this issue Jul 26, 2023 · 9 comments
Labels
enhancement improvements on existing functionality

Comments

@cbrake
Copy link
Member

cbrake commented Jul 26, 2023

We are running into problems (ex #579) where we might have points with key set to "0" and a rule runs with key set to "". The point scheme is nice in that you can easily upgrade from scaler (key = "") to array values (key = ""). However, what do you do if initially the key is set to "" and you want to convert to a scaler? Or if one bit of code writes the key as "" and another "0".

@cbrake cbrake added the enhancement improvements on existing functionality label Jul 26, 2023
@cbrake
Copy link
Member Author

cbrake commented Jul 26, 2023

There are several ways we can handle this:

  1. on queries or updates, assume key "0" equals ""
  2. consider key="" is invalid and set it to "0" instead

2 seems simpler long term. It is a constraint, but once that constraint is met, then all the other code gets simpler. Otherwise, every client will have to deal with the "0" == "" issue.

@cbrake
Copy link
Member Author

cbrake commented Jul 26, 2023

This commits implements this idea:

d896145

tests pass and this fixes #579

@bminer FYI

@cbrake
Copy link
Member Author

cbrake commented Jul 26, 2023

documentation: 7cd32c8

@bminer
Copy link
Contributor

bminer commented Jul 27, 2023

I'm not sure that I understand the reasoning behind this change. When Key is "", this should be interpreted to mean we have a scalar value; whereas, when Key is set to 0 or any positive integer, we have an array element. Clients should really anticipate if a node type should be an array or should be a scalar in advance, right?

@cbrake
Copy link
Member Author

cbrake commented Jul 27, 2023

Good question -- initially I thought that clients could do this. However, the issue came up when multiple clients interact -- in this case the rule client with shelly client (#579).

The issue that precipitated this change was Shelly device support -- we can have relay devices that have one switch, two, or four. To keep the code the same in the shelly io client, I always assume an array that is initialized to 1, 2, or 4 elements, depending on the device. This drives the UI as well, as it displays as many switches as elements in the array:

image

In the case of a light bulb and some relays, there is only one switch.

So when a user writes a rule to control a light bulb, the natural inclination is to leave key blank, as there is only one light bulb. Then two points get populated for the same light -- one with key == "" and key == "0", which essentially mean the same thing. Since key = "" and key = "0" mean the same thing, then it may make sense to eliminate the possibility of having both points in the store. We can say that users should know this, but people will make mistakes, and then when you get duplicate points in the store, how do we handle that?

Another case is when we start with a scalar but want to migrate to an array. For instance, we may initially only support one email address for a user, but in the future support multiple. If we always initialize key to "0", this transition is easy. If not, then we have to do a data migration.

I started down the path to making the store point merge code handle this case, but then this rippled into multiple areas of the code (UI, querying, etc). After going down this path for a bit, it seemed easier to add a constraint on the data than to handle this special case everywhere.

The downside to always initializing key is if we ever need to differentiate between a scaler point and an array. Can you think of any scenarios where this might be needed?

@bminer
Copy link
Contributor

bminer commented Jul 27, 2023

Not really, to be honest. After hearing your full explanation, I think this makes a lot of sense. Basically there are no scalar point values. You either have an array or an object, and an array is allowed to contain a single element.

@cbrake
Copy link
Member Author

cbrake commented Jul 27, 2023

Yes. That is the conclusion I came to after using it in real-life applications. Appreciate the review/feedback as it seems a little clunky on the surface to have the key="0" constraint on scalar points, but I think this is a big-picture optimization, not a micro-optimization.

@bminer
Copy link
Contributor

bminer commented Aug 22, 2023

Can we close this?

@cbrake
Copy link
Member Author

cbrake commented Aug 22, 2023

yes, closing ...

@cbrake cbrake closed this as completed Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improvements on existing functionality
Projects
Development

No branches or pull requests

2 participants