fix!: forbid to change information_schema #4233
Conversation
CookiePieWw
added
the
breaking-change
WalkthroughThe recent changes introduce checks to ensure that operations on read-only schemas and tables, specifically within the "information_schema," result in user-friendly error messages rather than internal errors. These checks have been added across several modules, enhancing user experience by preventing unwanted modifications and providing descriptive error messages. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Application
participant QueryEngine
participant Catalog
participant SchemaRegistry
User->>Application: Perform operation on information_schema
Application->>QueryEngine: Forward request
QueryEngine->>Catalog: Check if schema/table is read-only
Catalog-->>QueryEngine: Schema/table is read-only
QueryEngine-->>Application: Return SchemaReadOnlySnafu / TableReadOnlySnafu error
Application-->>User: Display user-friendly error message
Assessment against linked issues
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- src/operator/src/error.rs (2 hunks)
- src/operator/src/req_convert/insert/stmt_to_region.rs (2 hunks)
- src/operator/src/statement/ddl.rs (6 hunks)
- src/query/src/datafusion.rs (3 hunks)
- src/query/src/error.rs (2 hunks)
- tests/cases/standalone/common/information_schema/region_peers.result (3 hunks)
- tests/cases/standalone/common/information_schema/region_peers.sql (2 hunks)
- tests/cases/standalone/common/information_schema/table_constraints.result (1 hunks)
- tests/cases/standalone/common/information_schema/table_constraints.sql (1 hunks)
- tests/cases/standalone/common/information_schema/tables.result (1 hunks)
- tests/cases/standalone/common/information_schema/tables.sql (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/cases/standalone/common/information_schema/region_peers.sql
Additional comments not posted (22)
tests/cases/standalone/common/information_schema/tables.sql (1)
20-21: LGTM!The addition of
drop schema information_schema;correctly tests the new restriction on dropping theinformation_schemaschema.tests/cases/standalone/common/information_schema/table_constraints.sql (1)
9-10: LGTM!The changes to switch between
INFORMATION_SCHEMAandpublicschemas correctly test the new restrictions on operations within theinformation_schemaschema.Also applies to: 13-14, 19-21
tests/cases/standalone/common/information_schema/tables.result (1)
47-50: LGTM!The addition of the expected error message for attempting to drop the
information_schemaschema correctly verifies the new restriction.tests/cases/standalone/common/information_schema/region_peers.result (1)
2-2: LGTM!The changes to switch between
publicandINFORMATION_SCHEMAschemas correctly test the new restrictions on operations within theinformation_schemaschema.Also applies to: 38-39, 50-54
tests/cases/standalone/common/information_schema/table_constraints.result (4)
28-29: Ensure correct schema usage.Switching to the
publicschema after queryingINFORMATION_SCHEMAis consistent with the new restrictions. Verify if this change aligns with the expected behavior.
36-37: Ensure correct schema usage.Switching back to
INFORMATION_SCHEMAfor further queries is consistent with the new restrictions. Verify if this change aligns with the expected behavior.
52-57: Validate constraint query results.Ensure that the constraints for the
testtable are correctly displayed after switching schemas. This aligns with the new restrictions.
63-63: Ensure proper cleanup.Dropping the
testtable in thepublicschema is consistent with the overall schema management. Verify if this aligns with the expected behavior.src/operator/src/req_convert/insert/stmt_to_region.rs (2)
32-32: New error variant added.The
SchemaReadOnlySnafuerror variant is added to handle read-only schema errors. This aligns with the PR objectives.
68-71: Restrict operations oninformation_schema.The added check ensures that operations on the
information_schemaschema are restricted. This aligns with the PR objectives.src/query/src/error.rs (2)
310-316: New error variant added.The
TableReadOnlyerror variant is added to handle read-only table errors. This aligns with the PR objectives.
330-330: Map new error variant to status code.The
TableReadOnlyerror variant is mapped to theInvalidArgumentsstatus code. This aligns with the PR objectives.src/query/src/datafusion.rs (3)
53-53: New error variant added.The
TableReadOnlySnafuerror variant is added to handle read-only table errors. This aligns with the PR objectives.
177-180: Restrict delete operations oninformation_schema.The added check ensures that delete operations on the
information_schemaschema are restricted. This aligns with the PR objectives.
225-228: Restrict insert operations oninformation_schema.The added check ensures that insert operations on the
information_schemaschema are restricted. This aligns with the PR objectives.src/operator/src/error.rs (2)
272-277: New error variant added:SchemaReadOnly.This new error variant will help in identifying and handling operations on read-only schemas like
information_schema.
728-728: New error variantSchemaReadOnlymapped toInvalidArgumentsstatus code.This mapping ensures that operations on read-only schemas are correctly classified as invalid arguments.
src/operator/src/statement/ddl.rs (5)
75-76: New error handling added:SchemaReadOnlySnafu.This error handling will ensure that operations on read-only schemas like
information_schemaare correctly identified and handled.
154-159: Check added to prevent table creation ininformation_schemaschema.The
ensure!macro is used to validate the schema name, raising aSchemaReadOnlySnafuerror if the schema isinformation_schema. This enforces the read-only status of the schema.
655-660: Check added to prevent dropping tables ininformation_schemaschema.The
ensure!macro is used to validate the schema name, raising aSchemaReadOnlySnafuerror if the schema isinformation_schema. This ensures that tables in the read-only schema cannot be dropped.
711-715: Check added to prevent dropping theinformation_schemaschema.The
ensure!macro is used to validate the schema name, raising aSchemaReadOnlySnafuerror if the schema isinformation_schema. This ensures that the read-only schema cannot be dropped.
747-753: Check added to prevent truncating tables ininformation_schemaschema.The
ensure!macro is used to validate the schema name, raising aSchemaReadOnlySnafuerror if the schema isinformation_schema. This ensures that tables in the read-only schema cannot be truncated.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4233 +/- ##
==========================================
- Coverage 84.86% 84.70% -0.17%
==========================================
Files 1045 1054 +9
Lines 184265 187005 +2740
==========================================
+ Hits 156384 158396 +2012
- Misses 27881 28609 +728 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- src/catalog/src/consts.rs (1 hunks)
- src/catalog/src/lib.rs (1 hunks)
- src/operator/src/error.rs (2 hunks)
- src/operator/src/req_convert/insert/stmt_to_region.rs (2 hunks)
- src/operator/src/statement/ddl.rs (7 hunks)
- src/query/src/datafusion.rs (4 hunks)
- src/query/src/error.rs (2 hunks)
- tests/cases/standalone/common/information_schema/tables.result (1 hunks)
- tests/cases/standalone/common/information_schema/tables.sql (1 hunks)
Files not summarized due to errors (2)
- src/query/src/datafusion.rs: Error: Server error. Please try again later.
- src/query/src/error.rs: Error: Server error. Please try again later.
Files skipped from review due to trivial changes (2)
- src/catalog/src/lib.rs
- src/operator/src/error.rs
Files skipped from review as they are similar to previous changes (4)
- src/operator/src/req_convert/insert/stmt_to_region.rs
- src/operator/src/statement/ddl.rs
- tests/cases/standalone/common/information_schema/tables.result
- tests/cases/standalone/common/information_schema/tables.sql
Additional comments not posted (6)
src/catalog/src/consts.rs (1)
17-19: LGTM! Consider future extensibility.The function correctly checks if the schema is
INFORMATION_SCHEMA_NAME. If more read-only schemas are added in the future, consider extending this function to handle them.src/query/src/error.rs (2)
311-316: LGTM! New error variantTableReadOnlyis well-defined.The new error variant
TableReadOnlyincludes the table name and location, which aligns with the objective of providing more informative error messages.
366-366: LGTM! Appropriate status code mapping forTableReadOnly.The status code mapping for
TableReadOnlytoStatusCode::Unsupportedis appropriate.src/query/src/datafusion.rs (3)
25-25: LGTM! Necessary import statement foris_readonly_schema.The import statement for
is_readonly_schemais correctly placed and necessary for the new functionality.
177-181: LGTM! Correct check for read-only schema indeletemethod.The
deletemethod correctly uses theis_readonly_schemafunction to ensure the schema is not read-only and raises an appropriate error if it is.
222-230: LGTM! Correct check for read-only schema ininsertmethod.The
insertmethod correctly uses theis_readonly_schemafunction to ensure the schema is not read-only and raises an appropriate error if it is.
|
Updated with a read-only check function, error code and some sqlness tests. PTAL @killme2008 I forgot to sync the clock before commit, so it may be a little strange that f336170 is ahead of the comments QaQ. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/common/catalog/src/consts.rs (1 hunks)
- src/operator/src/req_convert/insert/stmt_to_region.rs (2 hunks)
- src/operator/src/statement/ddl.rs (7 hunks)
- src/query/src/datafusion.rs (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/operator/src/req_convert/insert/stmt_to_region.rs
- src/operator/src/statement/ddl.rs
- src/query/src/datafusion.rs
Additional comments not posted (1)
src/common/catalog/src/consts.rs (1)
112-114: LGTM!The function
is_readonly_schemacorrectly checks if a schema name matchesINFORMATION_SCHEMA_NAMEusing thematches!macro.
There was a problem hiding this comment.
This looks good for now. But eventually we need to implement this based on our permission system @MichaelScofield
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
close #3653
What's changed and what's your intention?
As title. Following ops in
information_schemaare forbidden in this patch:Updated some sqlness tests for it.
Checklist
Summary by CodeRabbit
New Features
information_schema.Bug Fixes
Refactor