home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
69.74k stars 28.91k forks source link

RESTful notifications can only return strings #117365

Open bogd opened 1 month ago

bogd commented 1 month ago

The problem

I am unable to use the REST notify integration to send an integer parameter as part of a POST_JSON request.

In my case, I have to set the NTFY priority in the request JSON, and as per the ntfy docs that has to be an integer value. But no matter what I do, the integration sets the value as a string in the JSON.

I am not the only one in this situation - see for example this community discussion.

I have the new templates (legacy_templates is false in my installation), and when testing the template in DevTools the result is a number.

However, looking at the integration code, it appears that this line is forcing parse_result=False. And as far as I can tell, this effectively bypasses type detection and conversion, leaving the result as a string.

Is there a specific reason why that call is made with parse_result=False?

If not, would it be possible to change it to parse_result=True, allowing us to provide integer values as part of the JSON?

Thank you!

What version of Home Assistant Core has the issue?

core-2024.5.3

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

REST

Link to integration documentation on our website

https://www.home-assistant.io/integrations/notify.rest/

Diagnostics information

No response

Example YAML snippet

notify:
- name: ntfy
  platform: rest
  method: POST_JSON
  target_param_name: topic
  message_param_name: message
  data:
    attach: "{{data.attach}}"
    priority: >-
      {% if data.priority == "1" %}
        1
      {% endif %}

Anything in the logs that might be useful for us?

Priority should be returned as an integer, but the resulting JSON from the service call looks like this, with priority a string (which is rejected by the called API):

    "json": {
        "message": "Test",
        "topic": "bogd_general",
        "attach": "<some_url_here>",
        "priority": "1"
    }


### Additional information

_No response_
home-assistant[bot] commented 1 month ago

rest documentation rest source

timr49 commented 1 week ago

Hi @bogd, I did a quick test of your suggested change (parse_result=True) in a custom_components copy of the rest integration. It seemed to work. Configuration file was:

notify:
  - platform: rest
    name: notify_rest_test1
    resource: "https://echo.free.beeceptor.com"
    method: "POST_JSON"
    target_param_name: target
    message_param_name: message
    title_param_name: title
    data:
      priority: "{{ 3 }}"

I added response.text to the debug logging for HTTPStatus.OK and got:

2024-06-23 22:33:54.439 DEBUG (MainThread) [custom_components.rest.notify] Success. Response 200: OK: {
  "method": "POST",
  "protocol": "https",
  "host": "echo.free.beeceptor.com",
  "path": "/",
  "ip": "***.***.***.***:*****",
  "headers": {
    "Host": "echo.free.beeceptor.com",
    "User-Agent": "HomeAssistant/2024.6.4 httpx/0.27.0 Python/3.12",
    "Content-Length": "65",
    "Accept": "*/*",
    "Accept-Encoding": "gzip, deflate, br",
    "Content-Type": "application/json"
  },
  "parsedQueryParams": {},
  "parsedBody": {
    "message": "my message 3",
    "title": "my title 3",
    "priority": 3
  }
}

whereas with the original parse_result=False, the third-last line above was

  "priority": "3"

Of course this is not a complete test of the change but it confirms your idea.

bogd commented 1 week ago

@timr49 - thank you for testing! I would have gladly submitted a PR directly, but I do now know enough about the HAss internals to understand why that parse_result=False was put there in the first place (the fact that it is also there in rest_command made me think there might be some other reason for its presence).

Hopefully it is not mandatory, and we will be able to remove it in a future version - making the rest_* components much more useful :)

timr49 commented 1 week ago

@bogd - you are welcome! It appears that parse_result=False is used when only a string is required (e.g. icon and picture templates) and parse_result=True is used when a (Python) value is required (e.g. availability templates). I assume that the reason for its presence is simply a time-optimisation but, like you, am not sure.

I read somewhere that rest has 100% test coverage so if you were to do the full PR cycle (with a test added to demonstrate this change) and they all passed, that (plus the reviewers) should be enough. (I tried to build a dev environment but my available hardware isn't sufficient for docker and visual studio - I have been testing this and some other things in custom_components.)

timr49 commented 1 day ago

I did a quick test of your suggested change (parse_result=True) in a custom_components copy of the rest integration. It seemed to work.

It appears that my quick testing was indeed, too quick. Further testing shows that all strings that look like numbers are converted to numbers, all strings that look like Booleans are converted to Booleans, etc. For example,

data:
  answer: '{{ "42" }}'

results in the REST request content containing { "answer": 42 } rather than { "answer": "42" }.

The reason is that the template renderer always returns Python object of type str regardless of the Jinja2 data type. When parse_result=True this string is passed to the Python parser (literal_eval), which determines what data type the string contents would be if they were part of a Python program. Hence, whether a template renders the string "42" or the number 42, the result is the string "42" if parse_result=False or the number 42 if parse_result=True.

Other Notification integrations using restful APIs that I have looked at (e.g. pushover) don't have this problem because their API builds the JSON from the arguments passed to it in the form expected by their server.

As for a solution, we could add support for a payload_template in the manner of RESTful Integration, which renders the entire JSON payload, e.g.

data:
  payload_template: >-
    {% set priority = ... %}
    {{ '{  "message":"' ~ message ~ '", "title": "' ~ title ~ '", "priority": ' ~ priority ~ '}' }}

The existing data dict would be available to the renderer as variables to that message, title, etc. are available to it.