Fix: Make constraint queries schema-aware for multi-tenancy #93
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this change, the test suite only verified that the generated SQL matched the expected format, but didn't test the actual multi-tenant behavior. The bug would not have been caught by existing tests. This change adds 5 new integration tests that create multiple PostgreSQL schemas and verify that constraint queries are schema-aware: - test_check_existing_constraint_only_sees_current_schema - test_check_constraint_is_valid_only_sees_current_schema - test_check_constraint_is_not_valid_only_sees_current_schema - test_unique_constraint_creation_in_second_schema - test_check_constraint_creation_in_second_schema All tests are marked with @pytest.mark.xfail because the bug has not yet been fixed. They fail as expected, demonstrating the bug: - First 3 tests: Assert that queries don't see other schemas (they do) - Last 2 tests: Assert constraints are created in both schemas (they aren't, because the buggy queries make the operations think the constraint already exists) These tests use schema-aware SQL for verification to ensure they actually detect the bug, not just test the buggy queries with themselves. Related issue: kraken-tech#92
Prior to this change, the ConstraintQueries class queried pg_catalog.pg_constraint without filtering by PostgreSQL schema. This caused incorrect behavior in multi-tenant environments using django-tenants, where each tenant has its own schema. The queries would detect constraints from other tenants' schemas, leading to false positives (e.g., ConstraintAlreadyExists errors when creating a second tenant). This change adds INNER JOINs with pg_catalog.pg_class and pg_catalog.pg_namespace to filter constraints by current_schema(). This ensures that constraint checks only look at the current tenant's schema, not across all schemas in the database. Affected queries: - CHECK_EXISTING_CONSTRAINT - CHECK_CONSTRAINT_IS_VALID - CHECK_CONSTRAINT_IS_NOT_VALID Related issue: kraken-tech#92
RJPercival
force-pushed
the
fix-schema-awareness-multitenancy
branch
from
de312d9 to
779bf2c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #92
This PR makes the
ConstraintQueriesclass schema-aware to support multi-tenant environments using django-tenants.There is a similar PR for kraken-core: https://github.com/octoenergy/kraken-core/pull/230402. That PR also adds semgrep rules to mitigate against regressions - since semgrep doesn't seem to be set up for this repo, I haven't included similar rules here.
Problem
Prior to this change, the three constraint check queries (
CHECK_EXISTING_CONSTRAINT,CHECK_CONSTRAINT_IS_VALID, andCHECK_CONSTRAINT_IS_NOT_VALID) queriedpg_catalog.pg_constraintwithout filtering by PostgreSQL schema. In multi-tenant setups where each tenant has its own schema, these queries would detect constraints from other tenants' schemas, causing false positives.For example, when creating a second tenant, migrations would fail with
ConstraintAlreadyExistserrors because the queries found constraints from the first tenant's schema.Solution
Added INNER JOINs with
pg_catalog.pg_classandpg_catalog.pg_namespaceto filter constraints bycurrent_schema(). This ensures that constraint checks only examine the current tenant's schema.Changes
CHECK_EXISTING_CONSTRAINTto filter by schemaCHECK_CONSTRAINT_IS_VALIDto filter by schemaCHECK_CONSTRAINT_IS_NOT_VALIDto filter by schemaTesting
Behavioral Tests
Commit 1 adds 5 integration tests that create multiple PostgreSQL schemas:
test_check_existing_constraint_only_sees_current_schematest_check_constraint_is_valid_only_sees_current_schematest_check_constraint_is_not_valid_only_sees_current_schematest_unique_constraint_creation_in_second_schematest_check_constraint_creation_in_second_schemaAll tests are marked
@pytest.mark.xfailin commit 1, demonstrating they detect the bug:The tests use schema-aware SQL for verification to ensure they actually detect the bug, not just test the buggy queries with themselves.
Test Results