Allow sql=id to be declared for columns #1066
Conversation
| @@ -960,6 +991,7 @@ findAlters defs _tablename col@(Column name isNull sqltype def _defConstraintNam | |||
| | otherwise = [(name, ChangeType sqltype "")] | |||
| modDef = | |||
| if def == def' | |||
| || isJust (T.stripPrefix "nextval" =<< def') | |||
There was a problem hiding this comment.
kind of a nasty hack :\ this will keep it from removing a nextval default value as picked up from the database.
| in v `seq` vs `seq` (v:vs) | ||
| let !v = g col | ||
| !vs = use gs cols | ||
| in (v:vs) |
There was a problem hiding this comment.
this is really like a strict map but I am not entirely sure where that lives?
| return $ Right $ map showAlterDb $ acs' ++ ats' | ||
| -- Errors | ||
| (_, _, (errs, _)) -> return $ Left errs | ||
| case ([], old, partitionEithers old) of |
There was a problem hiding this comment.
Sorry for the syntactic noise here but I just couldn't read this code. I formatted it and made it a bit more spaced out to make it easier to read.
| let refTarget = | ||
| addReference allDefs refConstraintName refTblName cname | ||
|
|
||
| guard $ refTblName /= name && cname /= fieldDB (entityId val) |
There was a problem hiding this comment.
Now that the id column is part of the columns returned by mkColumn we have to treat the reference separately. We don't want a self-reference on the ID column treated as a foreign key.
persistent-mysql/test/main.hs
Outdated
| @@ -119,7 +119,7 @@ main = do | |||
| , Recursive.recursiveMigrate | |||
| , CompositeTest.compositeMigrate | |||
| , PersistUniqueTest.migration | |||
| , RenameTest.migration | |||
| -- , RenameTest.migration | |||
There was a problem hiding this comment.
Currently broken on mysql because DEFAULT=CURRENT_DATE is not supported on mysql ???
| ] | ||
| where | ||
| nonIdCols = | ||
| filter (\c -> cName c /= fieldDB (entityId entity) ) cols |
There was a problem hiding this comment.
We filter out the id column here,because it is treated specially in idtxt. And unfortunately it would be pretty difficult/annoying to set a default at the Column level because that bit isn't database agnostic.
|
|
||
| CustomSqlId | ||
| pk Int sql=id | ||
| Primary pk |
There was a problem hiding this comment.
This is the actual test case for the issue in question.
| @@ -38,6 +43,7 @@ specsWith runDb = describe "Migration" $ do | |||
| again <- getMigration migrationMigrate | |||
| liftIO $ again @?= [] | |||
| it "really is idempotent" $ runDb $ do | |||
| runMigrationSilent migrationMigrate | |||
There was a problem hiding this comment.
reall,y really, REALLY is idempotent
|
@tysonzero want to give this a shot? |
|
It seems to have fixed the issue. Although now it's trying to add a lot of foreign keys from tables to themselves: |
|
I think I've fixed the self-reference hack, thanks for noticing that :D |
Before submitting your PR, check that you've:
@sincedeclarations to the HaddockAfter submitting your PR:
Fixes #1059