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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support XML conversion for RESTful sensors #31809

Merged

Conversation

@bdraco
Copy link
Contributor

bdraco commented Feb 13, 2020

Breaking change

Proposed change

Many devices continue to use XML for RESTful
APIs. Interfacing with these APIs requires custom
integrations or command line fork()/exec() overhead
which many of these devices can work with as if
they were JSON using xmltojson via this spec:
https://www.xml.com/pub/a/2006/05/31/converting-between-xml-and-json.html

This change implements converting XML output to
JSON via xmltojson so it can work with the existing
rest sensor component. As the attributes that
usually need to be scraped are deeper in the document
support for passing in a template to find the
JSON attributes that have been added. JSON APIs that
do not have their attributes at the top level
can also benefit from this change.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml
sensor:
  - platform: rest
    resource: http://192.168.1.20/status.xml
    authentication: basic
    username: username
    password: password
    json_attributes:
      - "htstatus"
      - "poolsp"
      - "spasp"
      - "pooltemp"
      - "spatemp"
      - "airtemp"
    json_attributes_path: "$.response.temp"
    value_template: '{{ value_json.response.temp.poolsp }}'
  - platform: rest
    resource: http://192.168.1.5/status.xml
    json_attributes:
      - "led0"
      - "led1"
      - "user0"
      - "temp0"
      - "btn0"
    json_attributes_path: "$.response"
    value_template: "OK"

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum
Many devices continue to use XML for RESTful
APIs.  Interfacing with these APIs requires custom
integrations or command line fork()/exec() overhead
which many of these devices can work with as if
they were JSON using xmltojson via this spec:
https://www.xml.com/pub/a/2006/05/31/converting-between-xml-and-json.html

This change implements converting XML output to
JSON via xmltojson so it can work with the existing
rest sensor component.  As the attributes that
usually need to be scraped are deeper in the document
support for passing in a template to find the
JSON attributes that have been added.  JSON APIs that
do not have their attributes at the top level
can also benefit from this change.
@project-bot project-bot bot added this to Needs review in Dev Feb 13, 2020
@bdraco bdraco requested a review from balloob Feb 13, 2020
@codecov

This comment was marked as off-topic.

Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #31809 into dev will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #31809      +/-   ##
==========================================
- Coverage   94.68%   94.66%   -0.03%     
==========================================
  Files         763      763              
  Lines       55128    55193      +65     
==========================================
+ Hits        52197    52246      +49     
- Misses       2931     2947      +16
Impacted Files Coverage 螖
homeassistant/components/rest/sensor.py 99.28% <100%> (+0.1%) 猬嗭笍
...meassistant/components/homematicip_cloud/switch.py 93.4% <0%> (-6.6%) 猬囷笍
...eassistant/components/homematicip_cloud/climate.py 98.29% <0%> (-1.71%) 猬囷笍
...omponents/homematicip_cloud/alarm_control_panel.py 98.52% <0%> (-1.48%) 猬囷笍
homeassistant/components/template/cover.py 96.34% <0%> (-1.37%) 猬囷笍
...omeassistant/components/homematicip_cloud/cover.py 97.36% <0%> (-1.32%) 猬囷笍
homeassistant/components/uk_transport/sensor.py 93.47% <0%> (-0.73%) 猬囷笍
homeassistant/components/ps4/media_player.py 89.76% <0%> (-0.16%) 猬囷笍
homeassistant/helpers/config_validation.py 95.92% <0%> (-0.14%) 猬囷笍
homeassistant/components/ipma/config_flow.py 100% <0%> (酶) 猬嗭笍
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 3e23a3a...2c725e5. Read the comment docs.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Feb 14, 2020

We should not have a template generate JSON. So please drop the json_attributes_template.

For the XML support, let's not make it an option and just do it. If the response content type is XML, we parse the XML to JSON.

On a side note: the Rest component should really allow specifying 1 data source and multiple sensors/binary sensors on top of it using the DataUpdateCoordinator. Something like this:

rest:
  - resource: http://example.com/api
    binary_sensors:
      - resource: /is_on  # relative to parent
@bdraco

This comment has been minimized.

Copy link
Contributor Author

bdraco commented Feb 14, 2020

@balloob.
Thanks for the quick comments!

I鈥檓 using json_attributes_template to specify the location in the JSON to scrape attributes since at times they are not at the top level. Can you suggest another method of doing this?

I could switch to jsonpath instead as it鈥檚 a bit more flexible and already being used in other places.

Will do on the auto switch xml. Likely looking for text/xml or .xml as the path in the url instead of having to specify

@bdraco

This comment has been minimized.

Copy link
Contributor Author

bdraco commented Feb 14, 2020

Example configuration.yaml updated for jsonpath. If this is acceptable, I'll write the docs tomorrow

bdraco added a commit to bdraco/home-assistant.io that referenced this pull request Feb 14, 2020
@bdraco bdraco mentioned this pull request Feb 14, 2020
3 of 7 tasks complete
Dev automation moved this from Needs review to Reviewer approved Feb 15, 2020
bdraco added 3 commits Feb 16, 2020
鈥vior with jsonpath"

This reverts commit fe9e179.
@balloob balloob merged commit 096e7cc into home-assistant:dev Feb 16, 2020
10 checks passed
10 checks passed
CI Build #20200216.6 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100.00% of diff hit (target 94.68%)
Details
codecov/project 94.66% (target 90.00%)
Details
Dev automation moved this from Reviewer approved to Done Feb 16, 2020
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Feb 16, 2020

Realized this was a breaking change for people that currently where doing string searches on XML data. Can you update the PR description to summarize the change for the release notes?

@lock lock bot locked and limited conversation to collaborators Feb 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can鈥檛 perform that action at this time.