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

PostgreSQL JSON Operators #128

Merged
merged 36 commits into from Aug 5, 2019

Conversation

@Vlix
Copy link
Contributor

commented Jul 29, 2019

Added all JSON operators in the Database.Esqueleto.PostgreSQL.JSON module and wrote documentation and tests for all operators.

Also increased version to 3.1.0

Copy link
Collaborator

left a comment

I see that you're still working on this, so I'll complete the review later. Leave a comment when you're done and I'll be able to get back to it.

So far this is one of the nicest and most comprehensive PRs I've had the pleasure to review in my OSS time. Thanks so much!

src/Database/Esqueleto/PostgreSQL/JSON.hs Outdated Show resolved Hide resolved
(e.g. @'[1,2,3]' -> 1 \@> 2@)
/The PostgreSQL version the functions work with are included/
/in their description./

This comment has been minimized.

Copy link
@parsonsmatt

parsonsmatt Jul 29, 2019

Collaborator

This documentation is lovely so far!

This comment has been minimized.

Copy link
@Vlix

Vlix Jul 29, 2019

Author Contributor

😳 You make me blush

-- ? | text | Does the string exist as a top-level key within the JSON value? | '{"a":1, "b":2}'::jsonb ? 'b'
-- ?| | text[] | Do any of these array strings exist as top-level keys? | '{"a":1, "b":2, "c":3}'::jsonb ?| array['b', 'c']
-- ?& | text[] | Do all of these array strings exist as top-level keys? | '["a", "b"]'::jsonb ?& array['a', 'b']
-- @

This comment has been minimized.

Copy link
@parsonsmatt

parsonsmatt Jul 29, 2019

Collaborator

🙌

@Vlix

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Ugh... I can't get Travis to use PSQL 10 > .<

Any idea what's going on?

@parsonsmatt

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

Hmm, the docs are a little ambiguous, but I think you should remove the postgresql in services and just use the one in addons

Felix Paulusma added 2 commits Jul 29, 2019
@parsonsmatt

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

This is the example for using postgresql-10:

addons:
  postgresql: "10"
  apt:
    packages:
    - postgresql-10
    - postgresql-client-10

What you've got now is:

addons:
  postgresql: "10"
  apt:
    packages:
      - libgmp-dev
      - postgresql-client-10
      - postgresql-server-dev-all

Only thing I can see is postgresql-server-dev-all vs -postgresql-10

@Vlix

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Yeah, I'm not sure what the libqmp-dev or postgresql-server-dev-all is for... but I guess I'll keep them in?

Felix Paulusma added 2 commits Jul 29, 2019
@Vlix

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Seems PostgreSQ 10 runs on port 5433 by default?

Felix Paulusma added 4 commits Jul 29, 2019
…ting test conn to port 5433
Felix Paulusma
Felix Paulusma
@bitemyapp

This comment has been minimized.

Copy link
Owner

commented Jul 29, 2019

@Vlix this is looking great, I love the documentation!

@Vlix

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@Vlix this is looking great, I love the documentation!

Thanks! Not just for the compliment, but also for Haskell Programming: From First Principles. It's what really got me going a few years back :)

@parsonsmatt

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

I think there's a lot of value in the "raw" Value API. Safer APIs can be built on top of that relatively easily. SqlExpr (Value (Jsonb Value)) -> SqlExpr (Value (Jsonb yourType)) is just an veryUnsafeCoerceSqlExpr away 😂

Perhaps with Generics and OverloadedLabels we could have an API like (rec :: SqlExpr (Value (Jsonb MyType))) ->. #myTypeSomeField :: SqlExpr (Value (Jsonb SomeFieldType)), given something like

data MyType = MyType { myTypeSomeField :: SomeField }

data SomeField = SomeField { someFieldThing :: Int }

Not really relevant to this PR, but I would like to consider APIs that we can extend safely later on without invasive refactoring upstream.

SqlExpr (Maybe (Jsonb Value)) -> Text -> SqlExpr (Maybe (Jsonb Value)) is basically the same API in this PR, but it doesn't require an orphan instance, doesn't require dancing around package releases, doesn't break anything, and easily integrates with the Jsonb CustomType with some casts.

Using (ToJSON a, FromJSON a, ToJSON b, FromJSON b) => SqlExpr (Maybe (Jsonb a)) -> Text -> SqlExpr (Maybe (Jsonb b)) can easily be monomorphized to the above signature. I don't think that this signature would have awful type inference properties - the SqlExpr constructors are either going to be projecting a field from an entity (with a "known" result type) or using val with a "known" argument type. The result type may not be known, though, and that may need annotation.

However, the signature we use at Lumi is not restricted:

(|->?) :: SqlExpr (Value (Maybe (Jsonb a))) -> SqlExpr (Value Text) -> SqlExpr (Value (Maybe (Jsonb b)))
(|->?) = ((unsafeSqlFunction "jsonb_object_field") .) . (,)

This doesn't present any problems chaining. Maybe the constraints are unnecessary, except for when trying to deserialize the value.

@parsonsmatt

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

In my opinion, using Maybe Aeson.Value is the most honest representation, since most operators can arbitrarily change the structure of the JSON, and having that reflected in the types makes it obvious you need to be careful.

This is definitely true! But Esqueleto isn't trying to be a 100% honest representation of SQL - it tries to give best-of-both-worlds to SQL and Haskell by allowing you to use Haskell types as much as possible. The most "honest" representation for many Key entity type is Int64 but we have a richer language of types than SQL can support here.

@Vlix

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

That is very true. I'll try to add a newtype and see if any tests go haywire.

I'm wondering if the type inference will be fine with, e.g.:

jsonbVal :: a -> SqlExpr (Value (Maybe (JSONB a)))
jsonbVal = just . val . JSONB

v ^. JsonValue ->. Right "test" -. Left 1 @>. jsonbVal (Number 3)

The -. has no idea what type's coming in, and which is going out, right?

@Vlix

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Ok, seems like it wasn't too much trouble. Added some conveniences as well:

  • type synonym: JSONExpr
  • function: jsonbVal

Still not sure if we want to keep the raw Text / [Text] / Either Int Text...
Putting the Text in SqlExpr (Value (Maybe Text)) isn't such a big deal, but the other ones are a bit iffy.
The [Text] would only work out of a column if that column was a text[] (PostgreSQL text array type). And the 'Either Int Text' would have to be solved with, I think, a type class in the same style as SqlString?

Copy link

left a comment

This is looking very nice!

@Vlix

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@eborden
Do you have an opinion about the RHOs that aren't SqlExpr (Value a)?

@eborden

This comment has been minimized.

Copy link

commented Aug 1, 2019

@Vlix the general pattern in esqueleto up to this point has been to use open world assumptions and define a typeclass like SqlString. Following that pattern would align your RHO values with existing convention. I don't know if I'm particularly compelled by that option, but in some cases it does open up your strict structure to more options like Vector Text in place of [Text].

@parsonsmatt

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

My only issue with the polymorphic class JsonAccessor style is that I anticipate using literals with this somewhat often - indeed every use case of the ->. operator at Lumi uses a literal Text, and trying to use these would require a type annotation at every use site.

That's why I'm more OK with defining a somewhat dodgy

data JsonAccessor
  = JsonInt Integer
  | JsonText Text

instance Num JsonAccessor where
  fromInteger = JsonInt
  (+) = error "forgive me"

instance IsString JsonAccessor where
  fromString = JsonText . Text.pack

If you want the nice syntax, you can opt-in via numeric literals. If you want to use the constructors, they're right there - JsonText "hello" is IMO preferable to Right "hello". It would be a little annoying if you already have an Integer that you need to wrap it in a fromInteger or JsonInt call. Idk. We don't actually have any uses of the array access functions - we mostly tend to do a CROSS JOIN on array elements and treat them as tables.

@eborden

This comment has been minimized.

Copy link

commented Aug 1, 2019

Agreed that these will mostly be literal values.

@cdparks

This comment has been minimized.

Copy link

commented Aug 1, 2019

The concrete sum type works for me. I'd probably call them JsonIndex and JsonKey, but that's neither here nor there. Every version of the polymorphic version I can dream up feels like a lot of machinery for not much gain.

EDIT: dodgy Num instances make me uneasy, but I can live with them

@Vlix

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

I've taken your suggestions and implemented the JSONAccessor data type. Even with the dodgy Num instance. 🙃

Do you guys think anything's still missing? Or anything can still be improved upon?

@eborden
eborden approved these changes Aug 2, 2019
Copy link

left a comment

Copy link
Collaborator

left a comment

Hell yeah 😄

Felix Paulusma
)

-- | 'SqlExpr' of a NULL-able 'JSONB' value.
type JSONExpr a = SqlExpr (Value (Maybe (JSONB a)))

This comment has been minimized.

Copy link
@paf31

paf31 Aug 2, 2019

One small point - I understand why the Maybe is necessary here, but I find it a bit strange that JSONExpr is the name of the synonym for nullable JSONB values.

When I read (->.) :: JSONExpr a -> JSONAccessor -> JSONExpr b, I don't see any immediate indication that anything there is nullable, and I have to look at the type synonym to see it.

I'd suggest changing the name to indicate the Maybe in there - perhaps:

-- | 'SqlExpr' of a NULL-able 'JSONB' value.
-- Note: The null here is a SQL null, not a JSON null.
type JSONMaybeExpr a = SqlExpr (Value (Maybe (JSONB a))) -- name isn't great

On a related note, I don't see any way to operate on non-nullable JSONB values. Is that by design? Presumably it would be quite hard to provide an API which dealt with both without a lot of duplication, and I suppose I can always use just on any exprs from non-nullable columns. Sorry, I see you covered this in your documentation already. I should learn to read the docs before the types.

Please don't consider this to be blocking at all.

This comment has been minimized.

Copy link
@paf31

paf31 Aug 2, 2019

On the subject of changing the name, maybe it would also be good to add the B to indicate this in JSONB, in case there is any chance of JSON types being added later.

This comment has been minimized.

Copy link
@Vlix

Vlix Aug 2, 2019

Author Contributor

I'd say, you'll notice when you use it, and you can always look at the type synonym.
If someone has a good suggestion on how to make this clearer, I can easily integrate it.

This comment has been minimized.

Copy link
@Vlix

Vlix Aug 2, 2019

Author Contributor

On the subject of changing the name, maybe it would also be good to add the B to indicate this in JSONB, in case there is any chance of JSON types being added later.

This is probably a good idea, so I'll adjust that.

Felix Paulusma
@paf31
paf31 approved these changes Aug 2, 2019
Copy link

left a comment

🎉 Looks great to me!

@cdparks
cdparks approved these changes Aug 2, 2019
Copy link

left a comment

👏

@parsonsmatt

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

Thanks for the awesome PR @Vlix !

@parsonsmatt parsonsmatt merged commit a452946 into bitemyapp:master Aug 5, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.