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(io-manager): create primary key if missing #182

Merged
merged 1 commit into from
Jan 27, 2025
Merged

Conversation

hbruch
Copy link
Collaborator

@hbruch hbruch commented Jan 26, 2025

This PR creates missing primary keys for tables which were created before primary keys were created in conjuction with newly created tables.

FROM pg_index i
JOIN pg_class c ON c.oid = i.indrelid
WHERE c.relnamespace::regnamespace::varchar = '{schema}'
AND c.relname = '{table}' AND i.indisprimary ='t'"""
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT this creates the potential for an SQL injection vulnerability! ⚠️ – We don't know if and how _has_primary_key() will ever be used in a different context or without knowing if its arguments are user-defined.

Instead, psycopg's docs recommend passing the literals as additional arguments into execute().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right. While this should be avoided by the explicit call to self._assert_sql_safety(schema, table) right before, at this place this is not required and can be done via parameter passing, as it's not an DDL statement. Will change this.

Copy link
Member

Choose a reason for hiding this comment

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

In almost all cases, I don't see any reason to manually check for SQL safety, as the DB client should always (and psycopg2 does in this case!) provide means to send the values as arguments to the DB server.

To use SQL identifiers dynamically, psycopg2 also provides a tool:

If you need to generate dynamically SQL queries (for instance choosing dynamically a table name) you can use the facilities provided by the psycopg2.sql module:


There should also be a linting rule forbidding string interpolation with SQL queries.

@hbruch hbruch requested a review from derhuerst January 27, 2025 14:42
Copy link
Member

@derhuerst derhuerst left a comment

Choose a reason for hiding this comment

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

Haven't tested the code, but the changes look reasonable.

@hbruch hbruch force-pushed the fix/create-missing-pk branch from fe49cb3 to d9cee36 Compare January 27, 2025 15:11
@hbruch hbruch merged commit d774862 into main Jan 27, 2025
3 checks passed
@hbruch hbruch deleted the fix/create-missing-pk branch January 28, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants