Handle undefined value in an object

[neumino]

`ryexley on IRC had trouble replicating our nodejs chat application.
He got this error

[ERROR] 22:47:13 RqlDriverError
{ name: 'RqlDriverError',
  msg: 'Unknown datum value `undefined`, did you forget a `return`?',
  message: 'Unknown datum value `undefined`, did you forget a `return`?' 
}

While this error make a lot of sense when the user forget a return in a lamda function for a filter for example, here it was failing for this query:

r.db("config.database.name").table("users").insert( {
    name: data.name,
    email: data.email,
    message: data.message
} )

And the reason why, was because one of the field had the value undefined.
We should catch undefined values in r.expr() and just ignore the key if undefined is returned.

Assigning to @wmrowan, tagging as 1.5

[jdoliner]

I feel like ignoring undefined isn't as good as erroring null is one thing because that's frequently used to mean non existence but if someone's passing us undefined there's a decent chance they've made a mistake they're going to want to know about.

[coffeemug]

I agree with @jdoliner -- I think the JS driver should error when undefined is given as a value to a dict in expr (we already error, this way we'll just give a better error message in this case).

[neumino]

Hum, I don't think throwing an error is a absolutely bad, but in CoffeeScript, this syntax is legal

obj =
    id: 1
    key: "value" if boolean

It get parsed to

obj = {
  id: 1,
  key: boolean ? "value" : void 0
};

Which will throw an error if we directly pass it to insert()

[taybin]

If null throws an error, and undefined is ignored, how should one delete an attribute from an item when updating?

[jdoliner]

So the easiest way to delete An attribute is with replace and without. For
example:

table.replace(lambda row: row.without("foo"))
On Apr 13, 2013 7:10 PM, "Taybin Rutkin" notifications@github.com wrote:

If null throws an error, and undefined is ignored, how should one delete
an attribute from an item when updating?

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

[coffeemug]

To clarify, null wouldn't throw an error -- it would actually insert a key with a null value.

[wmrowan]

Here's what's actually happening here. The driver attempts to convert any native values passed to it into a RQL datum. Since it doesn't know how to convert undefined into a datum, it throws an error. I see a few options here.

1 this error is sufficient, do nothing
2 try to provide a better error in context (object key 'foo' is undefined)
3 silently ignore undefined as if it didn't exist

We already do something like option 2 in the case of a lambda function returning undefined. We could add other common cases to help people understand where the error is coming from (in this case because that property isn't set on the original object. This change would actually require more work than the other two options but helps people uncover situations that are mostly likely errors as in this case.

Michel's right that the last option is must more javascript-y. Fields whose value is undefined are treated as if they don't exist. This might better match user expectations / fit with the user's data model better and is easy to implement.

I'm not actually sure where my personal opinion falls on this. I do agree that the place to handle this situation is probably not within the datum constructor where we've lost all context for the error.

[coffeemug]

Fields whose value is undefined are treated as if they don't exist.

I feel like this could be confusing. If I saw a query row.update({ foo: undefined }), I'd expect it to change foo to undefined in the db, or remove foo from the document altogether. The last thing I'd expect is for this query to do absolutely nothing.

We could change the protobuf to actually support undefined, but I think that's a bad direction personally. It's more work, it doesn't play well with other drivers, and it requires thinking through many edge cases.

I'd prefer fixing the actual problem at hand. Why don't we add a better error message for this specific issue (people passing undefined as values to the literal dict)? It would fix the immediate problem that 90% of people are likely to run into, wouldn't be very much work, and would allow us to sidestep most of the tricky issues. If we discover more issues related to undefined, we could add better error messages there as needed. I think this is a better option than doing a more fundamental redesign or an auditing pass for better error messages in general.

TL;DR: less is more.

[taybin]

What might be better is to add DEL_ATTR to the protobuf, and allow the javascript driver to recognize setting undefined on an attribute as a call to DEL_ATTR. Then you'd be keeping with javascript conventions, and deleting an attribute might become faster/easier than row.update( row.without("foo") ).

[coffeemug]

This is almost certainly better from the user's POV, but requires more work on the server (which already has quite a backlog). @mlucy -- how much work do you think this is? (I think I'd prefer adding a better error message here in the JS driver in the short term, and adding DEL_ATTR later down the line when things quiet down a bit)

[jdoliner]

So I don't fully understand what it would mean for undefined to call DEL_ATTR. Would you mind clarifying a bit?

[taybin]

I have two proposals. Adding a DEL_ATTR to simplify deleting an attribute and having the javascript driver recognize undefined as a shortcut for this.

I'm imagining that much like {'foo': r.row('foo') + 1} is detected and changed into a FUNC term, {'foo': undefined} could be detected and changed into a DEL_ATTR term.

You might consider this too hacky if it has to be handled as a special value at the DatumTerm level, but it would fit with javascript conventions.

Also I'm just guessing that DEL_ATTR could be faster than WITHOUT, but I don't know enough about the implementation to know for sure.

[wmrowan]

@taybin is DEL_ATTR an operation or a value (akin to undefined in JS)?

As a valueDEL_ATTR would mean wading back into the treacherous waters of supporting a third kind of non-existence in RQL, a NULL with yet different semantics and different again from fields that simply don't exist. I think we've discussed this before and would really like to avoid this extra complication.

As an operation, I'm having trouble seeing how DEL_ATTR would different from without or how it could be made more efficient. Could you elaborate?

Personally, I agree with @coffeemug that the right way to close this issue is to provide a better error message when an undefined value sneaks it's way into a query. It's simpler and is probably what people want in most situations. While it clashes with JavaScript's model for undefined fields, we always need to consider how potential new RQL features fit into other supported or potentially supported languages. JavaScript's model is pretty unique and would feel more out of place in almost every other language we support than erroring on undefined is in JS.

[taybin]

I was thinking it would be an operation. Having a third type of non-existence clearly isn't the way to go.

I was speculating that WITHOUT would require loading the row and that DEL_ATTR might not. I'm not familiar with the server implementation, so I'm probably wrong on that count.

[coffeemug]

I'm having trouble seeing how DEL_ATTR would different from without or how it could be made more efficient.

It might not necessarily be different, but it certainly would offer a degree of convenience that without doesn't.

Anyway, let's go with a better error message for now. We'll open a separate issue for DEL_ATTR if/when this comes up again.

[coffeemug]

Assigning to @wmrowan via 1.5-sprint-3.

[wmrowan]

in code review 427 by @coffeemug

[coffeemug]

@wmrowan -- what's the state of this issue? (the review has been cancelled)

[wmrowan]

This was in the big review and part of the package that just got merged. I must not have split out the commit that contained this fix into a separate review. This is the relevant commit: e553169

[coffeemug]

Could you submit a code review with the relevant polyglot tests?

[wmrowan]

review number 452

[rolftimmermans]

I am trying to remove a value from a document. I am really, really surprised I can't simply call update with an object literal and an attribute set to undefined. I know there are alternatives but because I'm building the update object dynamically just setting an attribute to undefined really would be the easiest way.

The error is: Object field 'card' may not be undefined

[neumino]

If you want to remove a fielld, you should use replace

r,table("foo").replace( r.row.without("card") ).run( conn, callback)

If we were using undefined to remove a field, it would mean

r.table("foo").update( { card: undefined } ).run( conn, callback)

would erase all other fields since they are all undefined.

[jdoliner]

undefined isn't a value in JSON and it doesn't have a ready equivalent in languages besides Javascript. The way to do what you're looking for is using literal. For example to delete the field "a" from a value you can say:

table.update({a : r.literal()})

literal allows you to plop a value into an object exactly is it is. So literal() means put "nothing" in this value. This is the closest we could get to the use case you're describing and keep things cross platform.

[coffeemug]

So, we could make the JS driver treat undefined as r.literal() in case of update and it wouldn't impact other drivers. I think we chose not to do that because it would significantly complicate the driver (for example, how would we treat undefined in case of insert, or elsewhere?)

This might be worth revisiting though, if a lot of people report running into this problem.

[thelinuxlich]

I would like this behavior, but first #4428 should be fixed

[riteshsangwan]

@neumino From the above discussion there is no clear conclusion as to how undefined values will be handled not just in insert operation but also for filter operation.

1. I have below code that fails when either username or email is undefined.

const users = yield User.filter(r.row('username').eq(username).or(r.row('email').eq(email)));

This will throw error

Cannot convert `undefined` with r.expr() in:\nr.table("users").filter(r.row("username").eq(undefined).or(r.row("email").eq(undefined)))

Logically since I have a users table where email or username cannot be null/undefined I would expect this query to return 0 results. This is what mongo returns since many of the developers actively work on mongo we would expect this to work the same way.

What I would like is for this query to not throw error and return 0 results.

I have been reading the docs and is not able to find the solution. How to get this done in rethink world.

2. Consider below code

const users = yield User.filter({ username: undefined });

This will return all the users and completely ignore undefined value for username. This is logically not correct since ideally this should not return any results because there are no records where username is undefined

Additional Info

User is a thinky model

const User = thinky.createModel('users', {
    id: type.string().uuid(4),
    fullName: type.string().required(),
    username: type.string().required(),
    email: type.string().email().required(),
    password: type.string().required(),
    firstName: type.string().alphanum().allowNull(true),
    lastName: type.string().alphanum().allowNull(true),
    bio: type.string().allowNull(true),
    // the timestamp this resource is created
    createdAt: type.number().integer().required(),
    // the timestamp this resource was last updated
    updatedAt: type.number().integer().required(),
  }, {
    enforce_extra: 'remove',
    enforce_type: 'strict',
  });