Skip to content

Commit bb5b7b5

Browse files
hdgarroodhdgarrood
authored
postgresql: Add foreign key constraints the first time when migrating (#1614)
* postgresql: Add foreign key constraints the first time when migrating * Remove unneeded guard * refactor: don't match on all of the Column fields * Add changelog + bump version * Fix
1 parent 9d88bf4 commit bb5b7b5

File tree

4 files changed

+115
-45
lines changed

4 files changed

+115
-45
lines changed

‎persistent-postgresql/ChangeLog.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
# Changelog for persistent-postgresql
22

3+
# 2.14.2.0
4+
5+
* [#1614](https://github.com/yesodweb/persistent/pull/1614)
6+
* Generate migrations to create foreign key constraints while adding new foreign key columns (previously you had to generate migrations twice to create foreign key constraints)
7+
38
# 2.14.1.0
49

510
* [#1612](https://github.com/yesodweb/persistent/pull/1612)

‎persistent-postgresql/Database/Persist/Postgresql/Internal/Migration.hs‎

Lines changed: 51 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1097,80 +1097,87 @@ findAlters
10971097
-> EntityDef
10981098
-- ^ The entity definition for the entity that we're working on.
10991099
-> Column
1100-
-- ^ The column that we're searching for potential alterations for.
1100+
-- ^ The column that we're searching for potential alterations for, derived
1101+
-- from the Persistent EntityDef. That is: this is how we _want_ the column
1102+
-- to look, and not necessarily how it actually looks in the database right
1103+
-- now.
11011104
-> [Column]
1105+
-- ^ The columns for this table, as they currently exist in the database.
11021106
-> ([AlterColumn], [Column])
1103-
findAlters defs edef col@(Column name isNull sqltype def _gen _defConstraintName _maxLen ref) cols =
1104-
case List.find (\c -> cName c == name) cols of
1107+
findAlters defs edef newCol oldCols =
1108+
case List.find (\c -> cName c == cName newCol) oldCols of
11051109
Nothing ->
1106-
([AddColumn col], cols)
1110+
([AddColumn newCol] ++ refAdd (cReference newCol), oldCols)
11071111
Just
1108-
(Column _oldName isNull' sqltype' def' _gen' _defConstraintName' _maxLen' ref') ->
1112+
oldCol ->
11091113
let
11101114
refDrop Nothing = []
11111115
refDrop (Just ColumnReference{crConstraintName = cname}) =
11121116
[DropReference cname]
11131117

1114-
refAdd Nothing = []
1115-
refAdd (Just colRef) =
1116-
case find ((== crTableName colRef) . getEntityDBName) defs of
1117-
Just refdef
1118-
| Just _oldName /= fmap fieldDB (getEntityIdField edef) ->
1119-
[ AddReference
1120-
(crTableName colRef)
1121-
(crConstraintName colRef)
1122-
(name NEL.:| [])
1123-
(NEL.toList $ Util.dbIdColumnsEsc escapeF refdef)
1124-
(crFieldCascade colRef)
1125-
]
1126-
Just _ -> []
1127-
Nothing ->
1128-
error $
1129-
"could not find the entityDef for reftable["
1130-
++ show (crTableName colRef)
1131-
++ "]"
11321118
modRef =
1133-
if equivalentRef ref ref'
1119+
if equivalentRef (cReference oldCol) (cReference newCol)
11341120
then []
1135-
else refDrop ref' ++ refAdd ref
1136-
modNull = case (isNull, isNull') of
1121+
else refDrop (cReference oldCol) ++ refAdd (cReference newCol)
1122+
modNull = case (cNull newCol, cNull oldCol) of
11371123
(True, False) -> do
1138-
guard $ Just name /= fmap fieldDB (getEntityIdField edef)
1139-
pure (IsNull col)
1124+
guard $ Just (cName newCol) /= fmap fieldDB (getEntityIdField edef)
1125+
pure (IsNull newCol)
11401126
(False, True) ->
11411127
let
1142-
up = case def of
1128+
up = case cDefault newCol of
11431129
Nothing -> id
1144-
Just s -> (:) (UpdateNullToValue col s)
1130+
Just s -> (:) (UpdateNullToValue newCol s)
11451131
in
1146-
up [NotNull col]
1132+
up [NotNull newCol]
11471133
_ -> []
11481134
modType
1149-
| sqlTypeEq sqltype sqltype' = []
1135+
| sqlTypeEq (cSqlType newCol) (cSqlType oldCol) = []
11501136
-- When converting from Persistent pre-2.0 databases, we
11511137
-- need to make sure that TIMESTAMP WITHOUT TIME ZONE is
11521138
-- treated as UTC.
1153-
| sqltype == SqlDayTime && sqltype' == SqlOther "timestamp" =
1154-
[ ChangeType col sqltype $
1139+
| cSqlType newCol == SqlDayTime && cSqlType oldCol == SqlOther "timestamp" =
1140+
[ ChangeType newCol (cSqlType newCol) $
11551141
T.concat
11561142
[ " USING "
1157-
, escapeF name
1143+
, escapeF (cName newCol)
11581144
, " AT TIME ZONE 'UTC'"
11591145
]
11601146
]
1161-
| otherwise = [ChangeType col sqltype ""]
1147+
| otherwise = [ChangeType newCol (cSqlType newCol) ""]
11621148
modDef =
1163-
if def == def'
1164-
|| isJust (T.stripPrefix "nextval" =<< def')
1149+
if cDefault newCol == cDefault oldCol
1150+
|| isJust (T.stripPrefix "nextval" =<< cDefault oldCol)
11651151
then []
1166-
else case def of
1167-
Nothing -> [NoDefault col]
1168-
Just s -> [Default col s]
1152+
else case cDefault newCol of
1153+
Nothing -> [NoDefault newCol]
1154+
Just s -> [Default newCol s]
11691155
dropSafe =
1170-
if safeToRemove edef name
1171-
then error "wtf" [Drop col (SafeToRemove True)]
1156+
if safeToRemove edef (cName newCol)
1157+
then error "wtf" [Drop newCol (SafeToRemove True)]
11721158
else []
11731159
in
11741160
( modRef ++ modDef ++ modNull ++ modType ++ dropSafe
1175-
, filter (\c -> cName c /= name) cols
1161+
, filter (\c -> cName c /= cName newCol) oldCols
11761162
)
1163+
where
1164+
refAdd Nothing = []
1165+
-- This check works around a bug where persistent will sometimes
1166+
-- generate an erroneous ForeignRef for ID fields.
1167+
-- See: https://github.com/yesodweb/persistent/issues/1615
1168+
refAdd _ | fmap fieldDB (getEntityIdField edef) == Just (cName newCol) = []
1169+
refAdd (Just colRef) =
1170+
case find ((== crTableName colRef) . getEntityDBName) defs of
1171+
Just refdef ->
1172+
[ AddReference
1173+
(crTableName colRef)
1174+
(crConstraintName colRef)
1175+
(cName newCol NEL.:| [])
1176+
(NEL.toList $ Util.dbIdColumnsEsc escapeF refdef)
1177+
(crFieldCascade colRef)
1178+
]
1179+
Nothing ->
1180+
error $
1181+
"could not find the entityDef for reftable["
1182+
++ show (crTableName colRef)
1183+
++ "]"

‎persistent-postgresql/persistent-postgresql.cabal‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
name: persistent-postgresql
2-
version: 2.14.1.0
2+
version: 2.14.2.0
33
license: MIT
44
license-file: LICENSE
55
author: Felipe Lessa, Michael Snoyman <[email protected]>

‎persistent-postgresql/test/MigrationSpec.hs‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,17 @@ AdminUser sql=admin_users
6060

6161
promotedByUserId UserId
6262
UniquePromotedByUserId promotedByUserId
63+
64+
FKParent sql=migration_fk_parent
65+
66+
FKChildV1 sql=migration_fk_child
67+
68+
-- Simulate creating a new FK field on an existing table
69+
FKChildV2 sql=migration_fk_child
70+
parentId FKParentId
71+
72+
ExplicitPrimaryKey sql=explicit_primary_key
73+
Id Text
6374
|]
6475

6576
userEntityDef :: EntityDef
@@ -77,15 +88,32 @@ password2EntityDef = entityDef (Proxy :: Proxy Password2)
7788
adminUserEntityDef :: EntityDef
7889
adminUserEntityDef = entityDef (Proxy :: Proxy AdminUser)
7990

91+
fkParentEntityDef :: EntityDef
92+
fkParentEntityDef = entityDef (Proxy :: Proxy FKParent)
93+
94+
fkChildV1EntityDef :: EntityDef
95+
fkChildV1EntityDef = entityDef (Proxy :: Proxy FKChildV1)
96+
97+
fkChildV2EntityDef :: EntityDef
98+
fkChildV2EntityDef = entityDef (Proxy :: Proxy FKChildV2)
99+
100+
explicitPrimaryKeyEntityDef :: EntityDef
101+
explicitPrimaryKeyEntityDef = entityDef (Proxy :: Proxy ExplicitPrimaryKey)
102+
103+
-- Note that FKChild is deliberately omitted here because we have two
104+
-- versions of it
80105
allEntityDefs :: [EntityDef]
81106
allEntityDefs =
82107
[ userEntityDef
83108
, userFriendshipEntityDef
84109
, passwordEntityDef
85110
, password2EntityDef
86111
, adminUserEntityDef
112+
, fkParentEntityDef
113+
, explicitPrimaryKeyEntityDef
87114
]
88115

116+
-- Note that this function migrates to the schema expected by FKChildV1
89117
migrateManually :: (HasCallStack, MonadIO m) => SqlPersistT m ()
90118
migrateManually = do
91119
cleanDB
@@ -150,6 +178,9 @@ migrateManually = do
150178
, " ADD CONSTRAINT unique_promoted_by_user_id"
151179
, " UNIQUE(promoted_by_user_id);"
152180
]
181+
rawEx "CREATE TABLE migration_fk_parent(id int8 primary key);"
182+
rawEx "CREATE TABLE migration_fk_child(id int8 primary key);"
183+
rawEx "CREATE TABLE explicit_primary_key(id text primary key);"
153184
rawEx "CREATE TABLE ignored(id int8 primary key);"
154185

155186
cleanDB :: (HasCallStack, MonadIO m) => SqlPersistT m ()
@@ -162,6 +193,9 @@ cleanDB = do
162193
rawEx "DROP TABLE IF EXISTS ignored;"
163194
rawEx "DROP TABLE IF EXISTS admin_users;"
164195
rawEx "DROP TABLE IF EXISTS users;"
196+
rawEx "DROP TABLE IF EXISTS migration_fk_child;"
197+
rawEx "DROP TABLE IF EXISTS migration_fk_parent;"
198+
rawEx "DROP TABLE IF EXISTS explicit_primary_key;"
165199

166200
spec :: Spec
167201
spec = describe "MigrationSpec" $ do
@@ -582,3 +616,27 @@ spec = describe "MigrationSpec" $ do
582616
result2 <-
583617
liftIO $ migrateEntitiesStructured getter allEntityDefs allEntityDefs
584618
result2 `shouldBe` Right []
619+
620+
it "suggests FK constraints for new fields first time" $ runConnAssert $ do
621+
migrateManually
622+
623+
getter <- getStmtGetter
624+
result <-
625+
liftIO $
626+
migrateEntitiesStructured
627+
getter
628+
(fkChildV2EntityDef : allEntityDefs)
629+
[fkChildV2EntityDef]
630+
631+
cleanDB
632+
633+
case result of
634+
Right [] ->
635+
pure ()
636+
Left err ->
637+
expectationFailure $ show err
638+
Right alters ->
639+
map (snd . showAlterDb) alters
640+
`shouldBe` [ "ALTER TABLE \"migration_fk_child\" ADD COLUMN \"parent_id\" INT8 NOT NULL"
641+
, "ALTER TABLE \"migration_fk_child\" ADD CONSTRAINT \"migration_fk_child_parent_id_fkey\" FOREIGN KEY(\"parent_id\") REFERENCES \"migration_fk_parent\"(\"id\") ON DELETE RESTRICT ON UPDATE RESTRICT"
642+
]

0 commit comments

Comments
 (0)