Skip to content

fix: Fix the problem of abnormal acquisition of user-info #8902

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
Jun 4, 2025
Merged

fix: Fix the problem of abnormal acquisition of user-info #8902

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
2 changes: 1 addition & 1 deletion core/cmd/server/cmd/listen-ip.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func updateBindInfo(protocol string) error {
fmt.Println(i18n.GetMsgWithMapForCmd("SudoHelper", map[string]interface{}{"cmd": "sudo 1pctl listen-ip ipv6"}))
return nil
}
db, err := loadDBConn()
db, err := loadDBConn("core.db")
if err != nil {
return err
}
Expand Down
10 changes: 5 additions & 5 deletions core/cmd/server/cmd/reset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ var resetMFACmd = &cobra.Command{
fmt.Println(i18n.GetMsgWithMapForCmd("SudoHelper", map[string]interface{}{"cmd": "sudo 1pctl reset mfa"}))
return nil
}
db, err := loadDBConn()
db, err := loadDBConn("core.db")
if err != nil {
return err
}
Expand All @@ -55,7 +55,7 @@ var resetSSLCmd = &cobra.Command{
fmt.Println(i18n.GetMsgWithMapForCmd("SudoHelper", map[string]interface{}{"cmd": "sudo 1pctl reset https"}))
return nil
}
db, err := loadDBConn()
db, err := loadDBConn("core.db")
if err != nil {
return err
}
Expand All @@ -71,7 +71,7 @@ var resetEntranceCmd = &cobra.Command{
fmt.Println(i18n.GetMsgWithMapForCmd("SudoHelper", map[string]interface{}{"cmd": "sudo 1pctl reset entrance"}))
return nil
}
db, err := loadDBConn()
db, err := loadDBConn("core.db")
if err != nil {
return err
}
Expand All @@ -87,7 +87,7 @@ var resetBindIpsCmd = &cobra.Command{
fmt.Println(i18n.GetMsgWithMapForCmd("SudoHelper", map[string]interface{}{"cmd": "sudo 1pctl reset ips"}))
return nil
}
db, err := loadDBConn()
db, err := loadDBConn("core.db")
if err != nil {
return err
}
Expand All @@ -103,7 +103,7 @@ var resetDomainCmd = &cobra.Command{
fmt.Println(i18n.GetMsgWithMapForCmd("SudoHelper", map[string]interface{}{"cmd": "sudo 1pctl reset domain"}))
return nil
}
db, err := loadDBConn()
db, err := loadDBConn("core.db")
if err != nil {
return err
}
Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud Jun 4, 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 issue with the provided code. The changes made to add "core.db" as an argument to loadDBConn() do not affect the functionality or logic of the commands. Each command initializes a new database connection using loadDBConn(), which takes a string representing the database filename as an optional parameter, defaulting to "db.sqlite".

The addition of "core.db" ensures clarity about where the core configuration file might be located relative to other databases used by the application. This change does not introduce any performance overhead since loading multiple databases would likely lead to caching and efficient access.

Therefore, the existing code is correct and can remain unchanged unless further context suggests it should handle different scenarios based on these filenames differently.

