THRIFT-3037 Go Codegen - Support Struct TypeDefs

[ethan-gallant]

[fishy]

Please create a jira ticket to describe what problem you are trying to resolve, what's the before and after generated go code from what thrift code.

[ethan-gallant]

Please create a jira ticket to describe what problem you are trying to resolve, what's the before and after generated go code from what thrift code.

Hello @fishy ! :)

The ticket THRIFT-3037 best describes this issue so I figured it best not to create a duplicate. Here is an example of how the generated code changes. I've listed them as bullet points for the major changes.

Change description

This PR makes it possible for typedefs that don't refer to a primative type to compile. This is accomplished by using a struct instead of declaring a type pointer (fixes the issue with READ methods etc. not existing).

Code changes

Here is the example schema which we will be using to describe the issue. Without the change, the code does not compile.

struct ExampleRequest {
    1: required string example_field
}

struct ExampleResponse {
    1: required string example_field
}

typedef ExampleRequest TypedefExampleRequest
typedef ExampleResponse TypedefExampleResponse

service EnvironmentService {
    ExampleResponse exampleMethod(1: ExampleRequest request)
    TypedefExampleResponse typedefExampleMethod(1: TypedefExampleRequest request)
}

Each change will have a title and will be formatted in three sections. Code Change (this PR), previous generated code and new generated code.

Use Structs instead of aliases

This was needed as you can't refer to methods on aliased types. You instead need to implement the existing struct to inherit it's methods.

Code change

https://github.com/apache/thrift/blob/cc2d009a0f036209bc8f39d206e9d2b7cf19f4a1/compiler/cpp/src/thrift/generate/t_go_generator.cc#L769-L773

Old Code

type TypedefExampleRequest *ExampleRequest

func TypedefExampleRequestPtr(v TypedefExampleRequest) *TypedefExampleRequest { return &v }

type TypedefExampleResponse *ExampleResponse

func TypedefExampleResponsePtr(v TypedefExampleResponse) *TypedefExampleResponse { return &v }

New Code

type TypedefExampleRequest struct { *ExampleRequest }

func TypedefExampleRequestPtr(v TypedefExampleRequest) *TypedefExampleRequest { return &v }

type TypedefExampleResponse struct { *ExampleResponse }

func TypedefExampleResponsePtr(v TypedefExampleResponse) *TypedefExampleResponse { return &v }

For Forward TypeDefs use the original typedef name

This was needed as it wouldn't use the correct type. For example it would use ExampleRequestwhere instead the TypeDefExampleRequest should have been used.

Code change

https://github.com/apache/thrift/blob/cc2d009a0f036209bc8f39d206e9d2b7cf19f4a1/compiler/cpp/src/thrift/generate/t_go_generator.cc#L2093-L2094

Old Code

func (p *EnvironmentServiceTypedefExampleMethodArgs)  ReadField1(ctx context.Context, iprot thrift.TProtocol) error {
  p.Request = &ExampleRequest{}
  if err := p.Request.Read(ctx, iprot); err != nil {
    return thrift.PrependError(fmt.Sprintf("%T error reading struct: ", p.Request), err)
  }
  return nil
}

New Code

func (p *EnvironmentServiceTypedefExampleMethodArgs)  ReadField1(ctx context.Context, iprot thrift.TProtocol) error {
  p.Request = &TypedefExampleRequest{}
  if err := p.Request.Read(ctx, iprot); err != nil {
    return thrift.PrependError(fmt.Sprintf("%T error reading struct: ", p.Request), err)
  }
  return nil
}

Pleasse let me know if there's anything else I can clarify :)

[fishy]

Thanks. Please

1. Add a test to verify the fix
2. Claim the jira ticket (cc @dcelasun you claimed the ticket previously)
3. instead of type TypedefExampleRequest struct { *ExampleRequest }, why not just type TypedefExampleRequest = ExampleRequest? that will make things much simpler isn't it?

[dcelasun]

Thanks for the PR!

I've unassigned myself from the ticket and I agree with @fishy that an alias would be better.

[ethan-gallant]

@dcelasun and @fishy thank you for the quick response :)

I have a Jira account Ethan-Gallant but seem to lack the permissions to self-assign the issue. I left a comment on it if someone is able to assign it to me when able.

As for the struct implementation, I've added it to simplify the code ex: type MyTypedefOfString = string it also supports more complicated types like type MyTypeDefOfStruct = MyRequest.

For testing I added a service to the Go test which will cause a compile failure. I'm struggling to get the suite working locally. Do you have any other guidance aside from the existing Readme file I can reference?

[fishy]

also, thrift files under test/ are for all languages and are not necessarily run by all language tests (currently there's no go test specifically run against test/TypedefTest.thrift, it's just the compiler change you made complete broke the go code so it failed the test). for go specific tests I'd suggest you to put it under lib/go/test instead. Note that currently tests under lib/go/test is not run by CI, see https://github.com/apache/thrift/pull/2886 (after that is merged the CI will run those tests)

[ethan-gallant]

@fishy I've added a test in the lib/go/test directory with some more extensive use-cases.

Currently it will test:

• Optional typedef properties
• Typedefs for primitive types
• Typedefs for struct types
• Services using the above types (where the compile issues originated from)

[fishy]

also, now https://github.com/apache/thrift/pull/2886 is merged, please rebase your change on top of latest master so the CI will actually run your tests.

[dcelasun]

@ethan-gallant any progress on this? Happy to help with any issues.

[binalp7]

@ethan-gallant Is there any update on this? I am also facing the similar issue.