Skip to content

[BUG FIX] JSON Serialize Decimal #13050

Closed
dgomes wants to merge 1 commit intohome-assistant:devfrom
dgomes:sql_sensor_data_types
Closed

[BUG FIX] JSON Serialize Decimal #13050
dgomes wants to merge 1 commit intohome-assistant:devfrom
dgomes:sql_sensor_data_types

Conversation

@dgomes
Copy link
Copy Markdown
Contributor

@dgomes dgomes commented Mar 10, 2018

Description:

Related issue (if applicable): fixes #13021

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

@dgomes dgomes requested a review from a team as a code owner March 10, 2018 12:07
@homeassistant homeassistant added core small-pr PRs with less than 30 lines. cla-signed labels Mar 10, 2018
@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 10, 2018

I don't think the solution is to add it to our JSON encoder. Instead, SQL sensor should not store decimals in the state machine to begin with. It's not up to us to decide that all decimal values can be represented as a float.

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 10, 2018

I tried to repro it with SQLite but it never returns the decimal type. Must be a MySQL thing or something.

@balloob balloob mentioned this pull request Mar 10, 2018
3 tasks
@dgomes
Copy link
Copy Markdown
Contributor Author

dgomes commented Mar 10, 2018

It was the update from 0.64 to 0.65 that triggered this bug. (I can reproduce the error)

Decimal is not sqlalchemy exclusive, other libs in the future can trigger the same issue, will we replicate #13059 throughout the code in the future ?

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 10, 2018

We will. If the precision is important, the decimal should be converted to a string (like with money!). It's not up to us to decide that precision is not important and convert it to a float.

@dgomes
Copy link
Copy Markdown
Contributor Author

dgomes commented Mar 10, 2018

I understand the view to ring-fence the issue to the source.

But the issue at hand is related to JSON serialisation, not with internal issues (might be forgetting something here)

A decimal in the state machine should be no different then of a set (which is still considered)

A 2nd order sensor that could use the precision of decimal will be stripped of the possibility.

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 10, 2018

All data in the state machine should be JSON serializable. (we also test this)

The driver behind this is that we want to be able to have a multi-Home Assistant setup and thus need to be able to share states via JSON without having to worry about all the different types that can be int the state machine

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 10, 2018

And so the issue here is not JSON serialization, it's that the SQL sensor violated this unwritten rule.

@dgomes
Copy link
Copy Markdown
Contributor Author

dgomes commented Mar 10, 2018

But this PR actually enables the serialization of Decimal...

Libraries such as Simplejson actually promote Decimal instead of float.

http://simplejson.readthedocs.io/en/latest/index.html?highlight=Decimal

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 10, 2018

The problem is that by enabling serialization of Decimal, you should also always deserialize to Decimal or else you still lose precision. Also, sometimes you don't care about precision (or it was never there) and you don't want people consuming your API to have to convert it to a float themselves.

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 10, 2018

I've merged the other PR since it's the right solution.

@balloob balloob closed this Mar 10, 2018
@dgomes dgomes deleted the sql_sensor_data_types branch March 18, 2018 19:46
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed core small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SQL Sensor adds non JSON serializable types to the state machine

3 participants