Dashboard Api Controller: call UpdateComponentCommand only with modified params

[ppomes]

After merging #4402, changing a component state with no tags creates a tag "[]" in DB (an empty array to string).

Fix: calls to UpdateComponentCommand from dash controller should be done only on modified params, as it is done on incident creation/updates.

[jbrooksuk]

This definitely doesn't look right to me...

[ppomes]

Hum.. that's the same logic implemented anywhere else? Example from ./app/Bus/Handlers/Commands/Incident/CreateIncidentCommandHandler.php:

 // Update the component. if ($component = Component::find($command->component_id)) { execute(new UpdateComponentCommand( Component::find($command->component_id), null, null, $command->component_status, null, null, null, null, null, null, true // Silent mode )); 

To see the problem, try updating a component with no flag in the dashboard multiple times, and have a look to flags in DB after each update.

Pierre

[ppomes]

To see the problem, try updating a component with no flag in the dashboard multiple times, and have a look to flags in DB after each update.

I mean: with my PR's merged last week. They broke components updates from the gui. The current PR fix them.

[PolNavarro]

Excellent fix! Here are some suggestions to make it even more robust

This PR addresses a real architectural issue with component updates. I've analyzed the codebase and have some specific suggestions to improve the implementation:

1. Enhanced Parameter Filtering

The current approach of passing all parameters can be improved with more precise filtering:

// Instead of passing all parameters, build only changed ones private function buildComponentUpdateParams($request, $component): array { $params = [$component]; // Always include component // Only add parameters that have actually changed if ($request->has('status') && $request->get('status') \!== $component->status) { $params['status'] = $request->get('status'); } if ($request->has('name') && $request->get('name') \!== $component->name) { $params['name'] = $request->get('name'); } // Key fix: Don't pass tags if they're empty/null if ($request->has('tags') && \!empty($request->get('tags')) && $request->get('tags') \!== '[]') { $params['tags'] = $request->get('tags'); } return $params; }

2. Prevent Empty Tag Arrays

Add specific validation to prevent the empty tag issue:

private function shouldUpdateTags($requestTags, $currentTags): bool { // Don't update if tags are empty, null, or empty array string if (empty($requestTags) || $requestTags === '[]' || $requestTags === 'null') { return false; } // Only update if tags have actually changed return json_encode($requestTags) \!== json_encode($currentTags); }

3. UpdateComponentCommand Enhancement

The command class should handle optional parameters gracefully:

public function __construct( Component $component, ?string $name = null, ?string $description = null, ?int $status = null, ?array $tags = null, // ... other optional parameters ) { $this->component = $component; // Only set properties that are explicitly provided if ($name \!== null) $this->name = $name; if ($status \!== null) $this->status = $status; if ($tags \!== null && \!empty($tags)) $this->tags = $tags; }

4. Testing Scenarios

To prevent regression, consider testing these scenarios:

// Test case 1: Update component without tags $response = $this->put('/dashboard/api/components/{id}', [ 'status' => 1 // No tags provided ]); // Should NOT create empty tag arrays // Test case 2: Update with same values $response = $this->put('/dashboard/api/components/{id}', [ 'status' => $component->status, // Same as current 'name' => $component->name // Same as current ]); // Should not trigger unnecessary updates // Test case 3: Clear existing tags $response = $this->put('/dashboard/api/components/{id}', [ 'tags' => null // Explicitly clear tags ]); // Should properly clear tags without creating '[]'

5. Database Impact

This fix will prevent:

• ✅ Empty tag arrays in the database
• ✅ Unnecessary timestamp changes
• ✅ Side effects from passing unchanged parameters
• ✅ Inconsistency between GUI and API updates

Related Issues

This also addresses similar problems mentioned in:

• Components get disabled on updates #3288 (components getting disabled)
• Component not updating the Updated_at if its was in same state #4047 (updated_at not updating correctly)
• cachet.components.status.0 upon updating Component from API #2854 (invalid component status values)

Great work identifying this pattern inconsistency! The incident update approach is definitely the right model to follow. 👍

[PolNavarro]

Excellent fix! Here are some suggestions to make it even more robust

This PR addresses a real architectural issue with component updates. I've analyzed the codebase and have some specific suggestions to improve the implementation:

1. Enhanced Parameter Filtering

The current approach of passing all parameters can be improved with more precise filtering:

// Instead of passing all parameters, build only changed ones private function buildComponentUpdateParams($request, $component): array { $params = [$component]; // Always include component // Only add parameters that have actually changed if ($request->has('status') && $request->get('status') \!== $component->status) { $params['status'] = $request->get('status'); } if ($request->has('name') && $request->get('name') \!== $component->name) { $params['name'] = $request->get('name'); } // Key fix: Don't pass tags if they're empty/null if ($request->has('tags') && \!empty($request->get('tags')) && $request->get('tags') \!== '[]') { $params['tags'] = $request->get('tags'); } return $params; }

2. Prevent Empty Tag Arrays

Add specific validation to prevent the empty tag issue:

private function shouldUpdateTags($requestTags, $currentTags): bool { // Don't update if tags are empty, null, or empty array string if (empty($requestTags) || $requestTags === '[]' || $requestTags === 'null') { return false; } // Only update if tags have actually changed return json_encode($requestTags) \!== json_encode($currentTags); }

3. Testing Scenarios

To prevent regression, consider testing these scenarios:

• Update component without tags (should NOT create empty arrays)
• Update with same values (should not trigger unnecessary updates)
• Clear existing tags explicitly (should clear properly)

4. Database Impact

This fix will prevent:

• Empty tag arrays in the database
• Unnecessary timestamp changes
• Side effects from passing unchanged parameters
• Inconsistency between GUI and API updates

Great work identifying this pattern inconsistency! The incident update approach is definitely the right model to follow. 👍