TwirpServer Constructor not capturing ServerOptions in v8.1.2

[faizanahmadsyed]

Hello, I am fairly new to Twirp so this may be something I am doing wrong. We recently upgraded from github.com/twitchtv/twirp/protoc-gen-twirp@v8.0.0 to github.com/twitchtv/twirp/protoc-gen-twirp@v8.1.2.

We have a service called Builder which we are instantiating in the router.go of our application using:

builderHandler := rpc.NewBuilderServer(d.handlers.builder, tw.DefaultOptions(), twirp.WithServerPathPrefix(pathPrefix))

Here is the definition of tw.DefaultOptions():

func DefaultOptions() twirp.ServerOption {
    options := func(o *twirp.ServerOptions) {
        o.JSONSkipDefaults = true
        o.Hooks = defaultHooks()
    }
    return options
}

The NewBuilderServer Twirp function gets defined as follows in v8.1.2:

func NewBuilderServer(svc Builder, opts ...interface{}) TwirpServer {
    serverOpts := newServerOpts(opts)

    // Using ReadOpt allows backwards and forwads compatibility with new options in the future
    jsonSkipDefaults := false
    _ = serverOpts.ReadOpt("jsonSkipDefaults", &jsonSkipDefaults)
    jsonCamelCase := false
    _ = serverOpts.ReadOpt("jsonCamelCase", &jsonCamelCase)
    var pathPrefix string
    if ok := serverOpts.ReadOpt("pathPrefix", &pathPrefix); !ok {
        pathPrefix = "/twirp" // default prefix
    }

    return &builderServer{
        Builder:          svc,
        hooks:            serverOpts.Hooks,
        interceptor:      twirp.ChainInterceptors(serverOpts.Interceptors...),
        pathPrefix:       pathPrefix,
        jsonSkipDefaults: jsonSkipDefaults,
        jsonCamelCase:    jsonCamelCase,
    }
}

The issue here is that, as we can see in the definition of tw.DefaultOptions(), jsonSkipDefaults should be set to true. However, _ = serverOpts.ReadOpt("jsonSkipDefaults", &jsonSkipDefaults) does not change the value of jsonSkipDefaults and leaves it as false.

For context here is how the NewBuilderServer Twirp function gets defined in v8.0.0:

func NewBuilderServer(svc Builder, opts ...interface{}) TwirpServer {
    serverOpts := twirp.ServerOptions{}
    for _, opt := range opts {
        switch o := opt.(type) {
        case twirp.ServerOption:
            o(&serverOpts)
        case *twirp.ServerHooks: // backwards compatibility, allow to specify hooks as an argument
            twirp.WithServerHooks(o)(&serverOpts)
        case nil: // backwards compatibility, allow nil value for the argument
            continue
        default:
            panic(fmt.Sprintf("Invalid option type %T on NewBuilderServer", o))
        }
    }

    return &builderServer{
        Builder:          svc,
        pathPrefix:       serverOpts.PathPrefix(),
        interceptor:      twirp.ChainInterceptors(serverOpts.Interceptors...),
        hooks:            serverOpts.Hooks,
        jsonSkipDefaults: serverOpts.JSONSkipDefaults,
    }
}

This works as expected. These two pieces of code are semantically identical for our purposes. I was wondering if I could get some help in determining what could be causing an issue like this to occur. Any guidance will be greatly appreciated!

[wmatveyenko]

Thanks for the bug report. We will look into this.

[wmatveyenko]

Still keeping this open.

[fpetelin]

Hello @faizanahmadsyed!

in v8.1.0 and onward, jsonSkipDefaults is set to false by default, and the NewBuilderServer constructor no longer recognizes the JSONSkipDefaults value. For reference, here's the note regarding that. We could potentially improve the comments in the options struct regarding deprecation of that value.

Two potential solutions would be:

1. Use the options as illustrated in the docs:

   builderHandler := rpc.NewBuilderServer(d.handlers.builder, 
   twirp.WithServerJSONSkipDefaults(true), 
   twirp.WithServiceHooks(defaultHooks()),
   twirp.WithServerPathPrefix(pathPrefix))

2. If still inclined to define reusable defaults, define defaultOptions and use them in the following way:

   
   defaultOptions := []twirp.ServerOption{
   twirp.WithServerJSONSkipDefaults(true),
   twirp.WithServiceHooks(defaultHooks()),
   }

opts := []twirp.ServerOption{} opts = append(opts, defaultOptions...) opts = append(opts, twirp.WithServerPathPrefix(pathPrefix))

builderHandler := rpc.NewBuilderServer(d.handlers.builder, opts...)

[wmatveyenko]

Given the explanation from @fpetelin, I am closing this issue.