feat: add get_table_foreign_key_info method for MySQL and Oracle database#351
Conversation
WalkthroughTwo database datasource classes receive new methods to retrieve foreign key relationship information. The MysqlDataSource and OracleDataSource each gain a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
dcs_core/integrations/databases/oracle.py (1)
706-706: Add docstring for public method.This public method should include a docstring describing its purpose, parameters, return value, and usage.
def get_table_foreign_key_info(self, table_name: str, schema: str | None = None): + """ + Get foreign key constraint information for a table. + + :param table_name: Name of the table + :param schema: Optional schema name (defaults to instance schema_name) + :return: List of dictionaries containing foreign key details with keys: + constraint_name, table_name, fk_column, referenced_table, referenced_column + """ schema = schema or self.schema_namedcs_core/integrations/databases/mysql.py (1)
402-402: Add docstring for public method.This public method should include a docstring describing its purpose, parameters, return value, and usage for consistency with other documented methods in the codebase.
def get_table_foreign_key_info(self, table_name: str, schema: str | None = None): + """ + Get foreign key constraint information for a table. + + :param table_name: Name of the table + :param schema: Optional schema name (defaults to instance schema_name) + :return: List of dictionaries containing foreign key details with keys: + constraint_name, table_name, fk_column, referenced_table, referenced_column + """ schema = schema or self.schema_name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dcs_core/integrations/databases/mysql.py(2 hunks)dcs_core/integrations/databases/oracle.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
dcs_core/integrations/databases/oracle.py (4)
dcs_core/integrations/databases/mysql.py (1)
get_table_foreign_key_info(402-437)dcs_core/integrations/databases/postgres.py (1)
get_table_foreign_key_info(420-447)dcs_core/integrations/databases/mssql.py (2)
get_table_foreign_key_info(824-872)fetchall(107-108)dcs_core/integrations/databases/sybase.py (1)
fetchall(194-195)
dcs_core/integrations/databases/mysql.py (5)
dcs_core/integrations/databases/oracle.py (1)
get_table_foreign_key_info(706-748)dcs_core/integrations/databases/postgres.py (1)
get_table_foreign_key_info(420-447)dcs_core/integrations/databases/mssql.py (2)
get_table_foreign_key_info(824-872)fetchall(107-108)dcs_core/core/datasource/sql_datasource.py (1)
fetchall(120-123)dcs_core/integrations/databases/sybase.py (1)
fetchall(194-195)
🪛 Ruff (0.14.6)
dcs_core/integrations/databases/oracle.py
709-730: Possible SQL injection vector through string-based query construction
(S608)
734-734: Do not catch blind exception: Exception
(BLE001)
dcs_core/integrations/databases/mysql.py
405-419: Possible SQL injection vector through string-based query construction
(S608)
423-423: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
dcs_core/integrations/databases/mysql.py (1)
18-18: LGTM!The logger import is appropriate for the new error handling in
get_table_foreign_key_info.
| def get_table_foreign_key_info(self, table_name: str, schema: str | None = None): | ||
| schema = schema or self.schema_name | ||
|
|
||
| query = f""" | ||
| SELECT | ||
| kcu.CONSTRAINT_NAME AS constraint_name, | ||
| kcu.TABLE_NAME AS table_name, | ||
| kcu.COLUMN_NAME AS fk_column, | ||
| kcu.REFERENCED_TABLE_NAME AS referenced_table, | ||
| kcu.REFERENCED_COLUMN_NAME AS referenced_column | ||
| FROM information_schema.TABLE_CONSTRAINTS tc | ||
| JOIN information_schema.KEY_COLUMN_USAGE kcu | ||
| ON tc.CONSTRAINT_NAME = kcu.CONSTRAINT_NAME | ||
| AND tc.TABLE_SCHEMA = kcu.TABLE_SCHEMA | ||
| WHERE tc.CONSTRAINT_TYPE = 'FOREIGN KEY' | ||
| AND tc.TABLE_NAME = '{table_name}' | ||
| AND tc.TABLE_SCHEMA = '{schema}'; | ||
| """ | ||
|
|
||
| try: | ||
| rows = self.fetchall(query) | ||
| except Exception as e: | ||
| logger.error(f"Failed to fetch fk info for dataset: {table_name} ({e})") | ||
| return [] | ||
|
|
||
| data = [ | ||
| { | ||
| "constraint_name": row[0], | ||
| "table_name": row[1], | ||
| "fk_column": row[2], | ||
| "referenced_table": row[3], | ||
| "referenced_column": row[4], | ||
| } | ||
| for row in rows | ||
| ] | ||
| return data |
There was a problem hiding this comment.
SQL injection vulnerability: use parameterized queries.
The table_name and schema parameters are directly interpolated into the SQL query using f-strings, creating a SQL injection vulnerability. A malicious input like table_name="users' OR '1'='1" could manipulate the query logic or extract unauthorized data.
Apply this diff to use parameterized queries with SQLAlchemy:
def get_table_foreign_key_info(self, table_name: str, schema: str | None = None):
schema = schema or self.schema_name
query = f"""
SELECT
kcu.CONSTRAINT_NAME AS constraint_name,
kcu.TABLE_NAME AS table_name,
kcu.COLUMN_NAME AS fk_column,
kcu.REFERENCED_TABLE_NAME AS referenced_table,
kcu.REFERENCED_COLUMN_NAME AS referenced_column
FROM information_schema.TABLE_CONSTRAINTS tc
JOIN information_schema.KEY_COLUMN_USAGE kcu
ON tc.CONSTRAINT_NAME = kcu.CONSTRAINT_NAME
AND tc.TABLE_SCHEMA = kcu.TABLE_SCHEMA
WHERE tc.CONSTRAINT_TYPE = 'FOREIGN KEY'
- AND tc.TABLE_NAME = '{table_name}'
- AND tc.TABLE_SCHEMA = '{schema}';
+ AND tc.TABLE_NAME = :table_name
+ AND tc.TABLE_SCHEMA = :schema
"""
try:
- rows = self.fetchall(query)
+ result = self.connection.execute(
+ text(query),
+ {"table_name": table_name, "schema": schema}
+ )
+ rows = result.fetchall()
except Exception as e:
logger.error(f"Failed to fetch fk info for dataset: {table_name} ({e})")
return []
data = [
{
"constraint_name": row[0],
"table_name": row[1],
"fk_column": row[2],
"referenced_table": row[3],
"referenced_column": row[4],
}
for row in rows
]
return dataNote: This SQL injection pattern appears throughout the codebase in similar query methods. Consider addressing this systematically across all database datasource classes.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.6)
405-419: Possible SQL injection vector through string-based query construction
(S608)
423-423: Do not catch blind exception: Exception
(BLE001)
| def get_table_foreign_key_info(self, table_name: str, schema: str | None = None): | ||
| schema = schema or self.schema_name | ||
|
|
||
| query = f""" | ||
| SELECT | ||
| ac.CONSTRAINT_NAME AS constraint_name, | ||
| ac.TABLE_NAME AS table_name, | ||
| acc.COLUMN_NAME AS fk_column, | ||
| r_ac.TABLE_NAME AS referenced_table, | ||
| r_acc.COLUMN_NAME AS referenced_column | ||
| FROM ALL_CONSTRAINTS ac | ||
| JOIN ALL_CONS_COLUMNS acc | ||
| ON ac.CONSTRAINT_NAME = acc.CONSTRAINT_NAME | ||
| AND ac.OWNER = acc.OWNER | ||
| JOIN ALL_CONSTRAINTS r_ac | ||
| ON ac.R_CONSTRAINT_NAME = r_ac.CONSTRAINT_NAME | ||
| AND ac.R_OWNER = r_ac.OWNER | ||
| JOIN ALL_CONS_COLUMNS r_acc | ||
| ON r_ac.CONSTRAINT_NAME = r_acc.CONSTRAINT_NAME | ||
| AND r_ac.OWNER = r_acc.OWNER | ||
| AND acc.POSITION = r_acc.POSITION | ||
| WHERE ac.CONSTRAINT_TYPE = 'R' | ||
| AND ac.TABLE_NAME = '{table_name.upper()}' | ||
| AND ac.OWNER = '{schema.upper()}'; | ||
| """ | ||
|
|
||
| try: | ||
| rows = self.fetchall(query) | ||
| except Exception as e: | ||
| logger.error(f"Failed to fetch fk info for dataset: {table_name} ({e})") | ||
| return [] | ||
|
|
||
| data = [ | ||
| { | ||
| "constraint_name": row[0], | ||
| "table_name": row[1], | ||
| "fk_column": row[2], | ||
| "referenced_table": row[3], | ||
| "referenced_column": row[4], | ||
| } | ||
| for row in rows | ||
| ] | ||
| return data |
There was a problem hiding this comment.
SQL injection vulnerability: use parameterized queries.
The table_name and schema parameters are directly interpolated into the SQL query using f-strings. Even though .upper() is called, this does not prevent SQL injection attacks. A malicious input like table_name="users' OR '1'='1" could manipulate the query logic.
Apply this diff to use parameterized queries with SQLAlchemy:
def get_table_foreign_key_info(self, table_name: str, schema: str | None = None):
schema = schema or self.schema_name
query = f"""
SELECT
ac.CONSTRAINT_NAME AS constraint_name,
ac.TABLE_NAME AS table_name,
acc.COLUMN_NAME AS fk_column,
r_ac.TABLE_NAME AS referenced_table,
r_acc.COLUMN_NAME AS referenced_column
FROM ALL_CONSTRAINTS ac
JOIN ALL_CONS_COLUMNS acc
ON ac.CONSTRAINT_NAME = acc.CONSTRAINT_NAME
AND ac.OWNER = acc.OWNER
JOIN ALL_CONSTRAINTS r_ac
ON ac.R_CONSTRAINT_NAME = r_ac.CONSTRAINT_NAME
AND ac.R_OWNER = r_ac.OWNER
JOIN ALL_CONS_COLUMNS r_acc
ON r_ac.CONSTRAINT_NAME = r_acc.CONSTRAINT_NAME
AND r_ac.OWNER = r_acc.OWNER
AND acc.POSITION = r_acc.POSITION
WHERE ac.CONSTRAINT_TYPE = 'R'
- AND ac.TABLE_NAME = '{table_name.upper()}'
- AND ac.OWNER = '{schema.upper()}';
+ AND ac.TABLE_NAME = :table_name
+ AND ac.OWNER = :schema
"""
try:
- rows = self.fetchall(query)
+ from sqlalchemy import text
+ result = self.connection.execute(
+ text(query),
+ {"table_name": table_name.upper(), "schema": schema.upper()}
+ )
+ rows = result.fetchall()
except Exception as e:
logger.error(f"Failed to fetch fk info for dataset: {table_name} ({e})")
return []
data = [
{
"constraint_name": row[0],
"table_name": row[1],
"fk_column": row[2],
"referenced_table": row[3],
"referenced_column": row[4],
}
for row in rows
]
return dataNote: Similar SQL injection vulnerabilities exist in other methods throughout the codebase (e.g., query_get_table_indexes, query_get_table_columns). Consider a broader refactoring effort to use parameterized queries consistently.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.6)
709-730: Possible SQL injection vector through string-based query construction
(S608)
734-734: Do not catch blind exception: Exception
(BLE001)
2 participants
… sources
Fixes/Implements
Description
Summary Goes here.
Type of change
Delete irrelevant options.
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.