[TS] optional escalar not set in the binary when it holds value 0

[jmillan]

flatbuffers: 23.5.26

Having the current schema:

table ConsumerLayers {
    spatial_layer: uint8;
    temporal_layer: uint8 = null;
}

Creating a buffer with temporal_layer = 0 results in a buffer without the temporal layer field.

I the field was not optional then skipping the field in the binary would make sense, but being it optional, how st the app supposed to distinguish between the field containing a 0 and it not being present?

[ibc]

It sounds like some wrong if (!value) in JS side which of course enters the condition if value is 0.

[jmillan]

It sounds like some wrong if (!value) in JS side which of course enters the condition if value is 0.

No, the code correctly checks for null. If the value is null then the field is not set, but if the value is 0 then it is attempted to be set, but not written in the buffer because 0 is the default value for the scalar.

[jmillan]

My suggestion here would be that for an optional field, the default value would be null, meaning that only if set to null the value would skip the buffer, otherwise, the value would be written in the buffer, even if it's 0.

[jmillan]

My suggestion here would be that for an optional field, the default value would be null, meaning that only if set to null the value would skip the buffer, otherwise, the value would be written in the buffer, even if it's 0.

Indeed this is how it's done in C++. There, if the field is optional it is unconditionally written in the buffer, if set.

[jmillan]

I see that we can simply set force defaults in the builder. I'll test this later.

[ibc]

If I make the temporal_layer field mandatory:

table ConsumerLayers {
    spatial_layer: uint8;
    temporal_layer: uint8;
}

then this is the diff in the generated TypeScript code:

diff -bur /tmp/fbs/consumer/consumer-layers.ts node/src/fbs/consumer/consumer-layers.ts
--- /tmp/fbs/consumer/consumer-layers.ts    2024-02-29 13:06:59
+++ node/src/fbs/consumer/consumer-layers.ts    2024-02-29 13:07:12
@@ -27,9 +27,9 @@
   return offset ? this.bb!.readUint8(this.bb_pos + offset) : 0;
 }

-temporalLayer():number|null {
+temporalLayer():number {
   const offset = this.bb!.__offset(this.bb_pos, 6);
-  return offset ? this.bb!.readUint8(this.bb_pos + offset) : null;
+  return offset ? this.bb!.readUint8(this.bb_pos + offset) : 0;
 }

 static startConsumerLayers(builder:flatbuffers.Builder) {
@@ -49,10 +49,9 @@
   return offset;
 }

-static createConsumerLayers(builder:flatbuffers.Builder, spatialLayer:number, temporalLayer:number|null):flatbuffers.Offset {
+static createConsumerLayers(builder:flatbuffers.Builder, spatialLayer:number, temporalLayer:number):flatbuffers.Offset {
   ConsumerLayers.startConsumerLayers(builder);
   ConsumerLayers.addSpatialLayer(builder, spatialLayer);
-  if (temporalLayer !== null)
   ConsumerLayers.addTemporalLayer(builder, temporalLayer);
   return ConsumerLayers.endConsumerLayers(builder);
 }
@@ -74,7 +73,7 @@
 export class ConsumerLayersT implements flatbuffers.IGeneratedObject {
 constructor(
   public spatialLayer: number = 0,
-  public temporalLayer: number|null = null
+  public temporalLayer: number = 0
 ){}

[ibc]

So AFAIS here the problem in builder.ts:

    addFieldInt8(voffset: number, value: number, defaultValue: number): void {
      if (this.force_defaults || value != defaultValue) {
        this.addInt8(value);
        this.slot(voffset);
      }
    }

When a ConsumerLayers is created with temporalLayer: 0, the above function is called with value: 0 and defaultValue: 0 so value != defaultValue => false and hence it doesn't enter the condition block and doesn't add the value.

The question here is, why is this addFieldInt8() being called with defaultValue: 0 instead of defaultValue: null as expected given the FBS?:

table ConsumerLayers {
    spatial_layer: uint8;
    temporal_layer: uint8 = null;
}

Yes, forcing the defaults would "fix" the problem but the issue does exist.

[ibc]

The question here is, why is this addFieldInt8() being called with defaultValue: 0 instead of defaultValue: null as expected given the FBS?

Well, it's called with defaultValue: 0 because that is what the generated fbs/consumer-layers.ts file does:

static addTemporalLayer(builder:flatbuffers.Builder, temporalLayer:number) {
  builder.addFieldInt8(1, temporalLayer, 0);
}

Notice that 3rd argument (defaultValue) is 0 instead of null. So the problem is in the TypeScript generator.

So setting temporal_layer: uint8 = null makes flatbuffers generate same TypeScript code than setting temporal_layer: uint8 = 0. In both cases, addTemporalLayer() calls builder.addFieldInt8() with defaultValue: 0.

[jmillan]

I see that we can simply set force defaults in the builder. I'll test this later.

This is defintely not an option as the C++ side is crashing with a SIGBUS, indicating there are issues when reading the buffer. This happens if doing a flatbuffers::FlatBufferToString() in C++ and also during reading.., so definitely this is not a fix.

The solution here would be as suggested:

My suggestion here would be that for an optional field, the default value would be null, meaning that only if set to null the value would skip the buffer, otherwise, the value would be written in the buffer, even if it's 0.

[jmillan]

So this problem seem to be solved already since Sept 2023 https://github.com/google/flatbuffers/pull/7864

But the last flatbuffer version (which we are using) was released in May 2023.

When is the next version being released?

[ibc]

So this problem seem to be solved already since Sept 2023 #7864

But the last flatbuffer version (which we are using) was released in May 2023.

10 months without releasing a new version (having obvious bug fixes merged in master branch in the meantime) worries me a lot.

[ibc]

Ok, ticket created asking for a new release: https://github.com/google/flatbuffers/issues/8246

[ibc]

I've tested the new release 24.3.6 and confirm that this issue is fixed (no surprise of course), so I think we can close this issue.