fix: process include/exclude devices

[danielbrunt57]

Process include_devices and exclude_devices strings as CSV lists.
Fixes issue #2477

[alandtse]

Something has changed. Those should already be lists. Not strings. The ingestion step should already convert them from strings.

[jleinenbach]

I've noticed that the code uses .splt instead of .split() in both changes:

if include and dev_name not in include.splt(","):

and

if exclude and dev_name in exclude.splt(","):

Just to clarify, is this intentional or could it be a typo, possibly due to a copy/paste error? I'm asking because .splt is not a standard Python method, and I'm wondering if this was meant to be .split().

Thanks!

[jleinenbach]

Something has changed. Those should already be lists. Not strings. The ingestion step should already convert them from strings.

With the assistance of ChatGPT, I believe I've identified the root cause of the issue:

Potential Issue in config_flow.py:

There is a potential problem in the way the CONF_INCLUDE_DEVICES and CONF_EXCLUDE_DEVICES lists are handled within the _save_user_input_to_config method. Specifically, the use of reduce to convert these lists into comma-separated strings can lead to unintended behavior.

Explanation:

• Use of reduce: When the CONF_INCLUDE_DEVICES or CONF_EXCLUDE_DEVICES lists are provided as input, the current code uses reduce to concatenate the list items into a single string, separated by commas:

  if isinstance(user_input[CONF_INCLUDE_DEVICES], list):
      self.config[CONF_INCLUDE_DEVICES] = (
          reduce(lambda x, y: f"{x},{y}", user_input[CONF_INCLUDE_DEVICES])
          if user_input[CONF_INCLUDE_DEVICES]
          else ""
      )

  This transformation turns a list like ["Echo Auto", "Bedroom"] into the string "Echo Auto,Bedroom". While this might seem convenient, it introduces a risk where the string may be incorrectly processed later, particularly in matching logic.

• Implication: Once converted to a string, any subsequent matching logic could mistakenly identify partial matches as valid exclusions or inclusions. For example, if CONF_EXCLUDE_DEVICES contains "Echo Auto", it might be incorrectly treated as "Echo,Auto", leading to unintended exclusions like "Echo".

Recommendation:

Avoid converting lists into comma-separated strings using reduce. Instead, maintain the list structure to ensure that each device name is matched exactly as intended. This approach will prevent issues where partial names are incorrectly excluded or included due to improper string processing.

[chrisvblemos]

There is a typo in split(). But I wouldn't apply the fix here, since in this case you would change the expected type to string instead of a list, which goes against the rest of the code (i.e, the function you changed expects an Optional[list[str]], not a Optional[str]. If you think this is the proper way, I'd change the rest of the code to conform to this change.

Instead, I'd just apply the fix inside save_user_input_to_config() at config_flow.py so that instead of transforming config[CONF...] insto a str, it keeps it as a list.

[alandtse]

So this is probably worth checking as these were the prior assumptions:

1. Strings are inputs from the config entry fields and also from the .storage location. Only code processing the config info needs to treat them as strings and will help convert to Lists.
2. Service calls probably also have to treat inputs as strings but also convert to lists.
3. All strings should have been converted to lists once live in HA. All interaction should be with lists within the component.

If any of these have been broken, then they need to be fixed. This also suggests the proposed fix here is also incorrect since it breaks 3. Let's plan to close this after a few days unless your investigation shows that assumptions are wrong.

[danielbrunt57]

Sorry, I've been busy today with 8 hours of landscaping work to make some $$$, and my body is not used to physical labor any more!

@jleinenbach Yes, it's a stupid typo but not due to copy/paste error, instead a failure to use copy/paste and attempt to type such a small change correctly, and fail!

@alandtse, @chrisvblemos
This is not a new issue from recent code changes that I'm aware of, rather it's a recent change in how I configure AMP that's brought it to light. After dozens and dozens of re & re's and setting areas for my 18 devices, I paused for reflection on why I needed some of them at all in HA and so started using exclude_devices thus uncovering what's likely been this way for I don't know how long.

I read the comments this morning and went back though AMP's version history until I found mention of UI config and that was v2.0.0. I downloaded it and saw this in config_flow.py:

