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

Migrating a table with a custom primary key fails in SQLite #669

Closed
JaSpa opened this issue May 1, 2017 · 1 comment
Closed

Migrating a table with a custom primary key fails in SQLite #669

JaSpa opened this issue May 1, 2017 · 1 comment

Comments

@JaSpa
Copy link
Contributor

JaSpa commented May 1, 2017

Description

Let's say we have this model definition

A
  fieldA Int
  Primary fieldA

and change it to

A
  fieldA Int
  fieldB Int default=0
  Primary fieldA

The migration fails with table a_backup has no column named id, because the generated SQL still includes the non-existing id-column although we have a custom primary key:

CREATE TEMP TABLE "a_backup"("fieldA" INTEGER NOT NULL,"field_b" INTEGER NOT NULL DEFAULT 0, PRIMARY KEY ("fieldA"));
INSERT INTO "a_backup"("id","fieldA") SELECT "id","fieldA" FROM "a";

Identified problem

The problem lies in these two lines, as they prepend the id column regardless of its actual existence.

Patch

This following patch works for me, but I don't really understand the ReferenceDef type, but if I have got it correctly and there are no other cases to consider, I'll prepare a pull request + test case.

diff --git a/persistent-sqlite/Database/Persist/Sqlite.hs b/persistent-sqlite/Database/Persist/Sqlite.hs
index ab72ec7..d811578 100644
--- a/persistent-sqlite/Database/Persist/Sqlite.hs
+++ b/persistent-sqlite/Database/Persist/Sqlite.hs
@@ -375,12 +375,11 @@ getCopyTable allDefs getter def = do
     let oldCols = map DBName $ filter (/= "id") oldCols' -- need to update for table id attribute ?
     let newCols = filter (not . safeToRemove def) $ map cName cols
     let common = filter (`elem` oldCols) newCols
-    let id_ = fieldDB (entityId def)
     return [ (False, tmpSql)
-           , (False, copyToTemp $ id_ : common)
+           , (False, copyToTemp $ addIdCol common)
            , (common /= filter (not . safeToRemove def) oldCols, dropOld)
            , (False, newSql)
-           , (False, copyToFinal $ id_ : newCols)
+           , (False, copyToFinal $ addIdCol newCols)
            , (False, dropTmp)
            ]
   where
@@ -418,6 +417,10 @@ getCopyTable allDefs getter def = do
         , " FROM "
         , escape tableTmp
         ]
+    addIdCol = let id_ = entityId def
+                in case fieldReference id_ of
+                     CompositeRef _ -> id
+                     _ -> (fieldDB id_ :)
 
 mkCreateTable :: Bool -> EntityDef -> ([Column], [UniqueDef]) -> Text
 mkCreateTable isTemp entity (cols, uniqs) =
@gregwebs
Copy link
Member

gregwebs commented Jul 8, 2017

Looks good. Probably use the entityPrimary helper.

parsonsmatt added a commit that referenced this issue Jul 16, 2019
Support for composite primary key migration in SQLite. Fixes #669.
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

No branches or pull requests

2 participants