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

change keep_user_target_list_in_result default to 1 #16234

Merged
merged 5 commits into from
May 22, 2024

Conversation

YANGGMM
Copy link
Contributor

@YANGGMM YANGGMM commented May 20, 2024

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #15831

What this PR does / why we need it:

change keep_user_target_list_in_result default to 1

@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label May 20, 2024
@mergify mergify bot requested a review from sukki37 May 20, 2024 03:18
@matrix-meow
Copy link
Contributor

@YANGGMM Thanks for your contributions!

Pull Request Review:

Title: change keep_user_target_list_in_result default to 1

Problem 1: Lack of Detailed Explanation

The title and body of the pull request lack a detailed explanation of why the change is necessary. It would be beneficial to provide more context on why the default value is being changed to 1 for keep_user_target_list_in_result. This information can help other team members understand the rationale behind the modification.

Problem 2: Security Concern - SQL Injection Vulnerability

In the tenant_upgrade_list.go file, the addition of upg_mo_mysql_compatibility_mode1 introduces a potential security risk due to the direct concatenation of SQL query in the UpgSql field. This can lead to SQL injection vulnerabilities. It is recommended to use parameterized queries or ORM methods to prevent SQL injection attacks.

Problem 3: Inconsistent Naming Convention

In the variables.go file, the variable keep_user_target_list_in_result is modified to have a default value of 1. However, the naming convention for this variable is not consistent with the existing variables in the file. It would be beneficial to ensure consistency in naming conventions across the codebase for better readability and maintainability.

Problem 4: Unnecessary Changes in Test Files

In the test files select_origin.result and select_origin.sql, there are changes made to set keep_user_target_list_in_result to 0. These changes seem unnecessary for the purpose of the pull request, which focuses on changing the default value to 1. It is recommended to avoid making unrelated changes in test files to maintain clarity and focus.

Optimization Suggestion:

  • Provide a more detailed description in the pull request body to explain the rationale behind changing the default value.
  • Refactor the SQL query in upg_mo_mysql_compatibility_mode1 to use parameterized queries to prevent SQL injection vulnerabilities.
  • Ensure consistent naming conventions for variables in the codebase to improve readability.
  • Avoid making unrelated changes in test files to keep the focus on the main purpose of the pull request.

By addressing the security concern, maintaining consistent naming conventions, and avoiding unnecessary changes, the pull request can be optimized for better code quality and security.

Copy link
Contributor

@qingxinhome qingxinhome left a comment

Choose a reason for hiding this comment

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

需要往1.2-dev做cherry-pick

@YANGGMM YANGGMM marked this pull request as draft May 20, 2024 04:00
@YANGGMM YANGGMM marked this pull request as ready for review May 22, 2024 08:13
@mergify mergify bot merged commit 5e5aa56 into matrixorigin:1.2-dev May 22, 2024
18 of 19 checks passed
@YANGGMM YANGGMM deleted the fix-sys-var branch June 3, 2024 07:45
@aylei aylei mentioned this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants