rethinkdb-elixir
rethinkdb-elixir copied to clipboard
hamiltop
Suggestion: return an atom indicating the response rather than a struct
Right now when I get a query back I need to match on one of the structs RethinkDB.Response, RethinkDB.Collection or RethinkDB.Record. The problem I'm running into is a tad convoluted - but the basics are that I'm trying to catch some errors.
Right now all the structs match on %{data: data}, which means that if I want to pipe the query results through to any other functions I need to match on 3 separate function heads (one for each struct). It makes things a bit unclear.
I can see the reasoning for each struct (allowing for enumerable, etc) but I can also see a benefit in just handing the data back as pure data:
{:ok, data: []}
{:error, message:message}
It would help tremendously! As with the other issue I just logged this would be a breaking change - but I just thought I'd ask :).
Good point. Structs are important for implementing the Enumerable protocol. Feeds/Cursors/Collections all support Enumerable. However, segmenting into {:ok, %{data: data}} and {:error, %{message: message}} or something along those lines might be valuable. Definitely a breaking change. Probably a bigger breaking change than I'd like right now. I'd want to see some community discussion around this, because people like you seem to be popping up left and right with "Hey, I'm using this right now and it's awesome!" and I want to keep that sentiment going. :smile:
Yep - completely understand. I swear every time I write an app with Elixir (sometimes just goofing around or whatever) I think about when a struct would be useful and the only time is wrapping up a message. I habitually never wrap data in a struct - not sure why.
I can see having something like this:
{:ok, %RethinkDB.Result{inserted: 1, generated_keys: [], data: []...}}
{:error, %RethinkDB.Error{message: "table X doesn't exist"}}
The nice thing about having a struct here is the {:error} could be something else entirely (network, whatever) so matching on the rethink error is nice.
Less code for you to maintain and even then you could probably ditch the structs altogether if you wanted :). I think it's implied that something is coming back. If you provide data as a Keyword.t every time, though, that solves your Enumerable thing :).
Anyway - cheers and thanks for the response.
It doesn't solve the Enumerable because Cursors, for example, require fetching more data to fully Enumerate. I'd have to move the struct inside the data: key, or return a much less descriptive vanilla stream (via Stream.resource).
I want to reopen this with the proposal of:
{:ok, %{data: data}}
{:error, %RethinkDB.Exception{}}
I really like that - it would certainly help my situation. Would data be a list, then, in every case?
%{data: data} would be one of the existing structs (we'd still use them). They all have a data field, but %Record{} for example only has a single element for data.
As a non-breaking change, I added the response option to run. It accepts :struct (default) and :tuple.
The current behavior is to drop the struct except for Feed. So %RethinkDB.Record{data: 5} becomes {:ok, 5} and %RethinkDB.Collection{data: [1,2,3,4]} will be {:ok, [1,2,3,4]}. %RethinkDB.Feed{} becomes {:ok, %RethinkDB.Feed{}}. Anything with an error message becomes {:error, %RethinkDB.Response{}}.
This is in branch tuple_response. Some issues I uncovered:
-
noreplyoptions. ifnoreplyis specified, we just return:ok. -
profile. Returning a response with a profile won't really work without a struct (or something map like). Structs are really easy to extend without it being a breaking change. We could solve this by always doing{:ok, struct}or we could have a{:ok, struct}only when profiling is enabled.
Looking for feedback here. @robconery @leolorenzoluis
This is really nice and feels like idiomatic Elixir. Using the :tuple option means we'll see less of pattern matching along the lines of %Record{ data: my_data } which is a good thing, in my opinion.
Can't comment on the issues uncovered as I've yet to use noreply and profile. Looking forward to seeing these features in the next release. :+1:
I'm inclined to do {:ok, struct}
{:ok, %{data: data}} is succinct enough. Another issue I found was that Records can be a list and that would not be distinguishable from a Collection. Dropping the struct would lose information.
is it necessary to differentiate between a record and collection struct? For example if I am calling get() i know i need to match either a map or nil, if i am calling get_all then i must match [] or [doc] or _others. For example, this is my implementation of a custom run() that returns {:ok, data} and {:error, error} and i haven't really found a case where this wouldn't work. https://gist.github.com/sikanhe/a749e4e8ae35903c0a86
One example is get_field. get(id) |> get_field(tags) will return a Record with a list, whereas get_all([id1, id2]) |> get_field(tags) will return a Collection of lists.
It can be argued that you should know what to expect from the query, but there's still information there and I don't think it's the drivers call to prune that information.
If we continued with a response: option for run, then it wouldn't be that big of a deal, but I'm inclined to always return a tuple and therefore don't want to prune data.
I think we have to take this from the developer's perspective. The pattern I have observed is that many, if not all, the developers that are that using the elixir driver are building some kind of wrapper/helper functions to be able to easily parse response. If we had the developers pick whether to learning about the response or coming up with wrapper functions themselves, I think would rather spend a minute to read the doc and/or inspect the result.
It actually confused me in the begginning days when i learned about the response data and to be in the :data field inside the struct, as this is a different behavior from the official drivers.
If we take a look at the official drivers, I think they all return what's inside the data directly as the response in the callback.
https://www.rethinkdb.com/docs/guide/ruby/
Also, from my experience with the driver so far and other people's experience (in slack chat), i haven't seen anyone need to really match against %RethinkDB.X{} struct. Most of the time you directly match against the structure of the data itself.
The only one I had to know was %RethinkDB.Exception.ConnectionClosed{}, in that case we should be either returning :error tuple or be raising.
The argument against not "wanting to prune data", is that the struct types haven't really been useful (at least to the developers) so far, and that data weren't available from the official drivers to begin with.
Maybe I just haven't used wide range of queries, but does anyone have an example were the existing structs play an important role in parsing the response?
Let's actually look at how the Ruby driver works.
irb(main):006:0> r.table_list.run
=> ["events", "people", "posts", "sources"]
irb(main):007:0> r.table("people").run
=> #<RethinkDB::Cursor:70162338578840 (enumerable): r.table("people")
[{"hello"=>"green", "id"=>"15fdec16-07c4-448d-8464-5f987c233f3e"},
{"hi"=>"dana", "id"=>"01eb4c68-99a7-4f4c-a10f-3dae8b4b77cc"},
{"hi"=>"dana", "id"=>"26a32d45-7556-4d33-9986-236f4bdb8f87"},
{"hello"=>"test", "id"=>"9d7125c9-1765-474d-a483-da0c0358da25"},
{"hello"=>"downtown", "id"=>"a6735b94-4f51-4d2f-8aae-fbf4ee6d6f81"},
{"hello"=>"otherwise", "id"=>"bc177367-720f-4a18-8d8e-3a10dfb7203e"},
{"hello"=>"blue", "id"=>"59bed884-d1f8-48ff-8fed-095855dfa243"},
{"id"=>"ccb05872-9643-4538-ae7b-0610c73be4c3", "num"=>3, "sick"=>3}]>
irb(main):010:0> r.table("people").changes.run()
=> #<RethinkDB::Cursor:70162351420340 (enumerable): r.table("people").changes
[...]>
Record always becomes raw data. Collection and/or Feed are consolidated under Cursor.
irb(main):008:0> r.table("people").run(profile: true)
=> {"profile"=>[{"description"=>"Evaluating table.", "duration(ms)"=>1.071806, "sub_tasks"=>[{"description"=>"Evaluating datum.", "duration(ms)"=>0.001103, "sub_tasks"=>[]}, {"description"=>"Evaluating db.", "duration(ms)"=>0.690212, "sub_tasks"=>[{"description"=>"Evaluating datum.", "duration(ms)"=>0.000769, "sub_tasks"=>[]}]}]}, {"description"=>"Perform read.", "duration(ms)"=>0.619878, "sub_tasks"=>[{"parallel_tasks"=>[[{"description"=>"Perform read on shard.", "duration(ms)"=>0.06057, "sub_tasks"=>[{"description"=>"Do range scan on primary index.", "duration(ms)"=>0.053051, "sub_tasks"=>[]}]}], [{"description"=>"Perform read on shard.", "duration(ms)"=>0.068405, "sub_tasks"=>[{"description"=>"Do range scan on primary index.", "duration(ms)"=>0.061018, "sub_tasks"=>[]}]}], [{"description"=>"Perform read on shard.", "duration(ms)"=>0.080197, "sub_tasks"=>[{"description"=>"Do range scan on primary index.", "duration(ms)"=>0.075086, "sub_tasks"=>[]}]}], [{"description"=>"Perform read on shard.", "duration(ms)"=>0.052218, "sub_tasks"=>[{"description"=>"Do range scan on primary index.", "duration(ms)"=>0.047464, "sub_tasks"=>[]}]}], [{"description"=>"Perform read on shard.", "duration(ms)"=>0.055584, "sub_tasks"=>[{"description"=>"Do range scan on primary index.", "duration(ms)"=>0.049821, "sub_tasks"=>[]}]}], [{"description"=>"Perform read on shard.", "duration(ms)"=>0.032157, "sub_tasks"=>[{"description"=>"Do range scan on primary index.", "duration(ms)"=>0.026225, "sub_tasks"=>[]}]}], [{"description"=>"Perform read on shard.", "duration(ms)"=>0.036902, "sub_tasks"=>[{"description"=>"Do range scan on primary index.", "duration(ms)"=>0.029159, "sub_tasks"=>[]}]}], [{"description"=>"Perform read on shard.", "duration(ms)"=>0.036854, "sub_tasks"=>[{"description"=>"Do range scan on primary index.", "duration(ms)"=>0.029242, "sub_tasks"=>[]}]}]]}]}], "value"=>#<RethinkDB::Cursor:70162351565720 (enumerable): r.table("people")
[{"hello"=>"green", "id"=>"15fdec16-07c4-448d-8464-5f987c233f3e"},
{"hi"=>"dana", "id"=>"01eb4c68-99a7-4f4c-a10f-3dae8b4b77cc"},
{"hi"=>"dana", "id"=>"26a32d45-7556-4d33-9986-236f4bdb8f87"},
{"hello"=>"test", "id"=>"9d7125c9-1765-474d-a483-da0c0358da25"},
{"hello"=>"downtown", "id"=>"a6735b94-4f51-4d2f-8aae-fbf4ee6d6f81"},
{"hello"=>"otherwise", "id"=>"bc177367-720f-4a18-8d8e-3a10dfb7203e"},
{"hello"=>"blue", "id"=>"59bed884-d1f8-48ff-8fed-095855dfa243"},
{"id"=>"ccb05872-9643-4538-ae7b-0610c73be4c3", "num"=>3, "sick"=>3}]>}
irb(main):009:0> r.table_list.run(profile: true)
=> {"profile"=>[{"description"=>"Evaluating table_list.", "duration(ms)"=>1.100048, "sub_tasks"=>[{"description"=>"Evaluating db.", "duration(ms)"=>1.075075, "sub_tasks"=>[{"description"=>"Evaluating datum.", "duration(ms)"=>0.00188, "sub_tasks"=>[]}]}]}], "value"=>["events", "people", "posts", "sources"]}
Profile changes the response structure to the equivalent of %{profile: ..., value: data}
Emulating those changes would be acceptable to me. {:ok, list} would only apply to Record responses, whereas {:ok, %Collection{}} or {:ok, %Feed{}} would remain. It does bring up the notion of consolidating those into {:ok, %Cursor{}}. To me there's a strong semantic difference between the two (it tells you whether you have a complete set of data or if you need to fetch more). The official drivers get away with not differentiating, so it's probably not a big deal, though perhaps cluster semantics in BEAM would mean you don't want to send off a %Feed{} to another process (on a different node) whereas sending a %Collection{} would be fine.
I strongly agree with @sikanhe in that creating a wrapper to consume the rethinkdb-elixir is tedious and error prone. That would be great to be able to get back a tuple {:ok, list} instead of trying to unwrap it :smile:
The remaining question is “What do we do with Collection?”
Option 1: Treat it as a Feed (like the official drivers do)
Option 2: Treat it like a Record (and have {:ok, list})
Using a tuple with first element indicating success/failure is idiomatic in Erlang and Elixir so I too would request to follow this pattern here.
Ruby doesn't have pattern matching :)
I've settled on a hybrid. Same structs as before, but wrapped in a tuple.
Why? Because it's easy and doesn't require different responses for profiles.
It's not perfect, but it's better.
It's in master. Please try it out and let me know what you think. This was the main blocker before releasing 1.0.0 so the sooner I get feedback the better.
Awesome, i'll try this out this week. I think this will solve 90% of the issues and a helper can still go the remaining 10% to fit the use case.
Also worth mentioning... perhaps having a tuple with {status, documents, struct} would be as easy and still provide an easy way to pick out just data?
Currently i'm using this run helper to get similar functionality (also handles return_changes):
https://github.com/AdamBrodzinski/awesome-stack/blob/phoenix-1.2-new-graphql/server/lib/db/helpers.ex#L9
table('users')
|> DB.Helpers.run
>> {:ok, [{name: "Jane"}, {name: "Bob"}]}