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

fix alter table add enum col (#1.1-dev) #15237

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

YANGGMM
Copy link
Contributor

@YANGGMM YANGGMM commented Mar 29, 2024

fix alter table add enum col

Approved by: @ouyuanning, @heni02

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #15176 #15177

What this PR does / why we need it:

fix alter table add enum col

fix alter table add enum col

Approved by: @ouyuanning, @heni02
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Mar 29, 2024
@mergify mergify bot requested a review from sukki37 March 29, 2024 07:01
@mergify mergify bot added the kind/bug Something isn't working label Mar 29, 2024
@matrix-meow
Copy link
Contributor

@YANGGMM Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that it fixes an issue related to altering a table to add an enum column.

Body:

The body of the pull request provides a brief description of the changes made and lists the approvals from specific individuals. It also mentions the type of PR (BUG) and the issues it fixes (#15176 and #15177). The body could be improved by providing more context on why the changes were necessary and what specific problem they address.

Changes:

  1. In build_alter_add_column.go:

    • The addition of a check to prevent adding an ENUM column to a primary key. This is a good addition to prevent potential issues with ENUM columns in primary keys.
  2. In build_alter_table.go:

    • Modification to handle ENUM columns when restoring DDL. This change correctly formats ENUM values when altering a table, which is a necessary adjustment.
  3. In test files:

    • Various test results have been updated to reflect the changes made in the code related to ENUM columns. This ensures that the tests align with the code changes.

Suggestions for Improvement:

  1. Security Issue - SQL Injection Vulnerability:

    • The changes made in the pull request do not directly address SQL injection vulnerabilities. It is crucial to sanitize and validate user input, especially when dealing with SQL queries. Consider implementing parameterized queries or input validation to prevent SQL injection attacks.
  2. Enhancement - Detailed Explanation:

    • Provide more detailed explanations in the body of the pull request to help reviewers and future developers understand the purpose and impact of the changes. This can improve the overall transparency and maintainability of the codebase.
  3. Optimization - Code Refactoring:

    • Consider refactoring the code to improve readability and maintainability. For example, extracting common logic into separate functions or modules can enhance code organization and reduce duplication.
  4. Testing - Comprehensive Test Coverage:

    • Ensure that the test cases cover all possible scenarios, including edge cases and error conditions related to adding ENUM columns and altering tables. Comprehensive testing can help prevent regressions and ensure the reliability of the code changes.
  5. Documentation - Update Documentation:

    • Update relevant documentation to reflect the changes made in the codebase, especially regarding the handling of ENUM columns in ALTER TABLE operations. Clear and up-to-date documentation can aid developers in understanding and using the code effectively.

By addressing the security issue, providing more detailed explanations, optimizing the code, enhancing test coverage, and updating documentation, the pull request can be further improved in terms of security, clarity, maintainability, and reliability.

@mergify mergify bot merged commit 2776017 into matrixorigin:1.1-dev Mar 29, 2024
17 of 18 checks passed
@YANGGMM YANGGMM deleted the fix-alter-enum-1.1-dev branch May 13, 2024 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants