Skip to content

Cast attribute values to string before publishing to MQTT#9872

Merged
balloob merged 3 commits into
home-assistant:devfrom
tinloaf:fix_statestream
Oct 27, 2017
Merged

Cast attribute values to string before publishing to MQTT#9872
balloob merged 3 commits into
home-assistant:devfrom
tinloaf:fix_statestream

Conversation

@tinloaf
Copy link
Copy Markdown
Contributor

@tinloaf tinloaf commented Oct 14, 2017

Description:

Breaking Change: MQTT Statestream now serializes all data to JSON before publishing. This means that string attributes and values will be quoted from now on (e.g.: '"on"' instead of 'on'). You can still read these strings without the quotes by using 'value_json' instead of 'value' where applicable (e.g., templates). This causes automatic JSON deserialization. Other simple types are not affected.

This fixes errors when an entity has an attribute that is not "a string, bytearray, int, float or None" and mqtt_statestream is used. As of now, the attribute is just handed over to paho, and paho can only send the aforementioned types. This patch fixes the issue by just casting everything to string before handing it over to paho.

There are a number of components / entities which have "other" attributes, e.g. light that have an RGB attribute which is a list.

Related issue (if applicable): fixes #9815

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

val_str = str(val)
hass.components.mqtt.async_publish(mybase + key,
val, 1, True)
val_str, 1, True)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not using str(val) directly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 14, 2017

Instead of casting everything to string, let's try to figure out which attribute types are not supported and convert those. It seems like it's only boolean?

@tinloaf
Copy link
Copy Markdown
Contributor Author

tinloaf commented Oct 14, 2017

@balloob No, it's also lists of things, like (see my initial PR comment) the RGB-tuple for lights, or the mode-list of climate devices. That's only the types that I found by going over my own config, I have no clue how many more different types are being used as attributes.

I also think that a generic solution, i.e., "handling every type", is the right thing to do here: Otherwise, if someone writes a new component / platform and uses a type in the attributes that we did not cover, suddenly code in mqtt_statestream breaks even though that code wasn't changed. Also, tests will never catch this: If you write a new component / platform, you probably don't test how your new code behaves in combination with mqtt_statestream.

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 14, 2017

In that case we shouldn't just cast it to a string but run it through json.dumps with the JSONEncoder from homeassistant.remote. All things in state attributes are JSON serializable by that encoder and that way the receiver will know how to process it.

@tinloaf
Copy link
Copy Markdown
Contributor Author

tinloaf commented Oct 14, 2017

@balloob It's using the JSON serializer now. However, this changes the behavior slightly: the serializer always quotes strings, i.e. the payload for a string attribute will now be "the_value" instead of the_value as before. I'm not sure if that is a problem. Since the component hasn't been around for long and not too many people are using it yet, I'm not sure if that should be considered a "breaking change"?

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 15, 2017

Oh, that's indeed a nasty side effect :|

@tinloaf
Copy link
Copy Markdown
Contributor Author

tinloaf commented Oct 20, 2017

@balloob As we've agreed having everything go through json.dumps() / json.loads(), I think this is ready for merging now?

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Ok to merge when you add a paragraph to your PR that we can list in the breaking change section and also create a PR for the docs.

@tinloaf
Copy link
Copy Markdown
Contributor Author

tinloaf commented Oct 26, 2017

I added a section describing the breaking change in my original PR comment, and a doc PR here: home-assistant/home-assistant.io#3783

Edit: The Travis build for the doc PR fails, however in some file that I never touched. I guess there's something broken there?

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 27, 2017

Thanks!

@balloob balloob merged commit 248d974 into home-assistant:dev Oct 27, 2017
@balloob balloob mentioned this pull request Nov 2, 2017
@tinloaf tinloaf mentioned this pull request Dec 28, 2017
2 tasks
@tinloaf tinloaf mentioned this pull request Jan 13, 2018
4 tasks
@home-assistant home-assistant locked and limited conversation to collaborators Mar 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MQTT Eventstream: Lots of errors b/c of weird types in attributes?

5 participants