Add create and delete services for input number helpers#1222
Open
potelux wants to merge 2 commits intofrenck:mainfrom
Open
Add create and delete services for input number helpers#1222potelux wants to merge 2 commits intofrenck:mainfrom
potelux wants to merge 2 commits intofrenck:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two new admin services for Home Assistant Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CreateService
participant StorageCollection
participant InputNumberEntity
rect rgba(200,200,255,0.5)
Client->>CreateService: Call create service (payload)
CreateService->>CreateService: Validate schema (CREATE_FIELDS + _cv_input_number)
CreateService->>StorageCollection: Resolve via hass.data.websocket_api input_number/list
alt object_id provided
CreateService->>StorageCollection: Check id_manager.has_id(object_id)
alt exists
CreateService-->>Client: Raise HomeAssistantError (ID collision)
else
CreateService->>CreateService: Acquire create lock
CreateService->>StorageCollection: Override _get_suggested_id -> object_id
CreateService->>StorageCollection: async_create_item(payload)
CreateService->>StorageCollection: Restore _get_suggested_id
CreateService->>CreateService: Release lock
StorageCollection-->>InputNumberEntity: Item created
InputNumberEntity-->>Client: Success
end
else object_id not provided
CreateService->>StorageCollection: async_create_item(payload)
StorageCollection-->>InputNumberEntity: Item created
InputNumberEntity-->>Client: Success
end
end
sequenceDiagram
participant Client
participant DeleteService
participant InputNumberEntity
participant StorageCollection
rect rgba(200,255,200,0.5)
Client->>DeleteService: Call delete service (target entity)
DeleteService->>InputNumberEntity: Check entity.editable
alt Not editable
DeleteService-->>Client: Raise HomeAssistantError
else Editable
DeleteService->>StorageCollection: Resolve via hass.data.websocket_api input_number/list
DeleteService->>StorageCollection: async_delete_item(entity.unique_id)
StorageCollection-->>Client: Item deleted
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom_components/spook/ectoplasms/input_number/services/create.py`:
- Around line 64-69: The code mutates the shared private attribute
collection._get_suggested_id around an await (collection.async_create_item)
which can race between concurrent service calls; wrap the override and the await
in an async lock stored on hass to serialize access: acquire an asyncio.Lock
from self.hass.data.setdefault("spook_input_number_lock", asyncio.Lock()) and
perform original_get_suggested_id = collection._get_suggested_id,
collection._get_suggested_id = lambda info: object_id, await
collection.async_create_item(data) inside an async with block, and restore
collection._get_suggested_id in a finally to ensure proper cleanup.
In `@custom_components/spook/services.yaml`:
- Around line 1010-1013: The numeric service schema sets number.min to 0.001
which is stricter than backend validation and can cause UI rejections; update
the number schema (keys: number, min, step) to align with backend by raising the
allowed range to the backend minimum (e.g., set min to 0 or remove the min
constraint) so the UI accepts values the backend already permits while keeping
step: any.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8c7e0b62-f8e6-4218-a05d-57416bf66a45
📒 Files selected for processing (3)
custom_components/spook/ectoplasms/input_number/services/create.pycustom_components/spook/ectoplasms/input_number/services/delete.pycustom_components/spook/services.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom_components/spook/ectoplasms/input_number/services/create.py`:
- Around line 61-72: The has_id check on collection.id_manager.has_id is outside
the create lock, causing a TOCTOU race; move that check inside the async with
self.hass.data.setdefault(f"{DOMAIN}_create_lock", asyncio.Lock()) block so you
acquire the lock before calling collection.id_manager.has_id, and only then
raise HomeAssistantError if the id exists; keep the existing pattern of setting
collection._get_suggested_id to return object_id, awaiting
collection.async_create_item(data), and deleting collection._get_suggested_id in
the finally block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9427b9f2-c1cf-4ebb-b98e-756788c3a1ac
📒 Files selected for processing (2)
custom_components/spook/ectoplasms/input_number/services/create.pycustom_components/spook/services.yaml
potelux
force-pushed
the
add-input-number-create-delete-services
branch
3 times, most recently
from
0e4b352 to
a3e785e
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
custom_components/spook/ectoplasms/input_number/services/create.py (1)
61-67:⚠️ Potential issue | 🟠 MajorMove duplicate-ID validation inside the lock.
Line 62 still checks
has_idbefore acquiring the lock on Lines 65–67, so concurrent calls can pass the check and race at create time.🔧 Suggested patch
if object_id: - if collection.id_manager.has_id(object_id): - message = f"An input number with object ID '{object_id}' already exists" - raise HomeAssistantError(message) async with self.hass.data.setdefault( f"{DOMAIN}_create_lock", asyncio.Lock() ): + if collection.id_manager.has_id(object_id): + message = f"An input number with object ID '{object_id}' already exists" + raise HomeAssistantError(message) collection._get_suggested_id = lambda info: object_id # noqa: SLF001 try: await collection.async_create_item(data) finally: del collection._get_suggested_id # noqa: SLF001🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom_components/spook/ectoplasms/input_number/services/create.py` around lines 61 - 67, The duplicate-ID check using collection.id_manager.has_id currently runs before acquiring the creation lock (the async with self.hass.data.setdefault(f"{DOMAIN}_create_lock", asyncio.Lock())), causing a race; move the has_id validation inside the async with block that uses that lock (i.e., acquire the lock first, then call collection.id_manager.has_id(object_id) and raise HomeAssistantError if true) so concurrent create calls re-check under the same lock and cannot both proceed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@custom_components/spook/ectoplasms/input_number/services/create.py`:
- Around line 61-67: The duplicate-ID check using collection.id_manager.has_id
currently runs before acquiring the creation lock (the async with
self.hass.data.setdefault(f"{DOMAIN}_create_lock", asyncio.Lock())), causing a
race; move the has_id validation inside the async with block that uses that lock
(i.e., acquire the lock first, then call collection.id_manager.has_id(object_id)
and raise HomeAssistantError if true) so concurrent create calls re-check under
the same lock and cannot both proceed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43f3dfcf-d7b5-4ea8-abd6-3a26e22d9ebc
📒 Files selected for processing (2)
custom_components/spook/ectoplasms/input_number/services/create.pycustom_components/spook/services.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- custom_components/spook/services.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom_components/spook/ectoplasms/input_number/services/delete.py`:
- Around line 37-40: The current access to the storage collection via
self.hass.data["websocket_api"]["input_number/list"][0].__self__.storage_collection
is fragile; update the logic in delete.py to first try the standard stored
collection under self.hass.data[DOMAIN] (the same pattern used in
zone/delete.py) and only if that key is missing fall back to the websocket_api
lookup, then call collection.async_delete_item(entity.unique_id); locate and
adjust the code around the variable collection and the async_delete_item call to
implement this conditional lookup (use the DOMAIN key constant as in other
services).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6a629740-4190-41cd-a1dc-7169a588689a
📒 Files selected for processing (3)
custom_components/spook/ectoplasms/input_number/services/create.pycustom_components/spook/ectoplasms/input_number/services/delete.pycustom_components/spook/services.yaml
✅ Files skipped from review due to trivial changes (1)
- custom_components/spook/services.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- custom_components/spook/ectoplasms/input_number/services/create.py
potelux
force-pushed
the
add-input-number-create-delete-services
branch
2 times, most recently
from
f523f56 to
5783ab9
Compare
Author
|
The code rabbit suggestion about async locks is not completely clear to me. I had Claude assist me in implementing the suggestion and was able to test and confirm its functionality. Reading over the code, it seems logical, but I am not knowledgeable enough on async to know if I am missing a key concept. Additionally, I corrected an issue where I was setting the underlying object ID, and not the entity ID. |
potelux
force-pushed
the
add-input-number-create-delete-services
branch
from
5783ab9 to
cca4f7f
Compare
potelux
force-pushed
the
add-input-number-create-delete-services
branch
2 times, most recently
from
6247754 to
f45728a
Compare
Author
|
@frenck -- I would like to implement the same thing for Text Sensor. You don't need to review this now, but is it a feature you'd actually consider? I don't want to implement the Text Sensor one, too, if both will end up rejected. |
potelux
force-pushed
the
add-input-number-create-delete-services
branch
from
f45728a to
ead5c7c
Compare
potelux
force-pushed
the
add-input-number-create-delete-services
branch
from
ead5c7c to
73ac0e0
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds create and delete services to the input_number integration, enabling helpers to be created and deleted programmatically at runtime without requiring a UI interaction or a configuration reload.
Motivation and Context
The primary use case (in my situation) is allowing the creation of robust blueprints that can also create their own necessary helpers. This makes it even easier for users to share their cool setups.
Specifically, I use this in my TagReader automation to create the NFC tag helper and in my Inovelli Global Configuration automation to have a single dashboard that can modify settings for all Inovelli devices.
How has this been tested?
I had originally created this for HomeAssistant core and had written working tests there. Now, I tested it manually checking for errors such as creating two helpers with the same entitiy id, out of range min/max, etc.
Screenshots (if appropriate):
Types of changes
Checklist