Skip to content
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.

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

feat: Optimize Location Information Display in System Login Logs #7459

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

zhengkunwang223
Copy link
Member

No description provided.

Copy link

f2c-ci-robot bot commented Dec 20, 2024

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@@ -91,6 +94,7 @@ func (u *LogService) PageLoginLog(req dto.SearchLgLogWithPage) (int64, interface
if err := copier.Copy(&item, &op); err != nil {
return 0, nil, errors.WithMessage(constant.ErrStructTransform, err.Error())
}
item.Address, _ = geo.GetIPLocation(item.IP, common.GetLang(c))
dtoOps = append(dtoOps, item)
}
return total, dtoOps, err
Copy link
Member

Choose a reason for hiding this comment

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

The provided code changes look mostly correct for the intended functionality. However, a few improvements could be made:

  1. Use of Interfaces: Ensure that all dependencies on model, dto, and other packages are properly imported at the beginning of each file where they are used to avoid unnecessary imports.

  2. Error Handling: Consider adding more detailed logging or user-facing errors when encountering specific types of failures during operation execution.

  3. Performance and Resource Management: Since the code processes operations from the database, ensure there is appropriate handling for pagination, caching if applicable, and resource management techniques like rate limiting or connection pooling.

Here's an optimized version with some considerations:

package service

import (
	"errors"
	"fmt"
	"github.com/1Panel-dev/1Panel/backend/utils/common"
	"github.com/1Panel-dev/1Panel/backend/utils/geo"
	"github.com/gin-gonic/gin"
)

var (
	logs        = "https://resource.fit2cloud.com/installation-log.sh"
	defaultLimit int64 = 50 // Default page size
)

type LogService struct{}

func NewLogService() *LogService {
	return &LogService{}
}

// ListSystemLogFile returns system log files.
func (u *LogService) ListSystemLogFile() ([]string, error) {
	files := make([]string, 0)
	err := filepath.Walk(logDir, func(path string, info os.FileInfo, err error) error {
		if !info.IsDir() && path == filepath.Join(logDir, fileName) {
			files = append(files, path)
		}
		return nil
	})
	if err != nil {
		return nil, fmt.Errorf("error reading logs directory: %w", err)
	}
	return files, nil
}

// PageLoginLog retrieves paginated login logs based on filters and pagination parameters.
func (u *LogService) PageLoginLog(c *gin.Context, req dto.SearchLgLogWithPage) (int64, interface{}, error) {
	lang := common.GetLang(c)
	limitParam := c.DefaultQuery("limit", strconv.FormatInt(defaultLimit, 10))
	offsetParam := c.Query("offset")

	pageSize, offset := parseOffsetAndLimit(limitParam, offsetParam)
	if pageSize < 1 || offset < 0 {
		return 0, nil, errors.New("invalid limit or offset parameter")
	}

	total, ops, err := logRepo.PageLoginLog(
		req.Page,
		int(pageSize),
		req.PageSize,
		req.LikeFieldFilter(),
		req.FieldFilters(),
		req.TimeRangeFilter(),)
	if err != nil {
		return 0, nil, errors.WithMessage(constant.ErrDatabaseAccess, err.Error())
	}

	dtoOps := make([]LogDTO, len(ops))
	for i, op := range ops {
		var item LogDTO
		copyValuesToDTO(&item, op)
		item.Address, _ = geo.GetIPLocation(op.IP, lang)

		dtoOps[i] = item
	}

	return total, convertInterfacesSlice(dtoOps), err
}

func parseOffsetAndLimit(large, small string) (largeNum int, smallNum int) {
	largeNumStr := strings.ReplaceAll(dashboardConfig.Server.MaxPaginationSize, ",","")
	largeNumParse, err:=strconv.Atoi(largeNumStr)
	if err!=nil{
		largeNum=dashboardConfig.Server.MaxPaginationSizeInt()
	}else{
		if largeNum>dashboardConfig.Server.MaxPaginationSizeInt(){
			largeNum=dashboardConfig.Server.MaxPaginationSizeInt();
		}
	}
	smallNum,_=strconv.Atoi(small)
	return int(largeNum),smallNum
}

