Skip to content

fix: Fix the problem of failed MySQL permission modification #8351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
f2c-ci-robot merged 1 commit into from
Apr 8, 2025
Merged

fix: Fix the problem of failed MySQL permission modification #8351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions backend/app/service/cronjob_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,12 +350,18 @@ func (u *CronjobService) removeExpiredBackup(cronjob model.Cronjob, accountMap m
if cronjob.Type == "snapshot" {
for _, account := range accounts {
if len(account) != 0 {
if _, ok := accountMap[account]; !ok {
continue
}
_, _ = accountMap[account].client.Delete(path.Join(accountMap[account].backupPath, "system_snapshot", records[i].FileName))
}
}
_ = snapshotRepo.Delete(commonRepo.WithByName(strings.TrimSuffix(records[i].FileName, ".tar.gz")))
} else {
for _, account := range accounts {
if _, ok := accountMap[account]; !ok {
continue
}
if len(account) != 0 {
_, _ = accountMap[account].client.Delete(path.Join(accountMap[account].backupPath, records[i].FileDir, records[i].FileName))
}
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes made do not have significant irregularities or issues.

However, there is room for improvement:

  1. Comments: The comment about skipping accounts that cannot be found in the map can be moved outside of the loop if you want to reduce redundant checks within each iteration cycle.

    for _, account := range accounts {
    if _, ok := accountMap[account]; !ok {
        continue
    }
    // Rest of the deletion logic
}

  2. Code Readability:

    • The line if _, ok := accountMap[account]; !ok { could use continue; on its own per iteration instead of repeating the expression multiple times.
    • Consider extracting the client creation logic into a helper function to maintain cleaner code structure.

Overall, the patch looks well-maintained and optimized given the current constraints.

Expand Down
25 changes: 13 additions & 12 deletions backend/utils/mysql/client/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,18 +188,19 @@ func (r *Local) ChangeAccess(info AccessChangeInfo) error {
info.Name = "*"
info.Password = r.Password
}
if info.Permission != info.OldPermission {
if err := r.Delete(DeleteInfo{
Version: info.Version,
Username: info.Username,
Permission: info.OldPermission,
ForceDelete: true,
Timeout: 300}); err != nil {
return err
}
if info.Username == "root" {
return nil
}
if info.Permission == info.OldPermission {
return nil
}
if err := r.Delete(DeleteInfo{
Version: info.Version,
Username: info.Username,
Permission: info.OldPermission,
ForceDelete: true,
Timeout: 300}); err != nil {
return err
}
if info.Username == "root" {
return nil
}
if err := r.CreateUser(CreateInfo{
Name: info.Name,
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code provided contains a few issues and potential improvements:

  1. Redundancy in Delete Method Call: The first branch of the conditional statement deletes the user with old permissions, while the second branch does not. This can lead to unintended behavior if there are no existing users with that permission.

  2. Handling Root User Deletion: The root user is handled differently based on whether the condition is met within the loop or after it. However, this logic seems unnecessary given how the root user deletion should only occur when all checks pass.

  3. Optimization Suggestion:

    • Remove the redundant info.Name = "*". It doesn't affect function behavior but could be removed for clarity.

Here's the cleaned-up version:

func (r *Local) ChangeAccess(info AccessChangeInfo) error {
	info.Password = r.Password

	switch {
	case info.Permission != "":
		if err := r.Delete(DeleteInfo{
			Version:     info.Version,
			Username:    info.Username,
			Permission:  info.OldPermission,
			ForceDelete: true,
			Timeout:     300}); err != nil {
			return err
		}

		if info.Username == "root" {
			return nil
		}
	default:
		if err := r.createUser(CreateInfo{
			Name:       info.Name,
            // Rest of the createUser parameters
		}); err != nil {
			return err
		}
		return r.changeUserAccess(UserModifyOptions{ // Assuming changeUserAccess exists
			OldPermissions: []string{info.OldPermission},
			NewPermissions: []string{info.Permission},
			Usernames:      []string{info.Username},
			Version:        info.Version,
		})
	}
}

Key Improvements Made:

  • Simplified the conditional structure using a switch, making the code more concise.
  • Removed the info.Name = "*" assignment since it wasn't doing anything meaningful.
  • Added a default case to handle cases where info.Permission isn’t an empty string, ensuring proper handling of both new and permission changes.

Expand Down
25 changes: 13 additions & 12 deletions backend/utils/mysql/client/remote.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,18 +199,19 @@ func (r *Remote) ChangeAccess(info AccessChangeInfo) error {
info.Name = "*"
info.Password = r.Password
}
if info.Permission != info.OldPermission {
if err := r.Delete(DeleteInfo{
Version: info.Version,
Username: info.Username,
Permission: info.OldPermission,
ForceDelete: true,
Timeout: 300}); err != nil {
return err
}
if info.Username == "root" {
return nil
}
if info.Permission == info.OldPermission {
return nil
}
if err := r.Delete(DeleteInfo{
Version: info.Version,
Username: info.Username,
Permission: info.OldPermission,
ForceDelete: true,
Timeout: 300}); err != nil {
return err
}
if info.Username == "root" {
return nil
}
if err := r.CreateUser(CreateInfo{
Name: info.Name,
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no significant irregularity, issue, or optimization suggestion in the provided code snippet. The changes you made to modify permission handling do not seem to introduce any new problems or improve efficiency beyond what was already present in the original version.

Expand Down
1 change: 1 addition & 0 deletions frontend/src/views/cronjob/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ const onDelete = async (row: Cronjob.CronjobInfo | null) => {
let ids = [];
showClean.value = false;
cleanData.value = false;
cleanRemoteData.value = true;
if (row) {
ids = [row.id];
names = [row.name];
Expand Down
Loading