flatbuffers icon indicating copy to clipboard operation
flatbuffers copied to clipboard

HasNullDefault in TypeScript returns null for 0 scalar value input

Open TJKoury opened this issue 3 years ago • 10 comments

Flatbuffers 2.0.5 Compiled to WASM (link) Chrome Version 1.38.111

The offset check here will return null for offset 0 if the HasNullDefault flag is set for a property.

Setting a 0 value for a scalar results in offset being set to 0, which means a zero can't be stored in an property that can be null.

TJKoury avatar Jul 28 '22 13:07 TJKoury

Seem like a bug to me. I have vague memory of it being noted in the past but as of yet I've been unable to find anything.

I suppose changing this behaviour could potentially be seen as a breaking change, but I think it's likely no one is dependent on that so I think we should just try to find a sensible fix as a patch release.

bjornharrtell avatar Aug 04 '22 11:08 bjornharrtell

It would be good/interesting to find out if this was also the behavior pre TS port (i.e Flatbuffers 1.x).

bjornharrtell avatar Aug 04 '22 11:08 bjornharrtell

I have been looking at this since I posted the issue, hard to think of a portable solution that won’t effect parsing flatbuffers in other languages. The main issue is there doesn’t seem to be a null datatype in the flatbuffer spec. Trying to shoehorn one in at the language level is not the right approach, especially when the null field descriptor is able to be applied to any datatype. There should be a “lack of value” datatype available as a type.

TJKoury avatar Aug 04 '22 11:08 TJKoury

Hmm yes there is more to this than I thought. But there does seem to be something previous to read up.. see https://github.com/google/flatbuffers/issues/6014 and https://github.com/google/flatbuffers/pull/6215.

bjornharrtell avatar Aug 04 '22 12:08 bjornharrtell

I should have read those more closely and then linked them to this issue, thanks. Very strange this was implemented without a null primitive, and seems to be incompletely supported cross-language. I don’t think any implementation will be accepted without engaging the wider community.

TJKoury avatar Aug 04 '22 12:08 TJKoury

@TJKoury not sure I agree, it's clear that the intent is that if you describe a scalar type as optional (with = null in schema) then it is taking up no wire space (which I believe equals offset zero) and should be interpreted as null. A 0 should in this case be stored and as such not result in a zero offset. If my interpretation is correct and that the current implementation returns null for 0 when default is null it's a clear bug.

bjornharrtell avatar Aug 04 '22 12:08 bjornharrtell

The issue in this case is that it appears that in all languages an offset of zero is equivalent to zero scalar, to save wire space as you say. To change the interpretation of zero offset to mean “null” in the case of an optional null field would encode that meaning into the flatbuffer without a way to interpret it cross-language. I suppose that’s true now with simply failing to compile in unsupported languages. We could store the default offset for the scalar with a value of 0, and the gain in wire space is the trade-off for using it.

TJKoury avatar Aug 04 '22 12:08 TJKoury

@TJKoury but the change to make that interpretation has already been done in most implementations? I don't see how it affect the encoding, it's simply a interpretation (schema generated code) change and I don't understand your claim that it's "without a way to interpret cross-lang".

bjornharrtell avatar Aug 04 '22 13:08 bjornharrtell

It seems most implementations regard a zero offset as the value zero, right?

TJKoury avatar Aug 04 '22 14:08 TJKoury

Picking this back up, I'm going to remove using null at all since:

  • It's not supported fully cross-language
  • It is not supported at all in strings or a few other types
  • It is potentially bad abstraction to give fields that can be different datatypes the same potential value that is not technically part of the set of possible values per datatype

TJKoury avatar Aug 25 '22 21:08 TJKoury

This issue is stale because it has been open 6 months with no activity. Please comment or label not-stale, or this will be closed in 14 days.

github-actions[bot] avatar Mar 04 '23 01:03 github-actions[bot]

This issue was automatically closed due to no activity for 6 months plus the 14 day notice period.

github-actions[bot] avatar Mar 18 '23 20:03 github-actions[bot]