Unable to check if Get() returns nothing about rethinkdb-go HOT 23 CLOSED

Hi, Thanks for the bug reports.

I agree that this is a problem and something I had forgotten to sort out. I am not too worry about backwards compatibility until both RethinkDB and this driver are stable and I don't believe your changes will effect too many people either.

I think it would be good to have an IsNil method instead of having ErrNotFound. It might make sense to use IsEmpty for ResultRows and IsNil for ResultRow. Looking at the mgo API docs they use an error as they dont return a result struct.

If you could prepare a PR that would be great. I will look into sorting your other issues.

from rethinkdb-go.

Two possibilities :

func (r *ResultRow) IsNil() (bool, error)

or

func (r *ResultRow) IsNil() bool

The former allows to handle errors (r.err != nil and r.rows.Next() == false). The latter is simpler...

What do you prefer ?

from rethinkdb-go.

I personally prefer func (r *ResultRow) IsNil() bool, as you mentioned it is simpler and both ResultRow and ResultRows already have an Err function.

What do you prefer?

from rethinkdb-go.

Same as you...

from rethinkdb-go.

In your example, why rows.Next should return true for a missing key?
We can just return false, and all things should work.

rows, err := r.Table(table).Get("missing key").RunRow(rs)
// err == nil
rows.Next() 

from rethinkdb-go.

I'm not sure, but there might be cases when a Run query returns a list of NULL values. Testing a NULL datum in Next() might cause problems in such queries.

I don't think the Get/Run/Next/Scan pattern will be often used compared to Get/RunRow/Scan.

Personally, having an awkward Get/Run/Next/Scan does not bother me.

from rethinkdb-go.

I dont think we should change the behavior of Next but adding the IsNil and
IsEmpty methods should work for all cases.

Having a NULL return value should be handled differently depending on the
query and this is up to the program which uses the driver.

On 6 October 2013 17:41, Jean-François Bustarret
[email protected]:

I'm not sure, but there might be cases when a Run query returns a list of
NULL values. Testing a NULL datum in Next() might cause problems in such
queries.

I don't think the Get/Run/Next/Scan pattern will be often used compared to
Get/RunRow/Scan.

Personally, having an awkward Get/Run/Next/Scan does not bother me.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/11#issuecomment-25771455
.

from rethinkdb-go.

What is a reason of rows.Next() being true where there is no value?

from rethinkdb-go.

RethinkDB always returns a row with a nil (NULL) value to Get queries for missing keys.

RethinkDB was created for dynamic/loosely typed langages in mind where nil (as in "not set") exists. Go has no notion of "not set", and this  adds complexity and/or awkwardness and/or whatever you name it in some use cases.

To be able to make Next() return false would require to test for this nil value, with side effects (on queries which would return an array of nil vaules) or to add complex code (if it can be done)

IMHO, that is not worth it

from rethinkdb-go.

Thanks for sorting this @jfbus.

from rethinkdb-go.

Current API for IsNil + Scan pattern doesn't work with errors.
I suggest to change the example (or API) because it misleads how to use it properly, eg for something like:

row := r.Table(table).Get("missing key").RunRow(rs)
// if a connection is broken then row.err != nil and row.IsNil() == true
err := row.Scan(&obj)
// check if obj exists:
err != nil && !row.IsNil()
}

Also it might lead to other bugs -> #16 (comment)

from rethinkdb-go.

It might be better to have RunRow return (*ResultRow, error) and move r.rows.Next() + r.rows.Close() to RunRow.

All server related errors would occur in RunRow, and none in IsNil.

from rethinkdb-go.

I agree with @jfbus as this would also mean that we dont have to have the fetched field in ResultRow as it will always be fetched.

from rethinkdb-go.

PR follows.

Maaaaaany tests use query.RunRow(sess).Scan(&response) => PR is quite large

from rethinkdb-go.

Yes, the API change is more clear 👍

from rethinkdb-go.

I know you want to hate me here ...
While writing a code with current API version and RunRow pattern I found that I always call IsNull or Scan. I didn't found any interesting use-case when you want to handle error in different place then value.
So the pattern is always:

row, _ := term.RunRow(s) // here we can skip error checking because it is the same as 2 lines below
var response someType
err :=row.Scan(&row)

or

row, err := term.RunRow(s)
exists := row.IsNull()

From this observation the following IsNull declaration would be more consistent:

func (*ResultRow) IsNil() (bool, error)

And everywhere we can simply write in one row:

exists, err = term.RunRow(s).IsNull()
// or:
err = term.RunRow(s).Scan(&response)

If @jfbus agrees I can PR (but for this we need to revert #22 )

from rethinkdb-go.

I know you want to hate me here ...

Daniel may be thinking we are two pains in the ass 😄

I find it strange to have Run return (*ResultRows, error) and RunRow just return *ResultRow. IsNil being just a test, I don't think it needs to return an error.

If you want compact code, you could also have :

err := term.RunAndScan(s, &response)
if _, ok := err.(*r.ErrorNilResult); ok {
  // not found
}

RunAndScan = RunRow + IsNil + Scan

and

exists := term.Exists(s)

Exists = RunRow + IsNil

... or you can have RunRow return a ErrorNilResult error when the returned row is nil.

from rethinkdb-go.

The case it to find a nicely working API, and without using it we won't find it ;)

Providing Exists and RunAndScan makes IsNil and Scan useless (publicly).
Other option for RunAndScan: (*)

notNil, err := term.RunAndScan(s, &response)

I was also asking myself: why we just can't set nil values for response as in rethinkdb first class drivers?, but it looks that we need to support simple types which can't have nil values.

1. What about dropping err from ResultRow.Scan() (and in internall implementation of Scan and IsNil)?
   If RunRow doesn't return non nil error then Scan and IsNil shouldn't.

2. As in (*) we can get rid of IsNil by returning two values in Scan:

   // ok is true if value exists, err is nil when there are no internal DB / connection / serialization errors.
   func (r *ResultRows) Scan(dest interface{}) (ok bool, err error)
   func (r *ResultRow) Scan(dest interface{}) (ok bool, err error)
   

Personally I like the second option.

from rethinkdb-go.

You cannot remove err from ResultRow.Scan() because of unmarshalling errors

from rethinkdb-go.

So 1. is out. What about 2. ?

from rethinkdb-go.

No particular opinion on the subject...

from rethinkdb-go.

Ok so firstly I agree with @jfbus as I think IsNil should only return a bool. My reasoning for this is that if the function returned multiple values then it becomes more difficult to use.

I also think that the having the IsNil method for checking if no result was returned is the best as you might not always be scanning the result into a variable for example if you were just checking for the existence of some value (This also makes me think we might need some function for counting the number of elements found).

That being said I dont think any changes need to be made since looking at your code example @robert-zaremba in this comment I think you are making the assumption that the error returned by RunRow and Scan are always the same however as mentioned by @jfbus Scan can return its own errors caused by unmarshalling.

Also while I dont mind breaking BC too much I am going to start saving any changes that do so until the RethinkDB releases.

from rethinkdb-go.

In current implementation, Scan won't continue if there is already an error (ie the one caused by RunRow) - and will return that error. For checking existence one should use Count or IsEmpty.

Also is there any possibility where there will be a nil record for Scan on non singleRowSelection?

from rethinkdb-go.