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. 👍