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

add case for restore system table(1.2-dev) #16481

Merged
merged 5 commits into from
Jun 2, 2024

Conversation

Ariznawlll
Copy link
Contributor

@Ariznawlll Ariznawlll commented May 29, 2024

User description

What type of PR is this?

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

Which issue(s) this PR fixes:

issue ##11492

What this PR does / why we need it:

add case for restore system table(mo_role,mo_user,mo_user_grant,mo_role_grant,mo_role_privs,mo_user_defined_function , mo_stored_procedure, mo_stages)


PR Type

Tests


Description

  • Added test cases for restoring system tables to sys account, including creating, dropping, and restoring user-defined functions (UDFs), stored procedures, stages, users, and roles.
  • Added test cases for restoring system tables to non-sys account, including creating, dropping, and restoring user-defined functions (UDFs), stored procedures, stages, users, and roles.

Changes walkthrough 📝

Relevant files
Tests
sys_restore_system_table_to_sys_account.result
Add test cases for restoring system tables to sys account

test/distributed/cases/snapshot/sys_restore_system_table_to_sys_account.result

  • Added SQL commands to create, drop, and restore user-defined functions
    (UDFs).
  • Added SQL commands to create, drop, and restore stored procedures.
  • Added SQL commands to create, drop, and restore stages.
  • Added SQL commands to create, drop, and restore users and roles.
  • +820/-0 
    sys_restore_system_table_to_nonsys_account.sql
    Add test cases for restoring system tables to non-sys account

    test/distributed/cases/snapshot/sys_restore_system_table_to_nonsys_account.sql

  • Added SQL commands to create, drop, and restore user-defined functions
    (UDFs).
  • Added SQL commands to create, drop, and restore stored procedures.
  • Added SQL commands to create, drop, and restore stages.
  • Added SQL commands to create, drop, and restore users and roles.
  • +655/-0 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @matrix-meow matrix-meow added the size/XXL Denotes a PR that changes 2000+ lines label May 29, 2024
    @mergify mergify bot requested a review from sukki37 May 29, 2024 11:09
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    5, because the PR is extensive with a large number of changes across multiple files, involving complex operations such as creating, dropping, and restoring various database objects like functions, procedures, snapshots, users, roles, and stages. The PR also includes detailed test cases which need thorough validation to ensure they cover all scenarios and their interactions with the system.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The PR includes operations on snapshots and restoration of database states which are critical and prone to errors if not handled correctly. Each step in the snapshot and restore process must be carefully reviewed to ensure it correctly captures and restores the intended state without data loss or corruption.

    Performance Concern: The repeated creation and dropping of database objects within test cases could lead to performance overhead and increased execution time for the tests. It's important to ensure that these operations are optimized and do not negatively impact the overall system performance.

    🔒 Security concerns

    - Sensitive Information Exposure: The PR includes hardcoded credentials in the stage creation (AWS_KEY_ID and AWS_SECRET_KEY). This should be replaced with environment variables or another secure method to avoid exposure of sensitive information in the codebase.

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the grant statement to ensure the correct user is granted the roles

    Correct the grant statement to ensure the correct user is granted the roles by changing
    role_u2 to role_u3 in the grant role_r1,role_r2,role_r3 to role_u1,role_u2,role_u2;
    statement.

    test/distributed/cases/snapshot/nonsys_restore_system_table_to_nonsys_account.sql [532]

    -grant role_r1,role_r2,role_r3 to role_u1,role_u2,role_u2;
    +grant role_r1,role_r2,role_r3 to role_u1,role_u2,role_u3;
     
    Suggestion importance[1-10]: 10

    Why: This suggestion fixes a potential bug where the wrong user could be granted access due to a typo in the grant statement, which is crucial for maintaining correct access controls and security.

    10
    Maintainability
    Remove duplicate drop table statement to avoid redundancy

    Ensure that the drop table if exists tbh2; statement is not duplicated to avoid
    unnecessary redundancy and potential confusion.

    test/distributed/cases/snapshot/nonsys_restore_system_table_to_nonsys_account.sql [105-106]

    -drop table if exists tbh2;
     drop table if exists tbh2;
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies and resolves a redundancy issue by removing a duplicated SQL statement, which improves code clarity and maintainability.

    8
    Remove redundant drop table statement to avoid unnecessary operations

    The drop table if exists tbh2; statement is repeated twice in lines 103 and 150. This
    redundancy should be removed to avoid unnecessary operations.

    test/distributed/cases/snapshot/sys_restore_system_table_to_sys_account.sql [103]

    -drop table if exists tbh2;
     drop table if exists tbh2;
     
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies and proposes the removal of a redundant SQL statement, which improves maintainability by reducing unnecessary operations.

    7
    Best practice
    Add a space between function and the function name to follow SQL syntax conventions

    Add a space between function and the function name concatenate to follow SQL syntax
    conventions and improve readability.

    test/distributed/cases/snapshot/nonsys_restore_system_table_to_nonsys_account.sql [24]

    -create function`concatenate`(str1 varchar(255), str2 varchar(255)) returns varchar(255)
    +create function `concatenate`(str1 varchar(255), str2 varchar(255)) returns varchar(255)
     
    Suggestion importance[1-10]: 7

    Why: Adding a space enhances readability and adheres to SQL syntax conventions, making the code more standard and easier to understand.

    7
    Split drop role statement into individual statements for better readability and error handling

    The drop role if exists statement in line 408 should be split into individual drop role
    statements for better readability and error handling.

    test/distributed/cases/snapshot/sys_restore_system_table_to_sys_account.sql [408]

    -drop role if exists r1,r2,r3,r4,r5,r6,r7,r8,r9,r10;
    +drop role if exists r1;
    +drop role if exists r2;
    +drop role if exists r3;
    +drop role if exists r4;
    +drop role if exists r5;
    +drop role if exists r6;
    +drop role if exists r7;
    +drop role if exists r8;
    +drop role if exists r9;
    +drop role if exists r10;
     
    Suggestion importance[1-10]: 5

    Why: While splitting the drop role statement could enhance readability, the original batch operation is not incorrect and is often used for conciseness. The suggestion is a matter of style preference rather than a necessity.

    5

    @mergify mergify bot merged commit d36f889 into matrixorigin:1.2-dev Jun 2, 2024
    18 checks passed
    @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
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants