hamiltop / rethinkdb-elixir

Rethinkdb client in pure elixir (JSON protocol)
MIT License
497 stars 64 forks source link

Should we support Structs? #45

Open hamiltop opened 9 years ago

hamiltop commented 9 years ago

Right now, we support Maps, Lists and values in our data. What should happen if someone does:

table("people") |> insert(%Person{name: "Joe"}) |> run conn

Options are:

  1. Treat it as a map. Downside: when we pull it back out of the database we won't be able to parse it into a map.
  2. Mirror Poison's approach. Allow an as: Foo option to run that will parse the data into the correct struct.
  3. Don't allow structs at all. You must convert it to a map before inserting.
  4. Builder Pattern version of number 2 above. In the module where you define a struct, call use RethinkDB.StructSupport and it will define a Foo.parse that will set a flag in the %Query{} with the struct type that should be parsed.

I feel like 3 > 1 on grounds of explicit > implicit.

I feel like 4 requires 2 and is just a fancier syntax.

I think a combination of 2 and 4 should fit the pattern of everything else we're building. We provide the generic function and also provide a fancy macro solution to make things cleaner.

vorce commented 9 years ago

I like the nr 2 approach

clessg commented 9 years ago

Struct support would be very useful.

hamiltop commented 9 years ago

@clessg Could you give me a few examples of how you might use them?

clessg commented 9 years ago

Well, I'm new to Elixir, so I imagine having a User struct, for example.

But I suppose that structs are basically maps with a couple extra features, so there's no reason I can't just have a User module with functions that act on those maps. Would lose out on default attributes and pattern matching but it's probably not a big deal.

I'm happy with whatever is decided on!

hamiltop commented 9 years ago

So it sound like you would like to be able to insert a struct. That's easy to do. You could either call Map.from_strict and then insert it or we can make this library do that automatically.

How about reading from the db? What would you expect to do there? While struct(User, data) works in theory, in practice keys in a rethinkDB response are strings, not atoms, and would be filtered out. So something to solve that would be handy.

On Thu, Sep 17, 2015, 4:54 PM Chris Gaudreau notifications@github.com wrote:

Well, I'm new to Elixir, so I imagine having a User struct, for example.

But I suppose that structs are basically maps with a couple extra features, so there's no reason I can't just have a User module with functions that act on those maps. Would lose out on default attributes and pattern matching but it's probably not a big deal.

I'm happy with whatever is decided on!

— Reply to this email directly or view it on GitHub https://github.com/hamiltop/rethinkdb-elixir/issues/45#issuecomment-141288138 .

clessg commented 9 years ago

Oh, yeah, you're right about that apparently. In that case, I think a way to serialize to and from a struct would be a good idea. I'm still new to Elixir so I don't really know what would be the most idiomatic way to do so, though!

hamiltop commented 9 years ago

How about a syntax like:

table("users") |> as_struct(User) |> run

What would it return?

Option 1:

%RethinkDB.Collection{data: [%User{id: ...}, %User{id: ...}])

Option 2:

[%User{id: ...}, %User{id: ...}]

Alternatively, the as_struct call could be moved to after run:

table("users") |> run |> as_struct(User)

That way, we don't need to change the semantics and return value of run.

Thoughts?

clessg commented 9 years ago

My understanding is that the stuff before run is typically part of a query sent to the database (?), so my initial inclination was to have the as_struct call after run.

But I'm not sure what would happen in the case of an error in the query. Also, it might be easier to perform optimizations with as_struct before run.

hamiltop commented 9 years ago

That's a good point. I could probably allow it anywhere in the query as well as after run.

As far as inserting structs, any reason not to automatically call Map.from_struct ?

On Fri, Sep 18, 2015, 6:31 AM Chris Gaudreau notifications@github.com wrote:

My understanding is that the stuff before run is typically part of a query sent to the database (?), so my initial inclination was to have the as_struct call after run.

But I'm not sure what would happen in the case of an error in the query. Also, it might be easier to perform optimizations with as_struct before run.

— Reply to this email directly or view it on GitHub https://github.com/hamiltop/rethinkdb-elixir/issues/45#issuecomment-141452279 .

clessg commented 9 years ago

I see no reason not to have Map.from_struct called automatically, other than some potential initial confusion, but I doubt it's a big deal.

hamiltop commented 9 years ago

Another question around as_struct will be what to do when an unknown key is encountered. Should it error? or ignore it?

clessg commented 9 years ago

I think ignoring it makes the most sense. That's certainly what I'm used to in other languages, but Erlang/Elixir is definitely its own beast.

hamiltop commented 9 years ago

I think I'm convinced that Ecto query building doesn't make sense for this driver. However, I think validations and other aspects of Ecto will be useful. I think this issue ties in with that.

Perhaps a simple use RethinkDB.Model might be useful, which defines CRUD for the struct and auto parses it.

hamiltop commented 8 years ago

I think I'm going to add a protocol for RethinkDB.Encoder. It will include an encode and a decode function. It will also include a derive macro. This enables a few useful things:

1) cleans up internal code around handling arrays, functions, etc. 2) allows user to specify how to handle structs while providing sensible default via derive 3) an implementation for Changeset can be included in a rethinkdb_ecto.

Encode will be called by default, but decode will still need some user input about what to decode to. I'm still not sure about how to do a List of Structs. Maybe two functions, result_as and each result_as, where the latter expects the result to be a List and will decode each. A corner case that could be supported in result_as might be [TypeA, TypeB, TypeC] which would strictly mean expect a 3 element list and decode each element into corresponding types. It's a weird case, so probably not worth building. Alternatively we can just do result_as and if the return type is a collection we would apply decode to each element.

result_as can come before or after run in a query, but will execute after the result is returned.

Going to put this up in a branch.

tlvenn commented 8 years ago

As any though been given to create a project on top of this one to provide all the goodies so to speak that avoid some boilerplates ? In my opinion, the driver should try keep its focus rather narrow and its perimeter should be well defined.

Ideally, the project on top of the driver should provide:

When you look at the feature list, it's very similar to Ecto so I understand the desire to have an adapter for it. It seems however that a dedicated query DSL would be needed to properly leverage RethinkDB.

hamiltop commented 8 years ago

I'm currently exploring Ecto without Ecto.Query, will report on findings.

On Thu, Nov 5, 2015 at 1:23 PM Tlvenn notifications@github.com wrote:

As any though been given to create a project on top of this one to provide all the goodies so to speak that avoid some boilerplates ? In my opinion, the driver should try keep its focus rather narrow and its perimeter should be well defined.

Ideally, the project on top of the driver should provide:

  • Struct support (Schema definition with validation and constraints, similiar to Ecto.Schema & Ecto.Changeset)
  • Automatically create tables and indexes.
  • Associations (hasOne, belongsTo, hasMany and hasAndBelongsToMany)
  • Life-cycle callbacks
  • Query API must be composable and allow to tap into rethinkdb driver api seamlessly

When you look at the feature list, it's very similar to Ecto so I understand the desire to have an adapter for it. It seems however that a dedicated query DSL would be needed to properly leverage RethinkDB.

— Reply to this email directly or view it on GitHub https://github.com/hamiltop/rethinkdb-elixir/issues/45#issuecomment-154196828 .

tlvenn commented 8 years ago

Great to hear ! :+1:

hamiltop commented 8 years ago

@Tlvenn Can you take a bit of time and play with https://github.com/hamiltop/rethinkdb_ecto ? It's super minimal, but I think it accomplishes what we want. It should support all the callbacks and validation goodness you want.

tlvenn commented 8 years ago

Hi @hamiltop, sorry for the late feedback.

Thanks for exploring this and I will definitely play with it soon. Regarding the feature set, when I look at what is left there, I think there is still some value that totally apply to Rethinkdb.

