Migration with is not idempotent when combining default=null and Maybe type

[danbroooks]

On the project that I work on, we have a default field similar to this:

SomeEntity
  name    Text Maybe    default=null

When we run migrations on the project, we always get:

ALTER TABLE "some_entity" ALTER COLUMN "name" SET DEFAULT null;

The column default gets set as 'nullable' in the database but every time we run migrations this ALTER TABLE is still output.

As with #1532 I looked at this by adding some logging here:

persistent/persistent-postgresql/Database/Persist/Postgresql.hs

Line 1568 in a4b4890

else let (acs, ats) = getAlters allDefs entity (newcols, udspair) old'

And looked in newcols and fst old' (the new columns vs existing columns), showing the field name and default value on the Column.

Here are the values that were logged out:

-- this is from `newcols`
(FieldNameDB {unFieldNameDB = "text_field"},Just "null")

-- this is from `fst old`
(FieldNameDB {unFieldNameDB = "text_field"},Nothing)

Different to 1532 where there is obviously a mis-match with the cases of the strings, I am less clear what is going on here. I am familiar enough with the types in persistent to know that the concept of nullable is also tracked beyond this field cDefault field here (ie on column there is cNull, there are also similar fields on other types), and I think it might be that in the case where there is Nothing for the default value the null has actually been converted into a flag to signal it being nullable somewhere else (while that hasn't happened on the parsed migration).

I'm not sure at all what the solution would be to this, but I'd be happy to also look to fix this too if I could get some steer in the right direction.

A key point to this issue is I think it is down to the fact that the field is already using Maybe in conjunction to a default null value, in this case, we may not actually need to specify default=null in our entity definitions, and that might be the solution here. But it may also be good for persistent to warn about this if you write this in your entity definitions.

I created a test to recreate this, there is an open PR with failing test here:

#1534

[parsonsmatt]

Isn't the default already null for nullable columns? If so the bug fix is to remove default=null from the model definition.

Perhaps the TH can parse this as unnecessary?