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

Are we missing "default" field requiredness? #92

Open
anxiousmodernman opened this issue Apr 25, 2016 · 1 comment
Open

Are we missing "default" field requiredness? #92

anxiousmodernman opened this issue Apr 25, 2016 · 1 comment

Comments

@anxiousmodernman
Copy link

anxiousmodernman commented Apr 25, 2016

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
Copy link
Author

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

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

1 participant