HQD-127: Refactor hub assignment logic and validation#279
Open
bladeroot wants to merge 1 commit into
Open
Conversation
- Improved AssignHubsForm with explicit model normalization and dynamic payload generation - Enhanced binding validation logic to correctly identify own device bindings - Refactored JS to use CSS classes instead of localized titles for column detection - Updated action to use structured attribute lists for API payloads
📝 WalkthroughWalkthroughThe PR refactors hub assignment logic to use explicit attribute-name definitions instead of underscore-based heuristics. Form adds ChangesHub Assignment Flow Refinement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/actions/AssignableHubs.php`:
- Around line 64-69: The code currently clones $form, loads request data into
it, then calls getHubPayloadAttributeNames() which causes the whitelist to be
derived from the posted state (including a crafted type) and merges into
$hub['hubs']; instead, create/resolve the allowed hub payload keys from the
persisted model/type (use setModelClass($this->getAssignableClassName()) on a
fresh model instance or otherwise instantiate the model for the persisted type
without loading request data), call getHubPayloadAttributeNames() on that
trusted model to get the whitelist, build a new array with only those keys from
the request (do not merge), assign it to $hub['hubs'] (replacing the whole
array), and remove the unset($hub[$key]) merge logic; use the same symbols shown
(clone $form, setModelClass, getAssignableClassName,
getHubPayloadAttributeNames, load/validate) to locate and update the code.
In `@src/forms/AssignHubsForm.php`:
- Around line 306-313: The current prefix check in AssignHubsForm (loop over
$binding->device_name / $binding->base_device_name comparing $bindingName and
$name using str_starts_with + ctype_digit) is too permissive; either compare
stable identifiers (e.g. $binding->device_id / $binding->base_device_id against
the current device ID) if those IDs are available, or replace the
prefix+ctype_digit logic with a stricter anchored pattern that only treats exact
matches or the specific alias form you intend (for example only allow a single
separator plus numeric suffix like "name-123"); update the check around
$bindingName, $name and the str_starts_with branch accordingly so names like
"server-backup" or "serverx" no longer falsely count as the same device.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6941ee5-3c63-4d0f-9e9c-16520826aa5d
📒 Files selected for processing (3)
src/actions/AssignableHubs.phpsrc/assets/js/assign-hubs-column-reveal.jssrc/forms/AssignHubsForm.php
Comment on lines
64
to
+69
| $model = clone $form; | ||
| $model::setModelClass($this->getAssignableClassName()); | ||
| if ($model->load($hub, '') && $model->validate()) { | ||
| foreach ($model->toArray() as $key => $value) { | ||
| if (str_contains($key, '_')) { | ||
| $hub['hubs'][$key] = $value ?? ''; | ||
| unset($hub[$key]); | ||
| } | ||
| foreach ($model->getHubPayloadAttributeNames() as $key) { | ||
| $hub['hubs'][$key] = $model->{$key} ?? ''; | ||
| unset($hub[$key]); |
There was a problem hiding this comment.
Don't derive the hub payload whitelist from posted state.
$model is a blank clone here, so getHubPayloadAttributeNames() is driven by request-loaded attributes, including type. A crafted payload can switch to a broader variant set and also preserve extra keys inside an existing hubs array, because this code merges into $hub['hubs'] instead of rebuilding it. Resolve the allowed keys from the persisted model/type and replace hubs wholesale before sending the API payload.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/actions/AssignableHubs.php` around lines 64 - 69, The code currently
clones $form, loads request data into it, then calls
getHubPayloadAttributeNames() which causes the whitelist to be derived from the
posted state (including a crafted type) and merges into $hub['hubs']; instead,
create/resolve the allowed hub payload keys from the persisted model/type (use
setModelClass($this->getAssignableClassName()) on a fresh model instance or
otherwise instantiate the model for the persisted type without loading request
data), call getHubPayloadAttributeNames() on that trusted model to get the
whitelist, build a new array with only those keys from the request (do not
merge), assign it to $hub['hubs'] (replacing the whole array), and remove the
unset($hub[$key]) merge logic; use the same symbols shown (clone $form,
setModelClass, getAssignableClassName, getHubPayloadAttributeNames,
load/validate) to locate and update the code.
Comment on lines
+306
to
+313
| foreach ([$binding->device_name ?? null, $binding->base_device_name ?? null] as $bindingName) { | ||
| if ($bindingName === null || $bindingName === '') { | ||
| continue; | ||
| } | ||
| $bindingName = (string)$bindingName; | ||
| $name = (string)$name; | ||
| if ($bindingName === $name || (str_starts_with($bindingName, $name) && !ctype_digit($bindingName[strlen($name)] ?? ''))) { | ||
| return true; |
There was a problem hiding this comment.
Tighten the own-binding name check.
Line 312 treats any non-digit suffix as “own”, so a device named server will also match bindings for server-backup or serverx. That can suppress the “already taken” error for a real conflict. Please constrain this to the exact alias pattern you intend to support, or compare stable identifiers instead of prefix-based names.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/forms/AssignHubsForm.php` around lines 306 - 313, The current prefix
check in AssignHubsForm (loop over $binding->device_name /
$binding->base_device_name comparing $bindingName and $name using
str_starts_with + ctype_digit) is too permissive; either compare stable
identifiers (e.g. $binding->device_id / $binding->base_device_id against the
current device ID) if those IDs are available, or replace the prefix+ctype_digit
logic with a stricter anchored pattern that only treats exact matches or the
specific alias form you intend (for example only allow a single separator plus
numeric suffix like "name-123"); update the check around $bindingName, $name and
the str_starts_with branch accordingly so names like "server-backup" or
"serverx" no longer falsely count as the same device.
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.
Summary by CodeRabbit