Add new Postgresql.JSON.Experimental #283

belevy · 2021-09-12T15:06:26Z

Add JsonValue newtype wrapper to support extracting Json values from sql queries
- Added instances for SqlSelect/ToAlias*/ToMaybe
Add new PgToJsonb typeclass to support toJsonb function on Value, Entity, and tuples therof (up to 8)

Before submitting your PR, check that you've:

Bumped the version number.
Documented new APIs with Haddock markup.
Added @since declarations to the Haddock.
Ran stylish-haskell and otherwise adhered to the style guide.

After submitting your PR:

Update the Changelog.md file with a link to your PR.
Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts).

belevy · 2021-11-03T14:35:10Z

@parsonsmatt when you get a chance, would you mind taking a look at this. I added the typeclasses with the intention being to get as much reuse between MySQL and postgres as possible but then MySQL json support is still working it's way through the underlying libraries.

Vlix · 2021-11-24T15:29:49Z

src/Database/Esqueleto/Internal/Internal.hs

+viewFieldBuilder :: SqlExpr (Entity val) -> IdentInfo -> FieldDef -> TLB.Builder
+viewFieldBuilder (ERaw m f) info fieldDef 
+    | Just alias <- sqlExprMetaAlias m =
+        fst (f Never info) <> ("." <> useIdent info (aliasedEntityColumnIdent alias fieldDef))


Isn't fst (f Never info) literally sourceIdent info?
Maybe define sourceIdent as fst (f Never info) and use in both places?

a better question, does this branch ever differ from the otherwise branch? both branches appear to produce identical results for the Just alias case.

Ah, very true. fieldIdent's Just case will never get triggered 🙃 (And is indeed the same)

src/Database/Esqueleto/Internal/Internal.hs

parsonsmatt

Great ideas but we gotta nail down the details. Some of this is pretty tricky stuff to get really right.

Since there are changes to types of exposed functions, this is a breaking change, so we can slot it in with 3.6

parsonsmatt · 2022-01-29T02:32:51Z

src/Database/Esqueleto/Internal/Internal.hs

+-- | (Internal) Coerce a SqlExpr from any arbitrary a to any arbitrary b 
+-- You should /not/ use this function unless you know what you're doing!
+veryVeryUnsafeCoerceSqlExpr :: SqlExpr a -> SqlExpr b
+veryVeryUnsafeCoerceSqlExpr = coerce


wait we have a Coercible instance on SqlExpr? That's no good :|

parsonsmatt · 2022-03-10T14:29:40Z

src/Database/Esqueleto/Internal/JSON.hs