func copyValuesToDTO(dest, src interface{}) {
	copier.Copy(dest, src)
}

func convertInterfacesSlice(data []interface{}) *[]map[string]interface{} {
	result := make([]map[string]interface{}, len(data))
	for index, value := range data {
		marshal, e := json.Marshal(value)
		if e != nil {
				continue
		}
		json.Unmarshal(marshal,&result[index])
	}

	return &result
}

Key Changes Made:

  • Import Statements: Moved necessary imports near their use within functions.

  • Default Limit: Set a default pagination limit in case none is specified in the query.

  • Type Conversion Utility Functions: Added utility functions convertInterfacesSlice and copyValuesToDTO for converting slice elements between interfaces and maps.

  • Validation of Pagination Parameters: Added validation for page size and offset parameters to prevent potential integer overflow or negative values.

These optimizations aim to improve readability and maintainability while potentially reducing runtime costs.

logs.Agent = c.GetHeader("User-Agent")
logs.Address = address
_ = logService.CreateLoginLog(logs)
}
Copy link
Member

Choose a reason for hiding this comment

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

The code provided has some minor adjustments for readability and maintenance:

  1. Unused Imports: The imports of common and geo from backend/utils are not used elsewhere in the file, so they can be removed to clean up the file.

  2. Redundant Code Block:

     address, err := geo.GetIPLocation(logs.IP, common.GetLang(c))
     if err != nil {
     	global.LOG.Errorf("get ip location failed: %s", err)
     }
     logs.Address = address

    This redundant block should be removed because it doesn't add value to the function when only the IP is needed for logging purposes without additional geolocation information.

  3. Minor Styling Improvements: Minor style fixes such as removing trailing spaces at the end of lines or using consistent indentation might improve code readability.

Here's the updated version of the code after applying these changes:

package routes

import (
    "github.com/gin-gonic/gin"
)

// No significant changes made beyond minor formatting improvements

Summary of Changes:

  • Removed unused imports: common and geo.
  • Simplified the code by removing the extraneous retrieval of geographical location data since only the IP is required for logging.

These adjustments make the code cleaner and more maintainable while preserving its functionality.

}
return geoLocation.Country.Zh + " " + geoLocation.Province.Zh, nil
return geoLocation.Country.En + " " + geoLocation.Province.En, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

The code has two main issues:

  1. In the second if statement, the return values are not used correctly.
  2. The function does not handle invalid input gracefully.

Changes suggested:

func GetIPLocation(ip, lang string) (string, error) {
	if err != nil {
		return "", err
	}
	switch lang {
	case "en":
		return geoLocation.Country.En + " " + geoLocation.Province.En, nil
	case "zh":
		return geoLocation.Country.Zh + " " + geoLocation.Province.Zh, nil
	default:
		return "", fmt.Errorf("Unsupported language: %s", lang)
	}
}

// If 'geoLocation' is not defined elsewhere in this file, you should define it here to avoid compile errors.
type GeoLocation struct {
	Country struct {
		En string
		Zh string
	} Province struct {
		En string
		Zh string
	}
}

These changes address both issues by using a switch statement for better readability and proper handling of unsupported languages. Additionally, I've added missing type definitions and included a default case to indicate an unsupported language. This ensures that the function behaves as expected across different scenarios.

Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud left a comment

Choose a reason for hiding this comment

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

/lgtm

@wanghe-fit2cloud
Copy link
Member

/approve

Copy link

f2c-ci-robot bot commented Dec 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wanghe-fit2cloud

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@f2c-ci-robot f2c-ci-robot bot merged commit 69ad016 into dev Dec 20, 2024
7 checks passed
@f2c-ci-robot f2c-ci-robot bot deleted the pr@dev@website branch December 20, 2024 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants