diff --git a/fixtures/safe_migrations_apps/bad_flow_long_identifier_app/__init__.py b/fixtures/safe_migrations_apps/bad_flow_long_identifier_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_long_identifier_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/bad_flow_long_identifier_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..ff1bba328f2ad5 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_long_identifier_app/migrations/0001_initial.py @@ -0,0 +1,24 @@ +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + + initial = True + + dependencies = [] + + operations = [ + migrations.CreateModel( + name="TestTable", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_long_identifier_app/migrations/0002_long_field_name.py b/fixtures/safe_migrations_apps/bad_flow_long_identifier_app/migrations/0002_long_field_name.py new file mode 100644 index 00000000000000..52e3460b899b30 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_long_identifier_app/migrations/0002_long_field_name.py @@ -0,0 +1,18 @@ +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + + dependencies = [ + ("bad_flow_long_identifier_app", "0001_initial"), + ] + + operations = [ + migrations.AddField( + model_name="testtable", + name="this_is_a_very_long_field_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes_and_should_fail", + field=models.IntegerField(null=True), + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_long_identifier_app/migrations/__init__.py b/fixtures/safe_migrations_apps/bad_flow_long_identifier_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_long_index_app/__init__.py b/fixtures/safe_migrations_apps/bad_flow_long_index_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_long_index_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/bad_flow_long_index_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..81c244487a7c45 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_long_index_app/migrations/0001_initial.py @@ -0,0 +1,25 @@ +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + + initial = True + + dependencies = [] + + operations = [ + migrations.CreateModel( + name="TestTable", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + ("name", models.CharField(max_length=100)), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_long_index_app/migrations/0002_long_index_name.py b/fixtures/safe_migrations_apps/bad_flow_long_index_app/migrations/0002_long_index_name.py new file mode 100644 index 00000000000000..b1e5a61cca933b --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_long_index_app/migrations/0002_long_index_name.py @@ -0,0 +1,20 @@ +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + + dependencies = [ + ("bad_flow_long_index_app", "0001_initial"), + ] + + operations = [ + migrations.AddIndex( + model_name="testtable", + index=models.Index( + fields=["name"], + name="this_is_a_very_long_index_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes_and_should_fail", + ), + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_long_index_app/migrations/__init__.py b/fixtures/safe_migrations_apps/bad_flow_long_index_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/src/sentry/new_migrations/migrations.py b/src/sentry/new_migrations/migrations.py index 4e90ae50c230d1..32dbcf9c1cbdeb 100644 --- a/src/sentry/new_migrations/migrations.py +++ b/src/sentry/new_migrations/migrations.py @@ -1,7 +1,6 @@ -from django.db.migrations import Migration, RunSQL, SeparateDatabaseAndState -from django_zero_downtime_migrations.backends.postgres.schema import UnsafeOperationException +from django.db.migrations import Migration -from sentry.new_migrations.monkey.special import SafeRunSQL +from sentry.new_migrations.validators import validate_operation class CheckedMigration(Migration): @@ -26,16 +25,3 @@ def apply(self, project_state, schema_editor, collect_sql=False): validate_operation(op) return super().apply(project_state, schema_editor, collect_sql) - - -def validate_operation(op): - if isinstance(op, RunSQL) and not isinstance(op, SafeRunSQL): - raise UnsafeOperationException( - "Using `RunSQL` is unsafe because our migrations safety framework can't detect problems with the " - "migration, and doesn't apply timeout and statement locks. Use `SafeRunSQL` instead, and get " - "approval from `owners-migrations` to make sure that it's safe." - ) - - if isinstance(op, SeparateDatabaseAndState): - for db_op in op.database_operations: - validate_operation(db_op) diff --git a/src/sentry/new_migrations/validators.py b/src/sentry/new_migrations/validators.py new file mode 100644 index 00000000000000..d9021157b1f66e --- /dev/null +++ b/src/sentry/new_migrations/validators.py @@ -0,0 +1,116 @@ +from __future__ import annotations + +from django.db import migrations +from django.db.migrations import RunSQL, SeparateDatabaseAndState +from django_zero_downtime_migrations.backends.postgres.schema import UnsafeOperationException + +from sentry.new_migrations.monkey.special import SafeRunSQL + + +def validate_operation(op): + if isinstance(op, RunSQL) and not isinstance(op, SafeRunSQL): + raise UnsafeOperationException( + "Using `RunSQL` is unsafe because our migrations safety framework can't detect problems with the " + "migration, and doesn't apply timeout and statement locks. Use `SafeRunSQL` instead, and get " + "approval from `owners-migrations` to make sure that it's safe." + ) + + if isinstance(op, SeparateDatabaseAndState): + for db_op in op.database_operations: + validate_operation(db_op) + + # Check identifier lengths for various operation types + _validate_operation_identifiers(op) + + +MAX_IDENTIFIER_LENGTH = 63 # max identifier length in postgres in bytes + + +def _validate_identifier_length(identifier): + """ + Validate that PostgreSQL identifiers don't exceed 63 bytes. + + Args: + identifier: The identifier to check + """ + if identifier and len(identifier.encode("utf-8")) > MAX_IDENTIFIER_LENGTH: + raise UnsafeOperationException( + f"PostgreSQL identifier '{identifier}' is {len(identifier.encode('utf-8'))} bytes long, " + f"which exceeds the {MAX_IDENTIFIER_LENGTH}-byte limit for identifiers in Postgres (e.g. table names, column names, index names)." + ) + + +def _validate_create_model_identifier_length(op): + _validate_identifier_length(op.name) + # check field names + for field_name, field in op.fields: + _validate_identifier_length(field_name) + if hasattr(field, "db_column"): + _validate_identifier_length(field.db_column) + # check indexes + if hasattr(op, "options") and op.options: + indexes = op.options.get("indexes", []) + for index in indexes: + if hasattr(index, "name") and index.name: + _validate_identifier_length(index.name) + + +def _validate_add_field_identifier_length(op): + """Validate identifier length for AddField operation.""" + _validate_identifier_length(op.name) + # Check db_column if specified + if hasattr(op.field, "db_column") and op.field.db_column: + _validate_identifier_length(op.field.db_column) + + +def _validate_rename_field_identifier_length(op): + """Validate identifier length for RenameField operation.""" + _validate_identifier_length(op.new_name) + + +def _validate_rename_model_identifier_length(op): + """Validate identifier length for RenameModel operation.""" + # Model names become table names (usually prefixed with app name) + _validate_identifier_length(op.new_name) + + +def _validate_add_index_identifier_length(op): + """Validate identifier length for AddIndex operation.""" + if hasattr(op.index, "name") and op.index.name: + _validate_identifier_length(op.index.name) + + +def _validate_remove_index_identifier_length(op): + """Validate identifier length for RemoveIndex operation.""" + if hasattr(op, "name") and op.name: + _validate_identifier_length(op.name) + + +def _validate_add_constraint_identifier_length(op): + """Validate identifier length for AddConstraint operation.""" + if hasattr(op.constraint, "name") and op.constraint.name: + _validate_identifier_length(op.constraint.name) + + +def _validate_remove_constraint_identifier_length(op): + """Validate identifier length for RemoveConstraint operation.""" + if hasattr(op, "name") and op.name: + _validate_identifier_length(op.name) + + +OPERATION_VALIDATOR = { + migrations.CreateModel: _validate_create_model_identifier_length, + migrations.AddField: _validate_add_field_identifier_length, + migrations.RenameField: _validate_rename_field_identifier_length, + migrations.RenameModel: _validate_rename_model_identifier_length, + migrations.AddIndex: _validate_add_index_identifier_length, + migrations.RemoveIndex: _validate_remove_index_identifier_length, + migrations.AddConstraint: _validate_add_constraint_identifier_length, + migrations.RemoveConstraint: _validate_remove_constraint_identifier_length, +} + + +def _validate_operation_identifiers(op): + validator = OPERATION_VALIDATOR.get(type(op)) + if validator: + validator(op) diff --git a/tests/sentry/db/postgres/schema/safe_migrations/integration/test_migrations.py b/tests/sentry/db/postgres/schema/safe_migrations/integration/test_migrations.py index ec739a26f6b8a7..73d944ad7b3b2a 100644 --- a/tests/sentry/db/postgres/schema/safe_migrations/integration/test_migrations.py +++ b/tests/sentry/db/postgres/schema/safe_migrations/integration/test_migrations.py @@ -483,3 +483,29 @@ def test(self): "SET statement_timeout TO '0ms';", "SET lock_timeout TO '0ms';", ] + + +class LongIdentifierFieldTest(BaseSafeMigrationTest): + app = "bad_flow_long_identifier_app" + migrate_from = "0001_initial" + migrate_to = "0002_long_field_name" + + def test(self): + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* is .* bytes long, which exceeds the 63-byte limit for identifiers in Postgres \\(e\\.g\\. table names, column names, index names\\)\\.", + ): + self.run_migration() + + +class LongIdentifierIndexTest(BaseSafeMigrationTest): + app = "bad_flow_long_index_app" + migrate_from = "0001_initial" + migrate_to = "0002_long_index_name" + + def test(self): + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* is .* bytes long, which exceeds the 63-byte limit for identifiers in Postgres \\(e\\.g\\. table names, column names, index names\\)\\.", + ): + self.run_migration() diff --git a/tests/sentry/new_migrations/test_validators.py b/tests/sentry/new_migrations/test_validators.py new file mode 100644 index 00000000000000..0c607139473cb5 --- /dev/null +++ b/tests/sentry/new_migrations/test_validators.py @@ -0,0 +1,379 @@ +import pytest +from django.db import migrations, models +from django.db.migrations import RunSQL, SeparateDatabaseAndState +from django_zero_downtime_migrations.backends.postgres.schema import UnsafeOperationException + +from sentry.new_migrations.monkey.special import SafeRunSQL +from sentry.new_migrations.validators import ( + MAX_IDENTIFIER_LENGTH, + _validate_identifier_length, + validate_operation, +) +from sentry.testutils.cases import TestCase + + +class ValidateIdentifierLengthTest(TestCase): + """Test the _validate_identifier_length function.""" + + def test_valid_identifier(self): + """Test that valid identifiers don't raise exceptions.""" + _validate_identifier_length("valid_identifier") + _validate_identifier_length("a" * MAX_IDENTIFIER_LENGTH) + _validate_identifier_length("") + _validate_identifier_length(None) + + def test_long_identifier(self): + """Test that long identifiers raise UnsafeOperationException.""" + long_identifier = "a" * (MAX_IDENTIFIER_LENGTH + 1) + with pytest.raises( + UnsafeOperationException, + match=f"PostgreSQL identifier .* is {MAX_IDENTIFIER_LENGTH + 1} bytes long", + ): + _validate_identifier_length(long_identifier) + + def test_utf8_identifier(self): + """Test that UTF-8 characters are counted correctly.""" + # Each emoji is 4 bytes + emoji_identifier = "😀" * 16 # 64 bytes, exceeds limit + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* is 64 bytes long", + ): + _validate_identifier_length(emoji_identifier) + + +class ValidateOperationTest(TestCase): + """Test the validate_operation function.""" + + def test_create_model_valid(self): + """Test CreateModel with valid identifiers.""" + op = migrations.CreateModel( + name="ValidModel", + fields=[ + ("id", models.AutoField(primary_key=True)), + ("valid_field", models.CharField(max_length=100)), + ("custom_column", models.IntegerField(db_column="custom_col")), + ], + options={ + "indexes": [ + models.Index(fields=["valid_field"], name="valid_index_name"), + ] + }, + ) + validate_operation(op) # Should not raise + + def test_create_model_long_name(self): + """Test CreateModel with long model name.""" + op = migrations.CreateModel( + name="VeryLongModelNameThatExceedsThePostgreSQLLimitOfSixtyThreeBytesForIdentifiers", + fields=[("id", models.AutoField(primary_key=True))], + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_create_model_long_field_name(self): + """Test CreateModel with long field name.""" + op = migrations.CreateModel( + name="Model", + fields=[ + ("id", models.AutoField(primary_key=True)), + ( + "very_long_field_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes", + models.CharField(max_length=100), + ), + ], + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_create_model_long_db_column(self): + """Test CreateModel with long db_column.""" + op = migrations.CreateModel( + name="Model", + fields=[ + ("id", models.AutoField(primary_key=True)), + ( + "field", + models.CharField( + max_length=100, + db_column="very_long_column_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes", + ), + ), + ], + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_create_model_long_index_name(self): + """Test CreateModel with long index name.""" + op = migrations.CreateModel( + name="Model", + fields=[ + ("id", models.AutoField(primary_key=True)), + ("field", models.CharField(max_length=100)), + ], + options={ + "indexes": [ + models.Index( + fields=["field"], + name="very_long_index_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes", + ), + ] + }, + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_add_field_valid(self): + """Test AddField with valid identifiers.""" + op = migrations.AddField( + model_name="Model", + name="new_field", + field=models.CharField(max_length=100), + ) + validate_operation(op) # Should not raise + + op_with_db_column = migrations.AddField( + model_name="Model", + name="field", + field=models.CharField(max_length=100, db_column="custom_column"), + ) + validate_operation(op_with_db_column) # Should not raise + + def test_add_field_long_name(self): + """Test AddField with long field name.""" + op = migrations.AddField( + model_name="Model", + name="very_long_field_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes", + field=models.CharField(max_length=100), + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_add_field_long_db_column(self): + """Test AddField with long db_column.""" + op = migrations.AddField( + model_name="Model", + name="field", + field=models.CharField( + max_length=100, + db_column="very_long_column_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes", + ), + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_rename_field_valid(self): + """Test RenameField with valid identifiers.""" + op = migrations.RenameField( + model_name="Model", + old_name="old_field", + new_name="new_field", + ) + validate_operation(op) # Should not raise + + def test_rename_field_long_new_name(self): + """Test RenameField with long new field name.""" + op = migrations.RenameField( + model_name="Model", + old_name="old_field", + new_name="very_long_new_field_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes", + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_rename_model_valid(self): + """Test RenameModel with valid identifiers.""" + op = migrations.RenameModel( + old_name="OldModel", + new_name="NewModel", + ) + validate_operation(op) # Should not raise + + def test_rename_model_long_new_name(self): + """Test RenameModel with long new model name.""" + op = migrations.RenameModel( + old_name="OldModel", + new_name="VeryLongNewModelNameThatExceedsThePostgreSQLLimitOfSixtyThreeBytesForIdentifiers", + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_add_index_valid(self): + """Test AddIndex with valid identifiers.""" + op = migrations.AddIndex( + model_name="Model", + index=models.Index(fields=["field"], name="valid_index_name"), + ) + validate_operation(op) # Should not raise + + def test_add_index_long_name(self): + """Test AddIndex with long index name.""" + op = migrations.AddIndex( + model_name="Model", + index=models.Index( + fields=["field"], + name="very_long_index_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes", + ), + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_add_index_no_name(self): + """Test AddIndex with no name doesn't raise.""" + # Django requires index names for AddIndex operations, but we can test + # that our validator doesn't crash when name is None/empty + op = migrations.AddIndex( + model_name="Model", + index=models.Index(fields=["field"], name="auto_generated_name"), + ) + validate_operation(op) # Should not raise + + def test_remove_index_valid(self): + """Test RemoveIndex with valid identifiers.""" + op = migrations.RemoveIndex( + model_name="Model", + name="valid_index_name", + ) + validate_operation(op) # Should not raise + + def test_remove_index_long_name(self): + """Test RemoveIndex with long index name.""" + op = migrations.RemoveIndex( + model_name="Model", + name="very_long_index_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes", + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_add_constraint_valid(self): + """Test AddConstraint with valid identifiers.""" + op = migrations.AddConstraint( + model_name="Model", + constraint=models.UniqueConstraint( + fields=["field"], + name="valid_constraint_name", + ), + ) + validate_operation(op) # Should not raise + + def test_add_constraint_long_name(self): + """Test AddConstraint with long constraint name.""" + op = migrations.AddConstraint( + model_name="Model", + constraint=models.UniqueConstraint( + fields=["field"], + name="very_long_constraint_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes", + ), + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_remove_constraint_valid(self): + """Test RemoveConstraint with valid identifiers.""" + op = migrations.RemoveConstraint( + model_name="Model", + name="valid_constraint_name", + ) + validate_operation(op) # Should not raise + + def test_remove_constraint_long_name(self): + """Test RemoveConstraint with long constraint name.""" + op = migrations.RemoveConstraint( + model_name="Model", + name="very_long_constraint_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes", + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + def test_run_sql_not_allowed(self): + """Test that RunSQL is not allowed.""" + op = RunSQL("SELECT 1;") + with pytest.raises( + UnsafeOperationException, + match="Using `RunSQL` is unsafe", + ): + validate_operation(op) + + def test_safe_run_sql_allowed(self): + """Test that SafeRunSQL is allowed.""" + op = SafeRunSQL("SELECT 1;") + validate_operation(op) # Should not raise + + def test_separate_database_and_state_validates_operations(self): + """Test that SeparateDatabaseAndState validates nested operations.""" + valid_op = migrations.AddField( + model_name="Model", + name="field", + field=models.CharField(max_length=100), + ) + invalid_op = migrations.AddField( + model_name="Model", + name="very_long_field_name_that_exceeds_the_postgresql_limit_of_sixty_three_bytes", + field=models.CharField(max_length=100), + ) + + # Valid operations should not raise + op = SeparateDatabaseAndState( + database_operations=[valid_op], + state_operations=[], + ) + validate_operation(op) + + # Invalid operations should raise + op = SeparateDatabaseAndState( + database_operations=[invalid_op], + state_operations=[], + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op) + + # Multiple operations - should validate all + op = SeparateDatabaseAndState( + database_operations=[valid_op, invalid_op], + state_operations=[], + ) + with pytest.raises( + UnsafeOperationException, + match="PostgreSQL identifier .* exceeds the 63-byte limit", + ): + validate_operation(op)