Line 140

    async def async_step_user`

Lines 163-178:

        if isinstance(user_input[CONF_INCLUDE_DEVICES], str):
            self.config[CONF_INCLUDE_DEVICES] = (
                user_input[CONF_INCLUDE_DEVICES].split(",")
                if "CONF_INCLUDE_DEVICES" in user_input
                else []
            )
        else:
            self.config[CONF_INCLUDE_DEVICES] = user_input[CONF_INCLUDE_DEVICES]
        if isinstance(user_input[CONF_EXCLUDE_DEVICES], str):
            self.config[CONF_EXCLUDE_DEVICES] = (
                user_input[CONF_EXCLUDE_DEVICES].split(",")
                if "CONF_EXCLUDE_DEVICES" in user_input
                else []
            )
        else:
            self.config[CONF_EXCLUDE_DEVICES] = user_input[CONF_EXCLUDE_DEVICES]

They used to be stored back then as lists in the .storage location and at some point over the years were changed to strings.

[danielbrunt57]

3. All strings should have been converted to lists once live in HA. All interaction should be with lists within the component.

I'm not seeing that in 4.12.8 or 4.10.3, which is before my Options feat change so I'm sure now I did not break it and think this has been a longstanding unnoticed possible failure point, which my particular setup has just now revealed. Or it may have been encountered before but never reported?

In __init__.py, async def setup_alexa inputs them as strings (since they are saved in .storage as strings):

1266    include = config.get(CONF_INCLUDE_DEVICES)
1267    exclude = config.get(CONF_EXCLUDE_DEVICES)

include & exclude are only used in __init__.py in lines 540 & 553 where I made my change.

So if I am to obey point #3, then it looks like the fix should be applied at lines 1266,1267?
However, if I use exclude = config.get(CONF_EXCLUDE_DEVICES).split(',') then media_player is broken. Why? I don't currently know but with this code change I get no alexa media players. I also get zero devices in AMP, just 73 entities.
Therefore, I either have to fix it:
a) the way I did.
or
b) leave include/exclude as strings, set exclude_list = exclude.splt(',') and change if exclude and dev_name in exclude to if exclude and dev_name in exclude_list (and similarly for include)

Is b) the preferred way to do it?

[alandtse]

Make sure you understand fully how exclude/include work as that confuses people.

That said, make sure that the behavior with empty lists is sane. For include, it used to ignore an empty list. If you've modified the code and it actually tries to process an empty list, then nothing will be included since they're not explicitly allowed. Exclude should not have that issue at all as it's a ban list and an empty list shouldn't ban anything.

[danielbrunt57]

@alandtse Yes, I understand how exclude/include work.
I will test any new code modifications to ensure empty lists remain functional.

@alandtse For include, it used to ignore an empty list. If you've modified the code and it actually tries to process an empty list, then nothing will be included since they're not explicitly allowed. Exclude should not have that issue at all as it's a ban list and an empty list shouldn't ban anything.

Debugging tells me config.get(CONF_INCLUDE_DEVICES) returns a string.
I do not see any code to convert the include and exclude strings to lists which would allow if include and dev_name not in include to correcty build the include_filter[] and exclude_filter[] lists.

@chrisvblemos Instead, I'd just apply the fix inside save_user_input_to_config() at config_flow.py so that instead of transforming config[CONF...] into a str, it keeps it as a list.

I was thinking of that but that would seem to go against Alan's three assumptions.

My original (rejected) fix works and after spending another 5 hours on this, the only other way I see to solve it is:

    _LOGGER.debug("Setting up Alexa devices for %s", hide_email(login_obj.email))
    config = config_entry.data
    email = config.get(CONF_EMAIL)

    include = config[CONF_INCLUDE_DEVICES]
    if include:
        include = include.split(',')
    _LOGGER.debug("include devices: %s", include)

    exclude = config[CONF_EXCLUDE_DEVICES]
    if exclude:
        exclude = exclude.split(',')
    _LOGGER.debug("exclude devices: %s", exclude)

    scan_interval: float = (

[danielbrunt57]

@chrisvblemos
I tried changing the transformation from list to str inside _save_user_input_to_config(self, user_input=None) like this:

        if CONF_EXCLUDE_DEVICES in user_input:
            if isinstance(user_input[CONF_EXCLUDE_DEVICES], str):
                self.config[CONF_EXCLUDE_DEVICES] = (
                    user_input[CONF_EXCLUDE_DEVICES].split(',')
                    if user_input[CONF_EXCLUDE_DEVICES]
                    else []
                )
            else:
                self.config[CONF_EXCLUDE_DEVICES] = user_input[CONF_EXCLUDE_DEVICES]

but it continues to save the config entry in .storage as "exclude_devices": "Bedroom,Daniel's Fire,Echo Auto,Mach-E SYNC,Living Room,Main floor,Norton Court,Office,This Device",. Either this code user_input[CONF_EXCLUDE_DEVICES].split(',')`is incorrect, or it's something else.

I don't know why the integration changed from lists to strings for these parameters sometime between v2.0.0 and now.
The only place I see INCLUDE & EXCLUDE being used is at lines 1246-1247 & 537-565 in __init__.py to build its include and exclude filters to filter the list of potential devices.

I'm going to leave it you guys to figure out the best fix. Once you do, I can amend my PR with your recommended changes or, you can submit a new PR to fix my issue and I'll close this one. But I am done trying to wrap my my head around everything for now. Maybe in 3-6 months after I've readjusted my life, I can resume doing what I love doing...

[alandtse]

Thanks again for all you help. Good luck with your IRL stuff!