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
73.34k stars 30.64k forks source link

Scripts defaults provided by fields breaks existing scripts #129545

Closed Petro31 closed 1 day ago

Petro31 commented 4 days ago

The problem

This PR https://github.com/home-assistant/core/pull/128622 is causing validators to populate default values into any script that executes.

Historically, these were just for the UI. People who make these scripts (or blueprints) have needed to provide those default values (or account for them being gone) in templates throughout the script. The linked PR breaks all of that functionality. Most of my scripts were broken by this PR.

To clarify, this paradigm has now been broken:

my_script:
  fields:
    image:
      description: Local image.
      example: "local\\images\\home.jpg"
      default:
      selector:
        text:
          type: url
  variables:
    external_url: !secret external_url
    url: >
      {% if image is defined %}
        {{ external_url }}/{{ image.replace('\\', '/') }}
      {% endif %}

This means all optional inputs that users have created in the past no longer work and every one of them needs to be updated. In this example, some_variable will now be None, as well as my_field1 when before some_variable would be filled with a default value.

Even a basic example now no longer works

my_script:
  fields:
    my_field1:
      selector:
        text:
      default: 
  variables:
    some_variable: "{{ my_field1 | default('X') }}`

some_variable will now be populated with null or None instead of X because the default function considers none a valid value. You specifically have to add default('X', False) to treat None as a bad value.

If you erroroneously used the keyword None in yaml, it will come through as the string "None". And many people likely make this mistake because templates resolve to None. Yes, it is unrelated and incorrect syntax, however it's common.

What version of Home Assistant Core has the issue?

2024.11.0b0

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

No response

Link to integration documentation on our website

No response

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

Petro31 commented 3 days ago

My suggestion is to add a grace period with warnings in the logs.

We add a temporary key named validate and it's default is set to false for existing scripts. However, the code will still validate scripts inputs and provide a warning for when things aren't meeting the generated validation. Add a hard line in the sand for when the validation: false will be removed and things will be always validated.

I recommend a grace period of 1 year.

Also, we could default newly created scripts via the UI to validate: true.

Petro31 commented 3 days ago

Here's a script that sends a notification with a time stamp and creates a persistent notification with it. This recently broke because image: randomly gained a default of the string None.

notify:
  alias: Notify
  fields:
    format: 
      description: The timestamp format for persistent notifications & notifications.  Annoucements do not have a timestamp.
      example: "%-I:%M:%S %p"
      default: "%-I:%M:%S %p"
      selector:
        text:
          type: text
    persistent_notification:
      description: On/Off for Persistent Notifications
      default: on
      example: on
      required: false
      selector:
        boolean:
    title:
      description: Title of the persistent notification.
      example: "Title"
      default: ""
      selector:
        text:
          type: text
    message:
      description: Message to be sent/annouced.
      example: "Hello"
      required: true
      selector:
        text:
          type: text
    image:
      description: Local image.
      example: "local\\images\\home.jpg"
      default: None
      selector:
        text:
          type: url
    notify: &notify_homeowners
      description: Devices to notify.
      example: notify.mobile_app_petro
      selector:
        select:
          options:
          - label: Petro
            value: notify.mobile_app_petro
          - label: Lambo
            value: notify.mobile_app_lindsays_iphone
          - label: Home Owners
            value: notify.homeowners
  variables:
    timestamp: >
      {% set t = now() %}
      {% if format is defined %}
        {{ None if format is none else t.strftime(format) }}
      {% else %}
        {{ t.strftime("%-I:%M:%S %p") }}
      {% endif %}
    persistent_notification: >
      {{ persistent_notification | default(true) }}
    out_notify: "{{ notify | default('notify.homeowners') }}"
    external_url: !secret external_url
    url: >
      {% if image | default %}
        {{ external_url }}/{{ image.replace('\\', '/') }}
      {% endif %}
    persistent_notification_data: >
      {% set ret = namespace(data=[]) %}
      {% if title | default %}
        {% set ret.data = ret.data +[('title', title)] %}
      {% endif %}
      {% if message | default %}
        {% set ret.data = ret.data +[('message', '[{}] {}'.format(timestamp, message) if timestamp else message)] %}
      {% endif %}
      {{ dict.from_keys(ret.data) or {'message': 'No Message Provided'} }}
    notification_data: >
      {% if image | default %}
        {{ dict(data={'attachment': {'url': url}}, **persistent_notification_data)}}
      {% else %}
        {{ persistent_notification_data }}
      {% endif %}

  sequence:
  - choose:
    - conditions: "{{ persistent_notification }}"
      sequence:
      - service: persistent_notification.create
        data: "{{ persistent_notification_data }}"
  - choose:
    - conditions: "{{ out_notify.startswith('notify.') }}"
      sequence:
      - service: "{{ out_notify }}"
        data: "{{ notification_data }}"

Here's an example of a script that most likely didn't need the required: true field. Mistake on my part, however it flooded my logs with errors.

harmony_activity_on:
  alias: Turn On Activity
  mode: restart
  description: >
    Turns on any harmony activity and optionally any specified devices.
  fields:
    activity:
      description: (Required) The harmony activity name.
      example: Xbox One
      required: true
      selector:
        select:
          options:
          - Chromecast
          - Playstation
          - Roku
          - Switch
          - TV
          - Xbox One
    <<: &remote_field
      remote:
        description: (Optional) The remote for the harmony activity
        example: remote.living_room
        required: true
        selector:
          entity:
            domain: binary_sensor
            device_class: door
    <<: &echo_field
      echo_through_reciever:
        description: (Optional) Switch that turns on/off bluetooth adapter for echo
        example: switch.echo_through_receiver
        selector:
          entity:
            domain: switch
  variables:
    harmony: "{{ remote | default('remote.living_room') }}"
    echo: "{{ echo_through_reciever | default('switch.echo_through_receiver') }}"
  sequence:
  - service: remote.turn_on
    target:
      entity_id: "{{ harmony }}"
    data:
      activity: "{{ activity }}"
  - wait_template: "{{ is_state_attr(harmony, 'current_activity', activity) }}"
  - service: switch.turn_off
    target:
      entity_id: "{{ echo }}"

Many more examples here: https://github.com/Petro31/home-assistant-config/tree/master/scripts

A lot of it is caused by the misuse of the required tag. I treated it as something that is required for the script to function and I built safety nets to ensure it the script would work even if it wasn't provided. I'm sure many others have done that as well.

Here's some other quick examples:

endless field -> https://github.com/Petro31/home-assistant-config/blob/master/scripts/lzw45.yaml

prefix -> https://github.com/Petro31/home-assistant-config/blob/master/scripts/laundry.yaml

I'm sure there's other examples, and I haven't even looked at any community script blueprints which use the | default or is defined paradigm.

Regardless if this change goes through, there needs to be a grace period. Because users will not realize things are broken. I only noticed when things went wrong and the stack traces in the logs were 3 exceptions deep.