fix: sync local HTTP speed when online limit is applied

[zhaohuiw42]

Bug: https://pms.uniontech.com/bug-view-359485.html

[github-actions]

CLA Assistant Lite bot:
提交邮箱中包含我们的合作伙伴，但您似乎并非合作伙伴的成员或对接人，请联系相关对接人将您添加至组织之中，或由其重新发起 Pull Request。
The commit email domain belongs to one of our partners, but it seems you are not yet a member of the current organization, please contact the contact person to add you to the organization or let them submit the Pull Request.

_{You can retrigger this bot by commenting recheck in this Pull Request}

[deepin-ci-robot]

deepin pr auto review

这段 me analyze this git diff that modifies the download speed limit configuration logic in a Go application.

Summary of Changes

The changes modify how the system handles download speed limiting when switching between online and local speed limit modes. The key changes are:

1. Added reflect import for deep comparison of structs
2. Modified disableLocalSpeedLimitConfig() to accept a limitSpeed parameter
3. Updated the logic to sync the platform speed limit value when disabling local limits
4. Added a test case to verify the new behavior

Code Review

1. Syntax and Logic

Positive aspects:

• The code compiles and follows Go syntax correctly
• The logic flow is clear and follows the existing code style
• The test case is well-structured and verifies the expected behavior

Potential issues:

1. Reflect.DeepEqual usage: While functional, using reflection for struct comparison can be:

   • Slower than direct field comparison
   • Less type-safe at compile time
   • Harder to understand for other developers

2. Empty string check: The check if limitSpeed != "" is a simple way to determine if a new speed value should be set, but it doesn't validate if the provided speed is actually a valid number.

3. Error handling: The function doesn't validate if limitSpeed is a valid speed value before setting it.

2. Code Quality

Positive aspects:

• Good variable naming
• Consistent formatting
• Added test case to verify new behavior
• Proper error handling for JSON marshaling/unmarshaling

Suggestions for improvement:

1. Consider adding validation for the limitSpeed parameter to ensure it's a valid speed value.

2. The comment "如果启用了在线限速，禁用本地限速，并将本地限速速度同步为平台下发的限速值" is clear but could be more concise.

3. Consider using a helper function to compare the structs instead of reflection.

3. Code Performance

1. Reflection overhead: Using reflect.DeepEqual adds some performance overhead compared to direct field comparison. For this simple struct with only a few fields, it's not a significant concern, but it's worth noting.

2. JSON marshaling: The code always marshals the struct to JSON, even when no changes are detected. The early return with reflect.DeepEqual helps avoid unnecessary writes, but the comparison itself uses reflection.

4. Code Security

1. Input validation: There's no validation of the limitSpeed parameter. If this value comes from user input or an external source, it could potentially be used to inject invalid data.

2. JSON unmarshaling: The code unmarshals JSON from a configuration string without validating its structure or content. While this is handled by the JSON unmarshaler, it's worth considering if additional validation is needed.

Improvement Suggestions

1. Replace reflection with direct comparison:

// Instead of reflect.DeepEqual(downloadSpeed, oldDownloadSpeed)
if downloadSpeed == oldDownloadSpeed {
    return nil
}

This works because all fields in the struct are comparable (no slices, maps, or functions).

2. Add validation for limitSpeed:

func (m *Manager) disableLocalSpeedLimitConfig(limitSpeed string) error {
    // Validate limitSpeed if provided
    if limitSpeed != "" {
        if _, err := strconv.Atoi(limitSpeed); err != nil {
            return fmt.Errorf("invalid speed limit value: %w", err)
        }
    }
    
    // Rest of the function...
}

3. Consider making the struct comparison more explicit:

if downloadSpeed.DownloadSpeedLimitEnabled == oldDownloadSpeed.DownloadSpeedLimitEnabled &&
   downloadSpeed.LimitSpeed == oldDownloadSpeed.LimitSpeed &&
   downloadSpeed.IsOnlineSpeedLimit == oldDownloadSpeed.IsOnlineSpeedLimit {
    return nil
}

4. Add more comprehensive test cases:

func TestDisableLocalSpeedLimitConfigSyncsPlatformSpeedOnly(t *testing.T) {
    tests := []struct {
        name     string
        config   string
        newSpeed string
        want     downloadSpeedLimitConfig
    }{
        {
            name:     "update speed with valid value",
            config:   `{"DownloadSpeedLimitEnabled":true,"LimitSpeed":"888","IsOnlineSpeedLimit":false}`,
            newSpeed: "666",
            want: downloadSpeedLimitConfig{
                DownloadSpeedLimitEnabled: false,
                LimitSpeed:                "666",
                IsOnlineSpeedLimit:        false,
            },
        },
        {
            name:     "keep existing speed with empty value",
            config:   `{"DownloadSpeedLimitEnabled":true,"LimitSpeed":"888","IsOnlineSpeedLimit":false}`,
            newSpeed: "",
            want: downloadSpeedLimitConfig{
                DownloadSpeedLimitEnabled: false,
                LimitSpeed:                "888",
                IsOnlineSpeedLimit:        false,
            },
        },
        // Add more test cases as needed
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            manager := &Manager{
                config: &config.Config{
                    LocalDownloadSpeedLimitConfig: tt.config,
                },
            }

            err := manager.disableLocalSpeedLimitConfig(tt.newSpeed)
            if err != nil {
                t.Fatalf("disableLocalSpeedLimitConfig() error = %v", err)
            }

            var got downloadSpeedLimitConfig
            if err := json.Unmarshal([]byte(manager.config.LocalDownloadSpeedLimitConfig), &got); err != nil {
                t.Fatalf("LocalDownloadSpeedLimitConfig unmarshal error = %v", err)
            }

            if got != tt.want {
                t.Fatalf("LocalDownloadSpeedLimitConfig = %+v, want %+v", got, tt.want)
            }
        })
    }
}

Conclusion

The changes implement the desired functionality of syncing the platform speed limit value when disabling local limits. The code is generally well-written and follows good practices. The main areas for improvement are:

1. Replace reflection with direct comparison for better performance and clarity
2. Add validation for the speed limit value
3. Expand test coverage with more test cases

These improvements would make the code more robust, maintainable, and efficient.

[deepin-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qiuzhiqian, zhaohuiw42

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• debian/deepin/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment