john-kelly / elm-postgrest

Make PostgREST requests in Elm
109 stars 14 forks source link

Non-trivial resource field codecs support #3

Closed danstn closed 7 years ago

danstn commented 7 years ago

Is it currently possible to provide different codecs for decoding and encoding?

The use-case is as follows:

Resource foo has a field email that could be null, so we'd like our Foo model to represent email as Maybe String. The resource and query definition look like this:

fooResource = 
  resourse "foo"
    { ...
    , email = field "email" <| Decode.maybe Decode.string
    , ...
    }

fooQuery = 
  query fooResource Foo
    |> ...
    |> select .email
    |> ...

fooRequest : List String -> Cmd (List Foo)
fooRequest emails =
  fooQuery
    |> filter [ .email |> in' emails ]
    |> ...

The problem is that if I try to apply a filter to the query (|> filter [ .email |> in' [<list of strings>]) the types don't line up, because the resource uses a Maybe String codec.

Is there a way to say which codec to use when making requests?

There is a high chance that I'm missing something fundamental though.

john-kelly commented 7 years ago

Interesting. I'll need to think about this a bit more. Definitely a valid/legitimate use case.

This issue may be related to postgREST casting -- which I have not yet implemented/thought about too extensively.

I'll take a look tonight.

danstn commented 7 years ago

Thanks John!

I'm not sure if implementing casting will solve the problem. There is another use-case when you have an enum in Postgres (i.e. TYPE gen AS ENUM('f', 'm')) which decodes to elm type as say:

type Gender = Male | Female

and represented as Maybe Gender to handle nulls. We need a way of changing the way url params are serialized (per type).

I was trying to make it work without having to leave the scope of coerceToString function (not sure it's possible without typeclasses, constraints or some kind of generics), but ended up hacking in a temporary work-around (see a commit referenced above) by just passing a modifier though.

Maybe it's the wrong extension point. Alternatively could change Field to be something like this:

type alias Encoder a 
    = a -> String

type Field a
    = Field String (Decode.Decoder a) (Encoder a)

Will have another go later today.

Keen to see the approach you take. :)

john-kelly commented 7 years ago

I like the idea of adding it to Field. It's pretty great b/c we can get rid of the hackyness of coerceToString.

Would this solve your use case?

fooResource = 
  resourse "foo"
    { ...
    , email = field (Decode.maybe Decode.string) myCustomToStringFunction "email"
    , ...
    }

fooQuery = 
  query fooResource Foo
    |> ...
    |> select .email
    |> ...

fooRequest : List String -> Cmd (List Foo)
fooRequest emails =
  fooQuery
    |> filter [ .email |> in' (List.map Just |> emails) ]
    |> ...

It still feels a bit weird that you have to (List.map Just |> emails).

Not sure yet, but I like our progress.


Random Brainstorm: Add another in'

-- no idea the name for it. 
-- maybeIn
-- inMaybe
inWithMaybe : List a -> (schema -> Field (Maybe a)) -> schema -> Filter

Then you would have:

fooRequest : List String -> Cmd (List Foo)
fooRequest emails =
  fooQuery
    |> filter [ .email |> inWithMaybe emails ]
    |> ...
danstn commented 7 years ago

Yeah, that's exactly what I landed on :) I have no problem with (List.map Just |> emails). Things like that can be easily cleaned up by passing correct types through.

Any particular reason you decided to swap the arguments order in Field?

john-kelly commented 7 years ago

Makes these a lot nicer https://github.com/john-kelly/elm-postgrest/commit/63c52e231f8e9eb3667658dd66745e153a264d1d#diff-12409062b2c3f8713b8133d11dd0cd35R161

Some helper/utility functions for fields.

Swapping allowed me to define those helpers with partial application

danstn commented 7 years ago

The internal implementation could use flip to keep the API nicer instead :)

Anyway - that's looking great! If you could roll out another version with this change at some point it'd be fantastic :beers:

john-kelly commented 7 years ago

That is a fair point.

email = field "email" (Decode.maybe Decode.string) myCustomToStringFunction

vs

email = field (Decode.maybe Decode.string) myCustomToStringFunction "email"

Edit Oct 19: However, I actually still think the latter is better than the former. It allows allows api users to do things like

maybeStringField = field (Decode.maybe Decode.string) maybeStringToString

-- which can then be used like so 
maybeStringField "email"
maybeStringField "name"
-- etc...

Anyways. I'll test this out a bit / think on it for a day and then get this live. Thanks for the help!

danstn commented 7 years ago

Good stuff! Thank you :+1:

john-kelly commented 7 years ago

Alright. So I was thinking about this a bit more last night -- this issue might be related to the changes needed to support writing.

Current necessary border control / serialization as i see it:

  1. Decoder : PostgREST Response / JSON -> Elm Types
    • has to do with decoding response; this is already implemented
  2. Encoder : Elm Types -> JSON / CSV (maybe?) / body of PostgREST Write Request
    • has to do with writing api; not yet implemented and/or considered
  3. ToString : Elm Types -> String / PostgREST specific query params
    • has to do with preparing query params for request; related to the current issue

Now my question, are the Encoder and ToString cases actually the same or at least related? It might be too early to say, and I am open to merging the current efforts to fix the current issue, but I just wanted to record some thoughts / get some input.

Additionally, as a side note, I come from a Django background and have used Django REST Framework. The term serializer explains how I am thinking about this border control of types and such. I'm not sure what other libraries call this, so I am interested if anyone has some input there.

danstn commented 7 years ago

Yes. I 100% agree that these are separate concerns and don't think that polluting Field type with the wrong Encoder (aka toString) is a good idea.

I think I'd prefer to keep the current Field type the same (otherwise implementing write will be confusing and API + docs will change in a non-obvious way multiple times) and instead see if there is an obvious way to provide Encoders for Query instead.

At the moment (if I understand it correctly) Query uses the same Decoder a from the Field. Maybe it should support passing a separate URL Encoder explicitly instead?

A note on 1. and 2. above: I was wondering if both Decoder (Json -> Elm) and Encoder (Elm -> Json) should be wrapped in a single type (i.e. FieldCodec or something). This way you can define Iso-like shaped product type with encode and decode functions attached which users have to supply Field with. Also, I agree on the order (field decode encode "foo").

john-kelly commented 7 years ago

I definitely see the type of Field looking like this:

type Field a
    = Field (Decoder a) (a -> String) (Encoder a) String

I'm just curious if having both (a -> String) for url encoding and then (Encoder a) for write request body encoding is redundant. I have a feeling that (a -> String) could be derived from (Encoder a), therefore, we would actually only need:

type Field a
    = Field (Decoder a) (Encoder a) String

However, there might be edge cases that make this the wrong choice.

In reference to your FieldCodec comment, I think that is the right abstraction / way to think about this, but I'm just not yet convinced on the need for expressing it in the types. It might make the internal api nicer, but I don't see a need to expose that to the users yet. It's already going to be easy enough to do:

maybeStringField = field (Decode.maybe Decode.string) maybeStringToString myEncoder

-- which can then be used like so 
maybeStringField "email"
maybeStringField "name"

Anyways, I think it makes sense to move forward with the current solution and figure this out when the time comes / we have more examples + info.

danstn commented 7 years ago

Sorry, misunderstood this:

Now my question, are the Encoder and ToString cases actually the same or at least related?

I get what you meant now :+1:

This looks great:

type Field a
    = Field (Decoder a) (a -> String) (Encoder a) String

Whether one encoder could be derived from another safely I'm not sure. Would be a nice breaking change to introduce later on I reckon :)

danstn commented 7 years ago

Thanks for the quick response @john-kelly :facepunch:

danstn commented 7 years ago

tag and publish v4? :)

john-kelly commented 7 years ago

tagged and published @danstn