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 4714 postgresql alter column nullability #4831

Merged
merged 5 commits into from
Nov 27, 2023

Conversation

griffio
Copy link
Contributor

@griffio griffio commented Nov 22, 2023

fixes #4714

🚧 πŸ” πŸ—οΈ PR to try and fix TableInterfaceGenerator for PostgreSql Dialect

The AlterColumn Mixin will set the QueryColumn to appropriate nullable flag and this can be read in TableInterfaceGenerator

As we can't modify ColumnDefs, use the QueryColumn to override the not null/nullable type if it is true or false (normally the nullable property will be null so no impact)

🦺 Migration Test case to show it works

Have tried locally with snapshot build πŸ“Έ

four fields - drop and set nullability on y, t - keep a, b the same as a control

1.sqm

CREATE TABLE X(
    y INTEGER NOT NULL,
    t TEXT,
    a INTEGER NOT NULL,
    b TEXT
);
                 Table "public.x"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 y      | integer |           | not null |
 t      | text    |           |          |
 a      | integer |           | not null |
 b      | text    |           |          |
public data class X(
  public val y: Int,
  public val t: String?,
  public val a: Int,
  public val b: String?,
)

public class XQueries(
  driver: SqlDriver,
) : TransacterImpl(driver) {
  public fun insertX(
    y: Int,
    t: String?,
    a: Int,
    b: String?,
  ) 

2.sqm

ALTER TABLE X
ALTER COLUMN y DROP NOT NULL;

ALTER TABLE X
ALTER COLUMN t SET NOT NULL;

After:

public data class X(
  public val y: Int?,
  public val t: String,
  public val a: Int,
  public val b: String?,
)

public class XQueries(
  driver: SqlDriver,
) : TransacterImpl(driver) {
  public fun insertX(
    y: Int?,
    t: String,
    a: Int,
    b: String?,
  ) 
                 Table "public.x"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 y      | integer |           |          |
 t      | text    |           | not null |
 a      | integer |           | not null |
 b      | text    |           |          |

@griffio griffio marked this pull request as ready for review November 22, 2023 18:42
Create a rule to express the not null constraint
Find the not null column constraint SET or DROP and set
nullability on QueryColumn
If the QueryColumn has been modified the nullable flag will be will either true or false
Override the columnDef nullable type
Testing that the data class properties have update nullable types and unmodified remain the same
The QueryColumn nullable property can be null, true or false
We can always assign queryColumn.copy(nullable = nullableColumn) when the column matches
@griffio griffio force-pushed the fix-4714-postgresql-alter-column branch from ba81ccd to 2490329 Compare November 25, 2023 15:16
@griffio griffio changed the title Fix 4714 postgresql alter column Fix 4714 postgresql alter column nullability Nov 26, 2023
@hfhbd hfhbd merged commit f25d1c3 into cashapp:master Nov 27, 2023
7 checks passed
@griffio griffio deleted the fix-4714-postgresql-alter-column branch November 27, 2023 12:30
hfhbd pushed a commit that referenced this pull request Apr 2, 2024
* Change postgresql grammar

Create a rule to express the not null constraint

* Update AlterTableAlterColumnMixin

Find the not null column constraint SET or DROP and set
nullability on QueryColumn

* Update TableInterfaceGenerator

If the QueryColumn has been modified the nullable flag will be will either true or false
Override the columnDef nullable type

* Add migration tests

Testing that the data class properties have update nullable types and unmodified remain the same

* Refactor applyTo

The QueryColumn nullable property can be null, true or false
We can always assign queryColumn.copy(nullable = nullableColumn) when the column matches
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.

Not supported for Postgresql: ALTER COLUMN y DROP NOT NULL
2 participants