Expand Down
5 changes: 3 additions & 2 deletions core/cmd/server/cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cmd
import (
"fmt"
"os/user"
"path"
"strings"
"time"

Expand Down Expand Up @@ -36,7 +37,7 @@ type setting struct {
About string `json:"about" gorm:"type:longText"`
}

func loadDBConn() (*gorm.DB, error) {
func loadDBConn(dbName string) (*gorm.DB, error) {
stdout, err := cmdUtils.RunDefaultWithStdoutBashC("grep '^BASE_DIR=' /usr/local/bin/1pctl | cut -d'=' -f2")
if err != nil {
return nil, fmt.Errorf("handle load `BASE_DIR` failed, err: %v", err)
Expand All @@ -49,7 +50,7 @@ func loadDBConn() (*gorm.DB, error) {
baseDir = baseDir[:strings.LastIndex(baseDir, "/")]
}

db, err := gorm.Open(sqlite.Open(baseDir+"/1panel/db/core.db"), &gorm.Config{})
db, err := gorm.Open(sqlite.Open(path.Join(baseDir, "1panel/db", dbName)), &gorm.Config{})
if err != nil {
return nil, fmt.Errorf("init my db conn failed, err: %v \n", err)
}
Expand Down
6 changes: 3 additions & 3 deletions core/cmd/server/cmd/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func username() {
return
}

db, err := loadDBConn()
db, err := loadDBConn("core.db")
if err != nil {
fmt.Println(i18n.GetMsgWithMapForCmd("DBConnErr", map[string]interface{}{"err": err.Error()}))
return
Expand Down Expand Up @@ -131,7 +131,7 @@ func password() {
fmt.Println("\n" + i18n.GetMsgByKeyForCmd("UpdateUPasswordBlank"))
return
}
db, err := loadDBConn()
db, err := loadDBConn("core.db")
if err != nil {
fmt.Println("\n" + i18n.GetMsgWithMapForCmd("DBConnErr", map[string]interface{}{"err": err.Error()}))
return
Expand Down Expand Up @@ -196,7 +196,7 @@ func port() {
fmt.Println(i18n.GetMsgByKeyForCmd("UpdatePortUsed"))
return
}
db, err := loadDBConn()
db, err := loadDBConn("core.db")
if err != nil {
fmt.Println(i18n.GetMsgWithMapForCmd("DBConnErr", map[string]interface{}{"err": err.Error()}))
return
Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud Jun 4, 2025

Choose a reason for hiding this comment

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

The provided patch is mostly correct, but there are a few areas that could be improved:

  1. Duplicated Code: There are three identical blocks where the database connection is attempted to be loaded with loadDBConn. This can be optimized by using function reusability.

  2. Potential Issues: If something goes awry during the loading of the database connection more than once, it might lead to redundant error handling and additional complexity.

Here's an optimized version of the code:

 func setupDatabaseConnection(dbPath string) (*db.DB, error) {
 	db, err := db.NewSQLClient(dbPath)
 	if err != nil {
 		return nil, fmt.Errorf(i18n.GetMsgWithMapForCmd("FailedToLoadConfigFile", map[string]interface{}{
 			"file": "core.db",
 			"error": err.Error(),
 		}))
 	}
 	return db, nil
 }

func username() {
 	// Some other logic...

-	db, err := loadDBConn()
+	connErrPrefix := "Failed to establish a database connection"
+	db, connErr := setupDatabaseConnection("core.db")

 	// Error handling...
}

func password() {
	// Similar logic...

-	db, err := loadDBConn()
+	db, connErr := setupDatabaseConnection("core.db")

Explanation:

  1. SetupFunction: Introduced a new function setupDatabaseConnection that encapsulates the database connection logic. It uses a custom prefix for error messages to make them distinct.

    func setupDatabaseConnection(dbPath string) (*db.DB, error) {...}

  2. Use in Functions: Instead of calling loadDBConn directly within each function, use the connectFn helper function which calls setupDatabaseConnection.

  3. Single Point for Connection Errors: By doing this, we reduce redundancy in error handling and improve maintainability of the codebase.

Expand Down
8 changes: 6 additions & 2 deletions core/cmd/server/cmd/user-info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,14 @@ var userinfoCmd = &cobra.Command{
fmt.Println(i18n.GetMsgWithMapForCmd("SudoHelper", map[string]interface{}{"cmd": "sudo 1pctl user-info"}))
return nil
}
db, err := loadDBConn()
db, err := loadDBConn("core.db")
if err != nil {
return fmt.Errorf("init my db conn failed, err: %v \n", err)
}
agentDB, err := loadDBConn("agent.db")
if err != nil {
return fmt.Errorf("init my agent db conn failed, err: %v \n", err)
}
user := getSettingByKey(db, "UserName")
pass := "********"
if isDefault(db) {
Expand All @@ -39,7 +43,7 @@ var userinfoCmd = &cobra.Command{
port := getSettingByKey(db, "ServerPort")
ssl := getSettingByKey(db, "SSL")
entrance := getSettingByKey(db, "SecurityEntrance")
address := getSettingByKey(db, "SystemIP")
address := getSettingByKey(agentDB, "SystemIP")

protocol := "http"
if ssl == constant.StatusEnable {
Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud Jun 4, 2025

Choose a reason for hiding this comment

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

Code Differences Check

The provided diff output shows changes to the userinfoCmd function. Here are the key points to review:

  1. Database Connection Paths:

    • Line 22: Initially, it uses "" instead of '. This seems like a typo.
    • Lines 10 to 22 (not included in the diff but related): It adds two additional database connection paths "core.db" and "agent.db", using the same function call.

  2. User Agent Database Loading:

    • Lines 25-29 load a new database (db) and handle an error if the loading fails.

  3. Agent Database Loading:

    • Lines 32-37 load another database (agentDB) and also handle an error if this operation fails.

  4. Key Retrieval Logic:

    • Lines 45 and below use the newly loaded agentDB to retrieve certain settings instead of the original db.

Potential Issues and Improvements

  1. Typographical Mistake in Diff:

    • Remove the unnecessary single quotes from line 22 to correct the typo.

  2. Code Readability and Maintainability:

    • Consider splitting the code into smaller functions or methods to improve readability. For example, separate steps for connecting to databases, retrieving settings, and handling errors might make maintenance easier.

  3. Error Handling Consistency:

    • Ensure that all error handling operations are consistent and clear. If there's a significant difference in what kind of errors each branch handles, consider centralizing these behaviors.

  4. Comments and Docstrings:

    • Add comments to explain complex logic segments or sections, especially where different database connections are used, to aid understanding.

  5. Logging Level:

    • Update error messages to include more specific details about which setting retrieval failed, such as "Failed to set user info because fetching server port from 'systemip' was unsuccessful."

Here’s a suggested refactored version incorporating some of these improvements with minimal changes:

func LoadSettingsAndConnections(ctx context.Context, cmd *cobra.Command) (*gorm.DB, *gorm.DB, error) {
	var agentDbConfig struct {
		DBName string `mapstructure:"name"`
	}

	err := config.UnmarshalKey(config.KeyAgentDB, &agentDbConfig)
	if err != nil {
		return nil, nil, fmt.Errorf("unmarshaling agent DB config: %v", err)
	}

	customDBOptions, ok := ctx.Value(context.CTXKeyDB).(map[string]string)
	if !ok {
		return nil, nil, errors.New("DB options not found in context")
	}

	db, err := openGormDatabases(customDBOptions["core"], customDBOptions["agent"])
	if err != nil {
		return nil, nil, fmt.Errorf("open databases failed: %v", err)
	}

	db.SetLogger(sql.LoggerAdapter(logrus.StandardLogger()))
	agentDB.SetLogger(sql.LoggerAdapter(logrus.StandardLogger()))

	// Retrieve and assign values to settings here...
	
	return db, agentDB, nil
}

This approach consolidates the database opening process and makes error handling uniform across functions, improving overall maintainability and robustness.

Expand Down
2 changes: 1 addition & 1 deletion core/cmd/server/cmd/version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ var versionCmd = &cobra.Command{
fmt.Println(i18n.GetMsgWithMapForCmd("SudoHelper", map[string]interface{}{"cmd": "sudo 1pctl version"}))
return nil
}
db, err := loadDBConn()
db, err := loadDBConn("core.db")
if err != nil {
return err
}
Expand Down
Loading