Allow sql=id to be declared for columns#1066
Conversation
parsonsmatt
left a comment
parsonsmatt
left a comment
There was a problem hiding this comment.
Overall, I'm happy with these changes. This is a good step towards non-privileged id fields in persistent.
| | 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
| , 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.
| 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 |
2 participants
Before submitting your PR, check that you've:
@sincedeclarations to the HaddockAfter submitting your PR:
Fixes #1059