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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle GeoJSON int to str conversion when the name is an int #108937

Merged
merged 1 commit into from Feb 4, 2024

Conversation

codyc1515
Copy link
Contributor

@codyc1515 codyc1515 commented Jan 27, 2024

Proposed change

Handles an error that could occur on setting up the config entry when generating the entity_id slug from the name if the name from the GeoJSON feed was an int (not a str).

This was an unexpected regression from #108753 and should be included before the new release in order to avoid breaking the integration unexpectedly.

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)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue:
Logger: homeassistant
Source: util/__init__.py:47
First occurred: 17:07:48 (3956 occurrences)
Last logged: 17:17:41

Error doing job: Task exception was never retrieved Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 507, in async_add_entities
    await asyncio.gather(*tasks)
  File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 658, in _async_add_entity
    entry = entity_registry.async_get_or_create(
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/entity_registry.py", line 661, in async_get_or_create
    entity_id = self.async_generate_entity_id(
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/entity_registry.py", line 581, in async_generate_entity_id
    preferred_string = f"{domain}.{slugify(suggested_object_id)}"
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/util/__init__.py", line 47, in slugify
    slug = unicode_slug.slugify(text, separator=separator)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/slugify/slugify.py", line 104, in slugify
    text = _unicode(text, 'utf-8', 'ignore')
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: decoding to str: need a bytes-like object, int found

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][dev-checklist]
  • I have followed the [perfect PR recommendations][perfect-pr]
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @exxamalte, mind taking a look at this pull request as it has been labeled with an integration (geo_json_events) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of geo_json_events can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign geo_json_events Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@codyc1515 codyc1515 marked this pull request as draft January 27, 2024 04:55
@codyc1515 codyc1515 force-pushed the geojson_name_slugify branch 6 times, most recently from 38cea7d to 10c8883 Compare January 27, 2024 20:28
@codyc1515 codyc1515 marked this pull request as ready for review January 27, 2024 20:31
@codyc1515 codyc1515 marked this pull request as draft January 27, 2024 20:32
@codyc1515 codyc1515 marked this pull request as ready for review January 27, 2024 20:53
codyc1515 added a commit to codyc1515/core that referenced this pull request Jan 27, 2024
I have seen some GeoJSON feeds that use capitalised name (`Name`) and some feeds that use non-capitalised name (`name`).

The GeoJSON spec itself doesn't speak to this as these are optional (albeit, often used) `properties`.

Follow on from home-assistant#108937.
codyc1515 added a commit to codyc1515/core that referenced this pull request Jan 27, 2024
I have seen some GeoJSON feeds that use capitalised name (`Name`) and some feeds that use non-capitalised name (`name`).

The GeoJSON spec itself doesn't speak to this as these are optional (albeit, often used) `properties`.

Follow on from home-assistant#108937.
@exxamalte
Copy link
Contributor

I was just thinking of an alternative where we enforce this type conversion to str in the underlying library?
https://github.com/exxamalte/python-aio-geojson-generic-client/blob/main/aio_geojson_generic_client/feed_entry.py#L26

@NasaGeek
Copy link
Contributor

I was just thinking of an alternative where we enforce this type conversion to str in the underlying library? https://github.com/exxamalte/python-aio-geojson-generic-client/blob/main/aio_geojson_generic_client/feed_entry.py#L26

I think that would be a good change (especially considering that the lib already claims to return str), but since this PR is also working with the name property which the underlying library has no opinions about, there would still be necessary changes to handle that. Generally I do think that this PR shouldn't be concerned with the .title property.

@codyc1515
Copy link
Contributor Author

codyc1515 commented Jan 28, 2024

I have done some further investigating. It turns out that the code which was causing this error (below):

def slugify(text: str | None, *, separator: str = "_") -> str:
"""Slugify a given text."""
if text == "" or text is None:
return ""
slug = unicode_slug.slugify(text, separator=separator)
return "unknown" if slug == "" else slug

Passes on any values from the entity name, including those names that are not str (as we are doing here with int), to the function unicode_slug.slugify. However, slugify does not, and will not, support any non-str values.

Should we not fix it at the root then (rather than in each integration)? Maybe I should raise a separate PR for that.

@codyc1515
Copy link
Contributor Author

Closing in favour of #108989. Will re-open this if that one was not accepted.

@codyc1515 codyc1515 closed this Jan 28, 2024
@NasaGeek
Copy link
Contributor

I think we were trending in right direction with this PR, as an entity's name property is expected to be a str (as indicated by the typing at

def name(self) -> str | UndefinedType | None:
). There could be other misbehavior we haven't yet encountered if we allow the name to remain an int and try to fix things downstream in the slugify call.

@codyc1515 codyc1515 reopened this Jan 28, 2024
@codyc1515
Copy link
Contributor Author

codyc1515 commented Jan 28, 2024

So what’s the proposal from here? All checks pass already and we handle the int scenario.

@codyc1515 codyc1515 marked this pull request as draft January 28, 2024 23:03
Resolves error:

Logger: homeassistant
Source: util/__init__.py:47
First occurred: 17:07:48 (3956 occurrences)
Last logged: 17:17:41

Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 507, in async_add_entities
    await asyncio.gather(*tasks)
  File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 658, in _async_add_entity
    entry = entity_registry.async_get_or_create(
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/entity_registry.py", line 661, in async_get_or_create
    entity_id = self.async_generate_entity_id(
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/helpers/entity_registry.py", line 581, in async_generate_entity_id
    preferred_string = f"{domain}.{slugify(suggested_object_id)}"
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/homeassistant/homeassistant/util/__init__.py", line 47, in slugify
    slug = unicode_slug.slugify(text, separator=separator)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/slugify/slugify.py", line 104, in slugify
    text = _unicode(text, 'utf-8', 'ignore')
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: decoding to str: need a bytes-like object, int found

Co-Authored-By: Chris Roberts <NasaGeek@users.noreply.github.com>
@codyc1515 codyc1515 marked this pull request as ready for review January 28, 2024 23:15
@codyc1515
Copy link
Contributor Author

I have incorporated all of the feedback now.

Copy link
Contributor

@NasaGeek NasaGeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @codyc1515! I'm happy where this ended up.

@codyc1515
Copy link
Contributor Author

If this PR is not merged soon, 2024.2 release will be broken for some GeoJSON users.

@joostlek joostlek added this to the 2024.2.0 milestone Feb 4, 2024
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @codyc1515 👍

../Frenck

@frenck frenck merged commit 770119c into home-assistant:dev Feb 4, 2024
23 checks passed
@codyc1515 codyc1515 deleted the geojson_name_slugify branch February 4, 2024 21:11
Balake pushed a commit to DeakoLights/homeassistant_core that referenced this pull request Feb 4, 2024
…sistant#108937)

Co-authored-by: Chris Roberts <NasaGeek@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants