Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return null for missing optional field? #333

Closed
zz85 opened this issue Nov 7, 2015 · 10 comments
Closed

Return null for missing optional field? #333

zz85 opened this issue Nov 7, 2015 · 10 comments

Comments

@zz85
Copy link

zz85 commented Nov 7, 2015

I'm trying out the javascript generated flatbuffer code when I observe that optional string fields return null if the field doesn't exist while missing field for uint returns 0. I would think that it's useful to know when a field is null(missing) instead of returning 0 (which is a valid number).

Is this the intended behaviour? A quick check on the generated python and java classes shows that they return null for missing string fields but 0 on missing integers too.

To give a reduced use case, here's the idl.

table File {
        size: uint;
}

root_type File;

and the generated JS code.

/**
 * @returns {number}
 */
File.prototype.size = function() {
  var offset = this.bb.__offset(this.bb_pos, 6);
  return offset ? this.bb.readUint32(this.bb_pos + offset) : 0;
};

imho, i would prefer it to be the following (simply replace 0 with null when no offset is found)

File.prototype.size = function() {
  var offset = this.bb.__offset(this.bb_pos, 6);
  return offset ? this.bb.readUint32(this.bb_pos + offset) : null;
};

cc @evanw (nice work on the js bindings btw!)

update: I observe a similar behaviour with arrays too. a missing field for [children] would return null in .children() but 0 in .childLength().

@evanw
Copy link
Contributor

evanw commented Nov 7, 2015

I agree that it's nice to be able to know if a field is present or not. However, that doesn't appear to be how the flatbuffers spec is designed, so I think the generated JavaScript code is working as intended.

I'm looking at http://google.github.io/flatbuffers/md__schemas.html and the closest thing I can find is "non-scalar (string/vector/table) fields default to NULL when not present" although I can't find anything about scalar values defaulting to 0 when not present. Maybe @gwvo can clarify? I imagine cross-language consistency would be the desired behavior here.

In our project (C++ code), we solved this problem by using invalid sentinel values as defaults for scalar fields: 255 for bools and enums, INT_MIN for ints, and FLT_MIN for floats.

@ghost
Copy link

ghost commented Nov 9, 2015

Yes, this is how all languages work, and so should JS.

Returning null for a scalar would be bad, since the field not being present means it is equal to the default, not that it wasn't set.

For example, if your field is defined a size : uint = 6, and you explicitly write this field when constructing the FlatBuffer, initializing it to 6, it will not get written, since it is equal to the default. Then when you read it, getting null instead of 6 would be weird.

@zz85
Copy link
Author

zz85 commented Nov 10, 2015

Hmm, that's an interesting behaviour. In which case, I'm interested to know if there could be a null default for integers and whether it make any sense for have an api which returns whether the field is default?

Otherwise, I think Evan's approach seems reasonable for most cases, or an additional field could be defined to indicate the presence of a field.

@ghost
Copy link

ghost commented Nov 11, 2015

The easiest way to get a null default for an integer field is to wrap it in a struct:

struct myint { x:int; }
table mytable { scalar:myint; }

this will get you null if scalar isn't present. It also doesn't take up any more space on the wire than a regular int.

@ghost ghost closed this as completed Nov 26, 2015
@zz85
Copy link
Author

zz85 commented Nov 26, 2015

That mighht change the way the code is called, but thanks for the suggestion!

@ghost
Copy link

ghost commented Nov 30, 2015

This functionality is now in:
1fa803d

This allows you to call e.g.

mymonster.CheckField(Monster::VT_HEALTH)

to see if a field was stored at all. Note the caveat I mentioned still applies: you won't be able to tell the difference between a field that wasn't written vs a field that happens to have the default value this way, unless you use force_defaults. This is intrinsic to how FlatBuffers works.

@dudehook
Copy link

Table::CheckField is not visible to clients of the table object.
I'm hacking the code to generate a "has_xxx" method to make the call to CheckField.
i.e. mymonster.has_health()
I'll create a PR if you want it, but I'm only modifying the cpp generator since that's all we're doing.

@ghost
Copy link

ghost commented Jan 27, 2016

Use IsFieldPresent instead of CheckField: 995ee86

@dudehook
Copy link

Ah, excellent. I missed that.
One more question: will this have a "false negative" if the field is set, but happens to be set with the default value? I think I'm seeing that happening...

@dudehook
Copy link

Yes, never mind. I see that in the code comments.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants