-
Notifications
You must be signed in to change notification settings - Fork 292
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
PersistLiteral support for SQL keywords #1122
Conversation
@@ -552,6 +554,19 @@ instance PGFF.FromField Unknown where | |||
instance PGTF.ToField Unknown where | |||
toField (Unknown a) = PGTF.Escape a | |||
|
|||
newtype UnknownLiteral = UnknownLiteral { unUnknownLiteral :: ByteString } | |||
deriving (Eq, Show, Read, Ord, Typeable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deriving (Eq, Show, Read, Ord, Typeable) | |
deriving (Eq, Show, Read, Ord) |
Typeable is automatically derived since iirc GHC 7.10
newtype UnknownLiteral = UnknownLiteral { unUnknownLiteral :: ByteString } | ||
deriving (Eq, Show, Read, Ord, Typeable) | ||
|
||
instance PGFF.FromField UnknownLiteral where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this instance used?
This would also work for calling functions, right? eg if I want to set a column to the database’s NOW() value or some other function? |
This is a fairly dangerous tool, since any dynamic value opens you up to SQL injection, right? Thoughts on adding more warning flags, or a name like UnsafeUnescapedLiteral? (Is it possible to call the escaping function at all for this, eg to implement something like ARRAY(‘a’), or can this only be used for static values (eg DEFAULT)? |
Yes, it would be governed by specific instances of newtype ComputedColumn a = ComputedColumn a
instance PersistField a => PersistField (ComputedColumn a) where
toPersistValue _ = PersistLiteral "DEFAULT"
fromPersistValue x = ComputedColumn <$> fromPersistValue x
instance PersistFieldSql a => PersistFieldSql (ComputedColumn a) where
sqlType _ = sqlType (Proxy @a) And that's just because that's how Postgres allows you to insert a row into a table that has a computed column. If you wanted to use newtype Timestamp = Timestamp UTCTime
instance PersistField Timestamp where
toPersistValue _ = PersistLiteral "now()"
fromPersistValue x = Timestamp <$> fromPersistValue x
instance PersistFieldSql Timestamp where
sqlType _ = sqlType (Proxy @UTCTime) |
In both use cases I give above, the literal happens to be completely static. I wonder if this suggests that a more-limited API would be better. Perhaps there's a way to use a quasiquote to ensures that the implementation of |
@parsonsmatt I see you lurking in this thread. IIRC, you were working on something for timestamps. I'm interested to hear how this concept relates to your work. Does your work subsume this concept? Are they orthogonal? |
In persistent currently, PersistDbSpecific only escapes values in persistent-postgresql but not persistent-mysql or persistent-sqlite. This explains @MaxGabriel 's questions about "I’m confused how [PersistLiteral can have] the same implementation as PersistDbSpecific" in the SQLite and MySQL implementations. It may be that is instead a bug in persistent-postgresql's implementation of PersistDbSpecific. I thought this was not the case because the documentation shows an example of using it with PostGIS, however on second reading I think that implementation is broken because both I will try to demonstrate that this is broken by adding a test. |
So, having a Using it for computed columns is very, very, very interesting, but I think we need to be cautious about exactly how it is used.
The embedded
Which means we get
will result in the following record in the database:
which is really not what we want. |
My first instinct is the following workaround: class PersistField a where
toPersistValue :: a -> PersistValue
fromPersistValue :: a -> Either Text PersistValue
insertPersistValue :: a -> PersistValue
insertPersistValue = toPersistValue So users could provide their own |
With the above class definition, we'd change newtype ComputedColumn a = ComputedColumn a
instance PersistField a => PersistField (ComputedColumn a) where
toPersistValue (ComputedColumn x) = toPersistValue x
fromPersistValue x = ComputedColumn <$> fromPersistValue x
insertPersistValue _ = PersistLiteral "DEFAULT"
instance PersistFieldSql a => PersistFieldSql (ComputedColumn a) where
sqlType _ = sqlType (Proxy @a)
newtype Timestamp = Timestamp UTCTime
instance PersistField Timestamp where
toPersistValue (Timestamp x) = toPersistValue x
fromPersistValue x = Timestamp <$> fromPersistValue x
insertPersistValue _ = PersistLiteral "now()"
instance PersistFieldSql Timestamp where
sqlType _ = sqlType (Proxy @UTCTime) |
The thing with tracking I don't think that adding an additional responsibility to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach a lot. Just needs changelog entries in the relevant spots.
|
||
instance forall a. PersistField a => PersistField (NullableGenerated a) where | ||
toPersistValue (NullableGenerated _) = PersistLiteral "DEFAULT" | ||
fromPersistValue g = NullableGenerated <$> fromPersistValue g |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a useful pattern for describing how this works but I really can't recommend using it in app code. Having it present in the tests is a bit worrying to me, as well, as other people might mimic it and then come across bugs for it.
Travis is failing because that version of Postgres doesn't support GENERATED ,looks like. We can either upgrade Travis or not use GENERATED in the schema - slight preference to drop GENERATED since that'd make it easier to run tests locally too for older OSes. |
I've made a PR that will add GENERATED column support, if Travis accepts it: #1141 Once that has landed, if you merge |
Actually, I have switched CI to GitHub Actions with postgres 12 installed. Can you merge |
@@ -377,6 +377,7 @@ data PersistValue = PersistText Text | |||
| PersistMap [(Text, PersistValue)] | |||
| PersistObjectId ByteString -- ^ Intended especially for MongoDB backend | |||
| PersistArray [PersistValue] -- ^ Intended especially for PostgreSQL backend for text arrays | |||
| PersistLiteral ByteString -- ^ Using 'PersistLiteral' you can customize PersistField instances to output unescaped SQL | |||
| PersistDbSpecific ByteString -- ^ Using 'PersistDbSpecific' allows you to use types specific to a particular backend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, here's a migration plan that can add this feature safely and manage #1123
PersistLiteral
is introduced as the "unescaping" variant.- We add a constructor
PersistLiteralEscaped
that has the escaping behavior consistently across DB packages. - Add a deprecation notice to
PersistDbSpecific
specifying what's changed and how. Mention that it will be removed in the next major version release of the library. This warning should percolate through the ecosystem and packages will update.
Then, in the next major version, we can make the behavior breaking changes in the relevant database packages, unifying how postgres/mysql/sqlite use these constructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add.
@parsonsmatt I'm working on getting it up to date and making sure all the tests pass. Will merge when finished. Thanks! |
@parsonsmatt It looks like I'm not going to be able to get this to work with Sqlite in its current state. We can do a few things: (1) merge and just say Sqlite generated columns are not supported [i'm not too keen on that one], (2) rework the feature, probably adding a field to |
2.11 is going to be a major release and so additional fields on |
Yeah, we're using this So, we'll probably want to keep |
25fadc6
to
5e9fc69
Compare
I pushed my latest working tree, but it won't get past CI. You'll find |
The failed test can be fixed in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few style suggestions but I'm otherwise happy to accept the PR for inclusion with 2.11 release.
| Just x <- T.stripPrefix "maxlen=" raw -> case reads (T.unpack x) of | ||
[(n, s)] | all isSpace s -> FieldAttrMaxlen n | ||
_ -> error $ "Could not parse maxlen field with value " <> show raw | ||
| otherwise -> FieldAttrOther raw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a case
expression.
fmap $ \case
"Maybe" -> FieldAttrMaybe
"nullable" -> FieldAttrNullable
-- etc
raw ->
asum
[ FieldAttrRefrence <$> T.stripPrefix "reference=" raw
, FieldAttrConstraint <$> T.stripPrefix "constraint=" raw
-- etc...
]
Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>
Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>
Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>
Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>
Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>
Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>
Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>
in Postgresql and MySQL backends
The MySQL test suite passes on my machine but fail on CI. I thought it might be a version issue, but it looks like CI is using the latest major version, just like I am. I am investigating. |
-- and serialization in backend-specific ways. | ||
-- | ||
-- While we endeavor to, we can't forsee all use cases for all backends, | ||
-- and so 'FieldAttr' is extensible through its constructor 'FieldAttrOther'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
@@ -882,7 +907,7 @@ findAlters edef allDefs col@(Column name isNull type_ def _defConstraintName max | |||
-- | Prints the part of a @CREATE TABLE@ statement about a given | |||
-- column. | |||
showColumn :: Column -> String | |||
showColumn (Column n nu t def _defConstraintName maxLen ref) = concat | |||
showColumn (Column n nu t def _gen _defConstraintName maxLen ref) = concat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is currently failing on MySQL because the column isn't being generated. Using _gen
here should fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.0
Maybe a merge conflict X(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect what's going on locally is that, your database was setup to have the GENERATED from a prior commit, and then this stuff ignores the GENERATED
expression when making migrations, so it doesn't un-generate it. So your local code isn't migrating away from GENERATED, and it's already GENERATED, so it passes the test. But in CI we're making the migration from scratch, so it doesn't get GENERATED.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to scrub the database after each test...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and then this stuff ignores the GENERATED expression when making migrations, so it doesn't un-generate it.
That seems like a grave oversight on my part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do migrations currently handle a field that goes from having a default to not having a default? Nevermind, I found it.
Okay, so. This patch is a lot different from what I originally described (#1122 (comment)). The original didn't work because column generation turned out to be too backend-specific (e.g. still haven't gotten Sqlite working), and because the original plan would have broke things like PS: This should get us automatic |
I think we can support this in SQLite without too much fuss. Here's the definition of insert val = do
conn <- ask
let esql = connInsertSql conn t vals
key <-
case esql of
ISRSingle sql -> withRawQuery sql vals $ do
x <- CL.head
case x of
Just [PersistInt64 i] -> case keyFromValues [PersistInt64 i] of
Left err -> error $ "SQL insert: keyFromValues: PersistInt64 " `mappend` show i `mappend` " " `mappend` unpack err
Right k -> return k
Nothing -> error $ "SQL insert did not return a result giving the generated ID"
Just vals' -> case keyFromValues vals' of
Left e -> error $ "Invalid result from a SQL insert, got: " ++ show vals' ++ ". Error was: " ++ unpack e
Right k -> return k
ISRInsertGet sql1 sql2 -> do
rawExecute sql1 vals
withRawQuery sql2 [] $ do
mm <- CL.head
let m = maybe
(Left $ "No results from ISRInsertGet: " `mappend` tshow (sql1, sql2))
Right mm
-- TODO: figure out something better for MySQL
let convert x =
case x of
[PersistByteString i] -> case readInteger i of -- mssql
Just (ret,"") -> [PersistInt64 $ fromIntegral ret]
_ -> x
_ -> x
-- Yes, it's just <|>. Older bases don't have the
-- instance for Either.
onLeft Left{} x = x
onLeft x _ = x
case m >>= (\x -> keyFromValues x `onLeft` keyFromValues (convert x)) of
Right k -> return k
Left err -> throw $ "ISRInsertGet: keyFromValues failed: " `mappend` err
ISRManyKeys sql fs -> do
rawExecute sql vals
case entityPrimary t of
Nothing -> error $ "ISRManyKeys is used when Primary is defined " ++ show sql
Just pdef ->
let pks = map fieldHaskell $ compositeFields pdef
keyvals = map snd $ filter (\(a, _) -> let ret=isJust (find (== a) pks) in ret) $ zip (map fieldHaskell $ entityFields t) fs
in case keyFromValues keyvals of
Right k -> return k
Left e -> error $ "ISRManyKeys: unexpected keyvals result: " `mappend` unpack e
return key
where
tshow :: Show a => a -> Text
tshow = T.pack . show
throw = liftIO . throwIO . userError . T.unpack
t = entityDef $ Just val
vals = map toPersistValue $ toPersistFields val Next up we have SQLite's insertSql' :: EntityDef -> [PersistValue] -> InsertSqlResult
insertSql' ent vals =
case entityPrimary ent of
Just _ ->
ISRManyKeys sql vals
where sql = T.concat
[ "INSERT INTO "
, escape $ entityDB ent
, "("
, T.intercalate "," $ map (escape . fieldDB) $ entityFields ent
, ") VALUES("
, T.intercalate "," (map (const "?") $ entityFields ent)
, ")"
]
Nothing ->
ISRInsertGet ins sel
where
sel = T.concat
[ "SELECT "
, escape $ fieldDB (entityId ent)
, " FROM "
, escape $ entityDB ent
, " WHERE _ROWID_=last_insert_rowid()"
]
ins = T.concat
[ "INSERT INTO "
, escape $ entityDB ent
, if null (entityFields ent)
then " VALUES(null)"
else T.concat
[ "("
, T.intercalate "," $ map (escape . fieldDB) $ entityFields ent
, ") VALUES("
, T.intercalate "," (map (const "?") $ entityFields ent)
, ")"
]
] The logic kinda ping-pongs here, so it can be easy to get lost.
So we have three touch points:
data SomePersistField = forall a. PersistField a => SomePersistField a
data SomePersistField where
SomePersistField :: (PersistEntity rec, PersistField a) => EntityField rec a -> a -> SomePersistField That gives us: toInsertVals :: (PersistEntity rec) => rec -> [PersistValue]
toInsertVals = map toPersistValue . filter p . toPersistFields
where
p (SomePersistField efield val) = not (isGenerated (persistFieldDef efield)) Anyway I'm just thinking out loud, I'll write this out myself since it touches the code-gen :) Thanks so much for all the great work! |
Nice explanation! I had written Thanks :-) |
How did this compile? persistent-zookeeper/Database/Persist/Zookeeper/Binary.hs It has underscores left in it... |
Thanks for reminding me, I forgot about #875 😅 |
When mapping to a generated column such as the following, the
toPersistValue
must outputDEFAULT
, without quotes.Currently,
PersistDbSpecific
will quote things it outputs (in persistent-postgresql), but an insert of'DEFAULT'
would be rejected by Postgres.This PR adds support for a
PersistLiteral
constructor onPersistValue
, meant to explicitly never escape its output in any database system. I've added an implementation for MySQL, PostgreSQL, and sqlite, as well as a test that uses a schema with a GENERATED column. My colleague @friedbrice said he would be happy to look into implementing this for the other db systems that I haven't added support for.A note on tests:
GENERATED
columns are supported in MySQL, PostgreSQL, and sqlite. While sqlite's documentation says its available from 3.31.0, my testing shows that it does not actually work in 3.31.0:sqlite version testing output
does not work with current sqlite 3.31.0
works with newer sqlite 3.33.0
If the sqlite.c "amalgamation" were to be updated to the latest 3.33.0, the test for sqlite will work. Currently it fails. Something need to be done about that and I would appreciate suggestions.
After submitting your PR:
(unreleased)
on the Changelog