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

Not supported for Postgresql: ALTER COLUMN y DROP NOT NULL #4714

Closed
tjerkw opened this issue Oct 11, 2023 · 9 comments · Fixed by #4831
Closed

Not supported for Postgresql: ALTER COLUMN y DROP NOT NULL #4714

tjerkw opened this issue Oct 11, 2023 · 9 comments · Fixed by #4831
Labels

Comments

@tjerkw
Copy link

tjerkw commented Oct 11, 2023

SQLDelight Version

2.0.0

SQLDelight Dialect

postgresql

Describe the Bug

Drop not null is not supported. Only way to mitigate this is to drop the column and recreate it again.

ALTER TABLE X ALTER COLUMN Y DROP NOT NULL

Stacktrace

No response

@tjerkw tjerkw added the bug label Oct 11, 2023
@griffio
Copy link
Contributor

griffio commented Oct 11, 2023

Support should be in 2.0.0 was merged back in May 🕊️

#4165

Are you seeing a compilation error

Have tried the below syntax in migration file

Note: Alter table statements are forbidden outside of migration files

CREATE TABLE X(
  Y INTEGER NOT NULL
 );

ALTER TABLE X ALTER COLUMN Y DROP NOT NULL;

@Thomas-Kuipers
Copy link

Thomas-Kuipers commented Oct 12, 2023

I'm also experiencing a problem with ALTER TABLE X ALTER COLUMN Y DROP NOT NULL in my migration file. There are no compilation errors, but the statement seems to have no effect. In my Postgres database I observe that the column is still not nullable.

@griffio
Copy link
Contributor

griffio commented Oct 12, 2023

🤔 I tried it two ways - let me know if you have a different structure?

with deriveSchemaFromMigrations.set(true) on empty schema

Two statements CREATE TABLE and ALTER TABLE in 1.sqm file (1->2)

 val driver = getSqlDriver()
 Database.Schema.migrate(driver, 1, 2)

Then again with the CREATE in 1.sqm and ALTER in 2.sqm the migration across two versions

    Database.Schema.migrate(driver, 2, 3)

DB Checked

postgres=# \d x
                 Table "public.x"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 y      | integer |           |          |

@griffio
Copy link
Contributor

griffio commented Oct 19, 2023

🧾 I think the issue being raised here is that the generated field y should be INT? after ALTER TABLE X ALTER COLUMN Y DROP NOT NULL.

This only does so if the column is dropped and added in the migration file

  public data class X(
  public val y: Int?,
)

@griffio
Copy link
Contributor

griffio commented Oct 30, 2023

After looking into 👓 - it's quite tricky to solve with the current code and I can only get it to half work.
It works for MySql as the modify column statement uses a SqlColumnDef type to add/remove the constraint.
For PostgreSql this doesn't work as the SqlColumnDef is only stored on the parent table def and can't be modified.
😞
TableInterfaceGenerator shows that getColumnConstraintList is used to determine the javaType nullability (NULL or NOT NULL)

table.query.columns.map { it.element as NamedElement }.forEach { column ->
val columnName = allocateName(column)
val columnDef = column.columnDefSource()!!
val columnType = columnDef.columnType as ColumnTypeMixin
val javaType = columnType.type().javaType

@iruizmar
Copy link

👋🏻 Any workaround for the migration not being applied to the generated code?

@griffio
Copy link
Contributor

griffio commented Nov 20, 2023

@iruizmar
🕊️ Sadly, the only work around I have tried is a bit of a hack 💇‍♂️ - Before running the ALTER TABLE X ALTER COLUMN Y DROP NOT NULL in your next version .sqm, change the CREATE TABLE constraints to what you need in the original .sqm where it is defined - This is because during code generation the constraints are still referencing the CREATE TABLE column definitions.

Example - to drop the not null constraint on y

Existing 1.sqm file that is the current schema state

CREATE TABLE X(
    y INTEGER NOT NULL,
    t TEXT
);

Change to

CREATE TABLE X(
    y INTEGER,
    t TEXT
);

Your new migration
2.sqm

ALTER TABLE X
ALTER COLUMN y DROP NOT NULL;

Then run your migration - check that the Table data class has the nullable type and any queries are nullable type now
☘️

@iruizmar
Copy link

Thanks for that, @griffio.

That's a brilliant idea to deceive the SQLDelight code generation, but it won't work in some CI environments in which migrations are verified via checksum, for example, just as Flyway does. It'll modify the generated migration and Flyway will complain (if verification is enabled, which I guess should be for security reasons).

Or am I misunderstanding something?

@griffio
Copy link
Contributor

griffio commented Nov 21, 2023

🔕 it's not a work-around for such a scenario, only useful if it helps immediately.

Somehow a "fix" in the PostgreSQL Dialect where the new constraints 🔗 are modified and applied during code generation will have to be found.

Currently there is quite a lot of PostgreSql functionality missing (The upcoming 2.1 release will have some improvements) #3944 (reply in thread)

However, supporting migrations correctly is a priority as otherwise prevents greater adoption in production environments. 👨‍🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants