creditkarma / thrift-typescript

Generate TypeScript from Thrift IDL files
Apache License 2.0
155 stars 32 forks source link

Initializers in unions don't work as expected #162

Closed hayes closed 5 years ago

hayes commented 5 years ago

I am not 100% sure what the correct behavior is, and just came across this on one of our schemas.

The unions looks something like:

enum NullSentinel {
  IS_NULL = 1,
}

union FieldValue {
  1: optional NullSentinel isNull = NullSentinel.IS_NULL
  2: optional bool boolValue
}

The generated code (for thrift-server) will look something like:

export class FieldValue extends thrift.StructLike implements IFieldValue {
    public isNull?: NullSentinel.NullSentinel = NullSentinel.NullSentinel.IS_NULL;
    public boolValue?: boolean;
    public readonly _annotations: thrift.IThriftAnnotations = {};
    public readonly _fieldAnnotations: thrift.IFieldAnnotations = {};
    constructor(args: IFieldValueArgs = {}) {
        super();
        let _fieldsSet: number = 0;
        if (args.isNull != null) {
            _fieldsSet++;
            const value_18: NullSentinel.NullSentinel = args.isNull;
            this.isNull = value_18;
        }
        if (args.boolValue != null) {
            _fieldsSet++;
            const value_19: boolean = args.boolValue;
            this.boolValue = value_19;
        }
        if (_fieldsSet > 1) {
            throw new thrift.TProtocolException(thrift.TProtocolExceptionType.INVALID_DATA, "TUnion cannot have more than one value");
        }
        else if (_fieldsSet < 1) {
            throw new thrift.TProtocolException(thrift.TProtocolExceptionType.INVALID_DATA, "TUnion must have one value set");
        }
    }
    public static read(input: thrift.TProtocol): FieldValue {
        return new FieldValue(FieldValueCodec.decode(input));
    }
    public static write(args: IFieldValueArgs, output: thrift.TProtocol): void {
        return FieldValueCodec.encode(args, output);
    }
    public write(output: thrift.TProtocol): void {
        return FieldValueCodec.encode(this, output);
    }
}

The issue here is that the union will always set isNull and if a value is passed boolValue, the constructor will end up with both props set.

My initial thought on this was that you should only set initializer values if _fieldSet < 1

hayes commented 5 years ago

~It looks like this is not an issue with strictUnions which is likely what I will try to migrate to~ I spoke too soon. I believe that strictUnions will encounter the same issue, but the way the default value is initialized is slightly different

kevin-greene-ck commented 5 years ago

@hayes This shouldn't be a hard fix. I'll get something published by Monday, will try to get it done today.

hayes commented 5 years ago

awesome! thank you for the quick response.