I started to use rethinkdb from node.js land, and neumino did a fantastic job creating a very performant driver for it and then on top of it, a light very useful ORM (https://thinky.io/) which gets the hassle and ceremony out of the way and yet when you need it, allow you to harness all the power of RethinkDB.

I believe Ecto could be a very valuable platform to do that for RethinkDB given that we find a way to elegantly still have access to Reql should we need it.

In any case, thanks again, will play with rethinkdb_ecto and I will report back.

hamiltop commented 8 years ago

Migrations are definitely necessary. That can be handled without Ecto.Query.

ReQL is pretty composable itself, though a little bit of capture/curry magic is sometimes necessary.

Thinky looks like a great reference. I'm a little concerned they created a while new ORM instead of hooking into an existing ORM. The analogous approach in Elixir would be to not use Ecto and build something custom tailored. I think I'll reach out to the Thinky creators and get their take, as I'm sure they considered the possibility of using a more general ORM.

Joins/associations are an area of RethinkDB I'm lacking experience in. It's also one of the areas I feel Ecto nailed (specifically forcing eager loading). I'll have to play with it a bit.

Overall, I have some PTSD from ActiveRecord that keeps me from blindly jumping into the Ecto boat. In Rails apps I've worked on, I've pretty much converted anything significant over to raw sql for performance and maintenance reasons. I'm probably biased by those experiences, so I greatly appreciate my view being challenged.

On Thu, Nov 12, 2015, 02:49 Tlvenn notifications@github.com wrote:

Hi @hamiltop https://github.com/hamiltop, sorry for the late feedback.

Thanks for exploring this and I will definitely play with it soon. Regarding the feature set, when I look at what is left there, I think there is still some value that totally apply to Rethinkdb.

-

Repo abstraction is interesting and useful concept.

Migration is very important, just because we use a schemaless DB does not mean we dont have to migrate our data when we decide to change the shape of our documents and we do have to create our tables and indexes.

Associations is very handy and can only be leveraged by a query DSL, using Reql to do that all the time is tedious especially when you have multiple joins going on.

The query API of Ecto is really nice to work with, the fact that it is composable is just awesome and so much to the core of Elixir and what FP is all about. I dont think Reql is so friendly in that regard and when you look at what the Ecto Query API, I bet it would address 90% of the use cases. The important part imho, is that for the remaining 10% you can tap into Reql specifically if you need to.

I started to use rethinkdb from node.js land, and neumino did a fantastic job creating a very performant driver for it and then on top of it, a light very useful ORM (https://thinky.io/) which gets the hassle and ceremony out of the way and yet when you need it, allow you to harness all the power of RethinkDB.

I believe Ecto could be a very valuable platform to do that for RethinkDB given that we find a way to elegantly still have access to Reql should we need it.

In any case, thanks again, will play with rethinkdb_ecto and I will report back.

— Reply to this email directly or view it on GitHub https://github.com/hamiltop/rethinkdb-elixir/issues/45#issuecomment-156070485 .

hamiltop commented 8 years ago

I explored migrations in Ecto and it looks like we have to support Ecto.Query for managing the schema_versions table. Everything else we can handle using the shim.

I reached out to the author of Thinky and he agrees that shoehorning ReQL into SQL is a waste and will cause confusion.

I think I'm close to satisfied with the idea of providing a RethinkDB.Ecto.Repo as a drop in replacement for Ecto.Repo. Just a few warts to sort out.

Anyone interested in becoming actively involved, this is a huge opportunity. I'd love a buddy on this.

AdamBrodzinski commented 8 years ago

Sounds good to me! I wish I had extra time to help, and also my Elxir is prob. not that good yet :smile:

I'd be more than happy to help out with testing. I'm using this in a slow moving greenfield app so i'm not too worried about the stability (worst case I could always fork it dev stops).

Changesets are the biggest feature for me! Such a great idea!

willykaram commented 8 years ago

@hamiltop I'm very interested in becoming actively involved and being your 'wingman' on this. Based on your presentation, I think that we share a similar interest in helping make the integration between RethinkDB and Elixir/Phoenix as solid as possible. If you think it makes sense, I can start with some documentation, testing and examples? Then I'll aim to gradually get more actively involved from there.

hamiltop commented 8 years ago

@willykaram reach out to me via email. I'd love the help!