+        where
+            ed = entityDef $ Proxy @a
+            baseFields = fmap (\fieldDef ->
+                ( unsafeSqlValue $ TLB.fromText $ "'" <> unFieldNameHS (fieldHaskell fieldDef) <> "'"


This doesn't seem right. We're using the Haskell names for the database representation of the Entity, which isn't going to be how the database wants to handle the JSON object.

This is one of the big problems I ran into with trying to have jsonb_agg on Entity a. The {To,From}JSON instances don't work out. So I ended up writing a newtype JSONViaPostgres a that would dig into the EntityDef and figure out how to decode the Postgres JSON. But that also requires that you have a more-or-less standardly defined JSON instance for the record in question.

Does persistent support a way to read the to/from json field name given a field?

We can sidestep the issue of to/from by using a custom decoder. As long as we are consistent at the boundaries we could even avoid the need to specify JSON instances for the types.

Am I right when I say this SqlToJSON instance tries to put all the columns in their own field of a JSON object? (Just checking if I'm reading this right)

Correct, the goal being to make something that automatically just works with the default JSON parser that gets generated.

which isn't going to be how the database wants to handle the JSON object.

@parsonsmatt I am not actually clear on what you mean by this? We aren't letting postgres do its default conversion.

Yeah, I think I'm unsure that this is the right approach for converting something to a JSON representation.

I think I'd rather see something like:

newtype JsonEntity a = JsonEntity (Entity a) instance PersistEntity a => FromJSON (JsonEntity a) where parseJSON = withObject "JsonEntity" $ \o -> do let edef = entityDef (Proxy @a) ...

which definitely parses how a database would convert things to JSON. Then we aren't having to worry about custom encoders or decoders.

persistent-2.14's tabulateEntityA could be useful here, but we may also need the FieldDict class from prairie to make it work how we want.

This could work sure, if we used a more general newtype SqlJson a = SqlJson a we can have it default to the underlying FromJSON and avoid surfacing the actual newtype to the end user though. The entity instance could then be defined as overlapping that default.

So there isn't really a great way to do this in general without that FieldDict class since we need to know that the value on the other side has a FromJSON dictionary.

Is there an example of someone using an exotic JSON parser? The thing that I need the default parser for is the underlying record not the Entity record since the entityIdFromJSON delegates to the FromJSON instance on the record.

It should be noted that using the QQ's json tag will actually only allow you to configure the outer encoder/decoder but the record its self is always just haskellName: someValue

I don't think it even has to be an exotic parse, just a different set of options. Like,

mkPersist sqlSettings [persistLowerCase| User name Text age Int |] deriveJSON defaultOptions ''User

This would expect the JSON record to have the type name prefixes. I don't think the haskellName has those. Any other modification to those options would break parsing the records from the database.

So I think we do need a newtype with a custom FromJSON instance, and we also need the FieldDict class. I'll see about re-exporting that from persistent and generating those instances.

parsonsmatt · 2022-03-10T14:30:25Z

src/Database/Esqueleto/Internal/JSON.hs

+                , ERaw noMeta $ \_ info -> (viewFieldBuilder ent info fieldDef, [])
+                )) (getEntityFields ed)
+            idField = fmap (\fieldDef ->
+                ( unsafeSqlValue "'id'"


This isn't necessarily proper, as you can specify the primary column name:

User Id UUID sql="my_user_id"

The actual field name in the SQL is irrelevant, it's the representation that FromJSON expects that matters. Entity assumes a key of id

Is it not possible to use the database names? 🤔 To me that sounds more robust/portable.

Though I guess if it only happens within one query, it doesn't really matter.

EDIT: Oh, that might also make it possible to get back easily using jsonb_to_record

Indeed the db names could be used and a custom JSON decoder could be used to extract the fields.

parsonsmatt · 2022-03-10T14:32:07Z

test/Common/Test/Models.hs

@@ -49,21 +49,21 @@ share [mkPersist sqlSettings, mkMigrate "migrateAll"] [persistUpperCase|
  YetAnother
    argh ShoopId

-  Person
+  Person json


To give this a real whirl we'd want to also customize the json options in MkPersistSettings (or whichever thing allows you to do that)

parsonsmatt · 2022-03-10T14:32:38Z

test/PostgreSQL/Test.hs

+                     , JSONE.multiset $ do
+                       posts <- Experimental.from $ table @BlogPost
+                       where_ $ posts ^. BlogPostAuthorId ==. person ^. PersonId
+                       pure ( posts
+                            , JSONE.multiset $ do
+                              comments <- Experimental.from $ table @Comment
+                              where_ $ comments ^. CommentBlog ==. posts ^. BlogPostId
+                              pure comments
+                            )


this is dope

Vlix · 2022-03-10T23:44:37Z

@belevy
I'm wondering if you couldn't just use the JSONB type from Database.Esqueleto.PostgreSQL.JSON? 🤔

And maybe also JSONBExpr? That way you can also use all the JSON operators while aggregating etc.
Would also provide free SqlSelect, ToMaybe, ToAlias instances, to name a few.

Vlix · 2022-03-10T23:30:12Z

src/Database/Esqueleto/Internal/JSON.hs

+        where
+            ed = entityDef $ Proxy @a
+            baseFields = fmap (\fieldDef ->
+                ( unsafeSqlValue $ TLB.fromText $ "'" <> unFieldNameHS (fieldHaskell fieldDef) <> "'"


Am I right when I say this SqlToJSON instance tries to put all the columns in their own field of a JSON object? (Just checking if I'm reading this right)

Vlix · 2022-03-10T23:34:15Z

src/Database/Esqueleto/Internal/JSON.hs

+                , ERaw noMeta $ \_ info -> (viewFieldBuilder ent info fieldDef, [])
+                )) (getEntityFields ed)
+            idField = fmap (\fieldDef ->
+                ( unsafeSqlValue "'id'"


Is it not possible to use the database names? 🤔 To me that sounds more robust/portable.

Though I guess if it only happens within one query, it doesn't really matter.

EDIT: Oh, that might also make it possible to get back easily using jsonb_to_record

Vlix · 2022-03-10T23:59:46Z

src/Database/Esqueleto/Internal/JSON.hs

+class JsonBuildArray jsonValue where
+    unsafeJsonbBuildArray :: UnsafeSqlFunctionArgument a => a -> SqlExpr (jsonValue b)
+
+class JsonBuildObject jsonValue where


Are the jsonValue type variables all here in these class definitions so this can later be ported over to the JSONB type, because this is considered experimental at the moment so you want to keep it separate?

MySQL supports JSON as well. The intention is to add support in a db agnostic manner with as much independent of db specifics as possible

parsonsmatt

Yeah, my only quibble here is over returning stuff that isn't going to parse how we want. We should expect people to be doing interesting things with their FromJSON instances and I don't think we should require that the JSON is in any particular shape that we can't control ourselves.

Committing to a JSON format for this stuff is going to make changing that format and/or testing regressions difficult.

parsonsmatt · 2022-03-14T19:29:35Z

src/Database/Esqueleto/Internal/JSON.hs

+        where
+            ed = entityDef $ Proxy @a
+            baseFields = fmap (\fieldDef ->
+                ( unsafeSqlValue $ TLB.fromText $ "'" <> unFieldNameHS (fieldHaskell fieldDef) <> "'"


Ohhh, I see what's going on. We're explicitly unwrapping the table and making the (key, value) pair ourselves in this. Neat!

Still, if there's a custom To/FromJSON instance, then it may not serialize or deserialize correctly. I wonder if there's a good way we can ensure we're puling it out in the wa we'd expecy based on the EntityDef 🤔

parsonsmatt · 2022-04-12T14:04:14Z

src/Database/Esqueleto/Internal/JSON.hs

+        where
+            ed = entityDef $ Proxy @a
+            baseFields = fmap (\fieldDef ->
+                ( unsafeSqlValue $ TLB.fromText $ "'" <> unFieldNameHS (fieldHaskell fieldDef) <> "'"


Yeah, I think I'm unsure that this is the right approach for converting something to a JSON representation.

I think I'd rather see something like:

newtype JsonEntity a = JsonEntity (Entity a) instance PersistEntity a => FromJSON (JsonEntity a) where parseJSON = withObject "JsonEntity" $ \o -> do let edef = entityDef (Proxy @a) ...

which definitely parses how a database would convert things to JSON. Then we aren't having to worry about custom encoders or decoders.

persistent-2.14's tabulateEntityA could be useful here, but we may also need the FieldDict class from prairie to make it work how we want.

…nb on newtype JsonValue. Also added multiset and multisetAgg convienience functions

… type

Vlix · 2023-10-20T07:10:22Z

src/Database/Esqueleto/Internal/JSON.hs

+sqlSelectProcessRowJSON :: (Applicative f, Aeson.FromJSON r)
+                        => [PersistValue] -> Either Text (f r)
+sqlSelectProcessRowJSON [PersistByteString bs] =
+    case Aeson.eitherDecode $ LBS.fromStrict bs of


No need to import Data.ByteString.Lazy, aeson's got your back:

either Text.pack pure $ Aeson.eitherDecodeStrict bs

Vlix · 2023-10-20T07:11:06Z

I still don't see why the JSONB newtype isn't used?

Move the JSONB newtype to Database.Esqueleto.Internal.JSON
Re-export it in Database.Esqueleto.PostgreSQL.JSON
Use it in Database.Esqueleto.PostgreSQL.JSON.Experimental
Be able to also use the JSON operators from Database.Esqueleto.PostgreSQL.JSON in values you get when using Database.Esqueleto.PostgreSQL.JSON.Experimental
???
profit?

belevy · 2023-12-26T20:11:40Z

@Vlix that would require anyone using the JSONB type to agree on how it works. If there are any inconsistencies between different implementations then the type would need to be split then. As for reusing the type internally with postgres, it's possible but would require ensuring that the existing operators all work as expected there.

belevy requested a review from parsonsmatt September 12, 2021 15:06

belevy marked this pull request as draft September 12, 2021 15:10

belevy marked this pull request as ready for review October 3, 2021 19:38

belevy changed the title ~~[WIP] Add new Postgresql.JSON.Experimental~~ Add new Postgresql.JSON.Experimental Nov 3, 2021

Vlix reviewed Nov 24, 2021

View reviewed changes

belevy commented Nov 28, 2021

View reviewed changes

src/Database/Esqueleto/Internal/Internal.hs Outdated Show resolved Hide resolved

parsonsmatt reviewed Mar 10, 2022

View reviewed changes

Vlix reviewed Mar 11, 2022

View reviewed changes

parsonsmatt added this to the 3.6 milestone Apr 12, 2022

parsonsmatt requested changes Apr 12, 2022

View reviewed changes

belevy mentioned this pull request Nov 29, 2022

Improve JSON support in Esqueleto.Postgresql #282

Open

belevy added 2 commits October 19, 2023 16:07

Add new Postgresql.JSON.Experimental that supports jsonbAgg and toJso…

67ab9c6

…nb on newtype JsonValue. Also added multiset and multisetAgg convienience functions

Add initial exemplary test

59ab459

belevy force-pushed the jsonagg-multiset-emulation branch from ac9cd12 to 1d2fc6d Compare October 19, 2023 21:11

belevy added 3 commits October 19, 2023 16:11

Add more tests for toJson. Fix bug with value toJson

82f484a

Extract postgres specific functionality into typeclass over jsonValue…

ab00454

… type

Simplify viewFieldBuilder

1d2fc6d

Vlix reviewed Oct 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new Postgresql.JSON.Experimental #283

Add new Postgresql.JSON.Experimental #283

belevy commented Sep 12, 2021

belevy commented Nov 3, 2021

Vlix Nov 24, 2021

belevy Nov 28, 2021

Vlix Nov 28, 2021

parsonsmatt left a comment

parsonsmatt Jan 29, 2022

parsonsmatt Mar 10, 2022

belevy Mar 10, 2022

Vlix Mar 10, 2022

belevy Mar 11, 2022

belevy Mar 14, 2022 •

edited

Loading

parsonsmatt Apr 12, 2022

belevy Apr 12, 2022

belevy Apr 14, 2022

belevy Apr 14, 2022

parsonsmatt Jun 2, 2022

parsonsmatt Mar 10, 2022

belevy Mar 10, 2022

Vlix Mar 10, 2022 •

edited

Loading

belevy Mar 11, 2022

parsonsmatt Mar 10, 2022

parsonsmatt Mar 10, 2022

Vlix commented Mar 10, 2022 •

edited

Loading

Vlix Mar 10, 2022

Vlix Mar 10, 2022 •

edited

Loading

Vlix Mar 10, 2022 •

edited

Loading

belevy Mar 11, 2022

parsonsmatt left a comment

parsonsmatt Mar 14, 2022

parsonsmatt Apr 12, 2022

Vlix Oct 20, 2023

Vlix commented Oct 20, 2023

belevy commented Dec 26, 2023

Add new Postgresql.JSON.Experimental #283

Are you sure you want to change the base?

Add new Postgresql.JSON.Experimental #283

Conversation

belevy commented Sep 12, 2021

belevy commented Nov 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parsonsmatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belevy Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vlix Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vlix commented Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Vlix Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Vlix Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parsonsmatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vlix commented Oct 20, 2023

belevy commented Dec 26, 2023

belevy Mar 14, 2022 •

edited

Loading

Vlix Mar 10, 2022 •

edited

Loading

Vlix commented Mar 10, 2022 •

edited

Loading

Vlix Mar 10, 2022 •

edited

Loading

Vlix Mar 10, 2022 •

edited

Loading