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

Breaking change: send sensor list attributes as list to server #2478

Merged

Conversation

jpelgrom
Copy link
Member

@jpelgrom jpelgrom commented Apr 26, 2022

Summary

When the app collects sensor attributes, it is stored in the app's database as a string and converted back to it's original type when sent to the server, if possible. This PR updates the "if possible" part to also include lists. This will allow users to use array features in HA templating for these values.

Because this changes the format for attribute values, this is a breaking change. Based on a quick search, this will have changes for the following sensors:

  • Bluetooth Connection
  • Last Notification
  • Last Removed Notification
  • Active Notification Count

I've tested this with core 2022.4.7. Fixes #2474.

Screenshots

n/a

Link to pull request in Documentation repository

n/a

Any other notes

 - Send a sensor attribute that is managed by the app as a list, to the server as a list as well instead of a stringified version of a list.
 - To prevent issues where we can't distinguish the list items separator from a value that includes the separator, don't try to convert a value to a list if that is the case but instead use string
@dshokouhi
Copy link
Member

  • Last Notification (technically, depends on extras but I haven't seen any lists in there)
  • Last Removed Notification (technically, depends on extras but I haven't seen any lists in there)

Not related to the changes here but on this subject we do have this long standing bug: #1206

I don't believe there are any other places other than what you have listed where this occurs

Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the debug APK and it checks out fine to me.

 - Instead of only falling back when a list includes a String with the separator, just use toString() to also support other objects which might include ", " when stringified, such as nested lists
@jpelgrom
Copy link
Member Author

Not related to the changes here but on this subject we do have this long standing bug: #1206

I've looked at the issue and believe that I've fixed it, see pull request #2482. Now the notification sensors will almost certainly get lists in their attributes 🙂. It's been tested in combination with this PR and I've made a small change to prevent problems when merged.

@jpelgrom
Copy link
Member Author

jpelgrom commented Apr 27, 2022

A concern raised here: the fallback to a string in case there is a ", " in the stringified list values may produce inconsistent attribute value types.

I'm not sure what the best solution is here;

  • drop this PR for consistency
  • continue with this PR and ask users to check the format themselves every time
  • perhaps store data in the database as JSON/custom format which allows escaping values (so the ", " won't be a problem) but will increase complexity because we need to do more conversions

Edit: did a quick test using jacksonObjectMapper().writeValueAsString(it.value) instead of it.value.toString() if it is a list (and also readValue when reading from DB to send), and it seems to work rather well. I'll need to do some additional testing later when I have time, but I think this will work in all scenarios and removes the need to fallback to strings.

 - To remove any confusion when sending individual list items, store data as JSON to escape characters if necessary
 - Map Any data in a list that is not any of the specific types to string, for consistency with the same types when not in a list and to prevent JSON serialization issues
@jpelgrom
Copy link
Member Author

Updated to store lists as JSON in the database, which escapes any problematic characters, so now users can be sure that lists are always sent as lists. List values will always be booleans, ints, longs, floats or, if not one of the previous types, strings using toString(), for consistency with values outside lists.

I also looked into storing all other types as JSON, which might be interesting when #2482 is merged because that code will allow (nested) key/value maps as attributes. Unfortunately Jackson's serialization of some classes throws exceptions (for example, Bitmap) and I don't see a simple fix for that, so it is out of scope for this PR.

@jpelgrom jpelgrom requested a review from dshokouhi April 28, 2022 18:50
@JBassett JBassett merged commit b418491 into home-assistant:master Apr 30, 2022
@jpelgrom jpelgrom deleted the sensor-attributes-list-type-2474 branch April 30, 2022 11:48
@Mincka
Copy link
Contributor

Mincka commented May 9, 2022

Can someone confirm this one works as expected for bluetooth connection or other sensors?
I get comma separated strings for bluetooth sensor or last_notification.

I just checked in beta-2300-f33743dc published last week and now android.textLines is a string. It's not an array converted as a string, it just comma separated elements. I was expecting that this PR would convert the array do a list but it looks like that's not the case.

android.appInfo: ApplicationInfo{8fc5718 com.whatsapp}
android.infoText: null
android.largeIcon: null
android.people.list: android.app.Person@739669d8
android.progress: 0
android.progressIndeterminate: false
android.progressMax: 0
android.reduced.images: true
android.remoteInputHistory: null
android.showChronometer: false
android.showWhen: true
android.subText: null
android.summaryText: ‎7 nouveaux messages
android.template: android.app.Notification$InboxStyle
android.text: ‎7 nouveaux messages
android.textLines: Un, Deux, Trois, Quatre, Cinq, 📷 Photo, 🥰
android.title: Morgane Bardet
androidx.core.app.extra.COMPAT_TEMPLATE: androidx.core.app.NotificationCompat$InboxStyle
is_clearable: true
is_ongoing: false
package: com.whatsapp
post_time: 1652110784918
icon: mdi:bell-ring
friendly_name: Téléphone Julien Last Notification
state_class: measurement
connected_not_paired_devices: 5A:33:CC:BC:FD:73
connected_paired_devices: 5A:33:9C:BB:FD:73, 98:28:DD:E8:01:4F
paired_devices: 70:CE:BB:DA:2C:0D, 00:80:AA:F9:94:4F, 88:5A:CC:70:2B:1D
unit_of_measurement: connection(s)
icon: mdi:bluetooth
friendly_name: Téléphone Julien Bluetooth Connection

Maybe we need to reopen #2474

@jpelgrom
Copy link
Member Author

jpelgrom commented May 9, 2022

I can confirm it is working correctly for me, tested just now with the Bluetooth Connection sensor on a build based on beta-2310. If they were still strings, I would expect to see [] around the values like you can see in the app.

Did you copy these values from the 'more info' panel or frontend dev tools states table? I think the frontend will only show lists as lists if you open the 'Set State' panel in the dev tools, otherwise it shows lists as a string with comma separated items.

@Mincka
Copy link
Contributor

Mincka commented May 9, 2022

Oh, indeed that's what I did. I can confirm it shows as a list using 'Set State'. Sorry about that and thanks again! I am going to have some fun with the for each loop now. 😛

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

Successfully merging this pull request may close these issues.

Bluetooth sensor not showing correct lists
5 participants