go-thrift icon indicating copy to clipboard operation
go-thrift copied to clipboard

Are we missing "default" field requiredness?

Open anxiousmodernman opened this issue 8 years ago • 1 comments

I've generated a few thrift clients with this lib. Working good so far. But I have an instance where I've needed to modify the generated code. It's related to the "requiredness" of fields on a struct.

My example

Using this library, I change this Thrift struct

/**
 *  Audit response status object.
 *
 *  @param success. True if audit request completed successfully, otherwise set to false
 *  @param messages. List of any error or warning messages that may have occurred
 */
struct AuditResponse {
    1: string status,
    2: list<string> messages
}

...into this Go struct.

type AuditResponse struct {
    Status   string   `thrift:"1,required" json:"status"`
    Messages []string `thrift:"2,required" json:"messages"`
}

The service I'm communicating with often omits Messages, so I get an error when I call an endpoint that returns an AuditResponse, due to (I think) Messages being interpreted as required.

If I hack the generated code by changing the requiredness, an error no longer occurs.

type AuditResponse struct {
    Status   string   `thrift:"1,required" json:"status"`
    Messages []string `thrift:"2,optional" json:"messages"` // NOTE: Modified.
}

Should there be a "default" requiredness?

According to this documentation from Apache, there is a requiredness type called "default", which is implicit if fields in Thrift IDL are not explicitly defined as "required" or "optional".

I change the generated code from "required" to "optional", because I need the Read behavior of optional, which the "default" requiredness retains.

Is there a reason this is not implemented?

anxiousmodernman avatar Apr 25 '16 19:04 anxiousmodernman

Ah, I see this as a TODO at the bottom of the README. Should have read to the bottom!

anxiousmodernman avatar May 25 '16 22:05 anxiousmodernman