copygen icon indicating copy to clipboard operation
copygen copied to clipboard

matcher: Intended Private Member Field Behavior is Incorrect

Open kb-sp opened this issue 2 years ago • 12 comments
trafficstars

I like the concept of copygen, where in theory the generated could approach ~20x faster than a json.Marshal -> protojson.Unmarshal sandwich (and infinitely faster than jinzhu/copier).

However, when using on structs that aren't flat with concrete builtins, things seem to go a little sideways.

Is there something I'm missing on how to handle non-concrete builtin types?

Setup

By default, Created will be ignored because its name/type combo don't match between the structs, which makes sense.

However, with tag .* json (which is required in my case) or adding a type override, copygen performs unexpectedly:

YML

generated:
  setup: ./gen.go
  output: ./generated/generated.go

Go

package copygen

import (
	"time"

	"google.golang.org/protobuf/runtime/protoimpl"
	"google.golang.org/protobuf/types/known/timestamppb"
)

type Copygen interface {
	EntityToPB(E *Entity) *EntityPB
	EntityToPBCreated(E *Entity, Created *timestamppb.Timestamp) *EntityPB
	// tag .* json
	EntityToPBTag(E *Entity) *EntityPB
	// tag .* json
	EntityToPBCreatedTag(E *Entity, Created *timestamppb.Timestamp) *EntityPB
}

type Entity struct {
	Created *time.Time `json:"created,omitempty"`
	Name    string     `json:"name,omitempty"`
}

type EntityPB struct {
	state         protoimpl.MessageState
	sizeCache     protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	Created *timestamppb.Timestamp `protobuf:"bytes,2,opt,name=created,proto3,oneof" json:"created,omitempty"`
	Name    string                 `protobuf:"bytes,4,opt,name=name,proto3" json:"name,omitempty"`
}

Generation (gofmted for better readability)

// Code generated by github.com/switchupcb/copygen
// DO NOT EDIT.

package copygen

import (
	"time"

	"google.golang.org/protobuf/runtime/protoimpl"
	"google.golang.org/protobuf/types/known/timestamppb"
)

type EntityPB struct {
	state         protoimpl.MessageState
	sizeCache     protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	Created *timestamppb.Timestamp `protobuf:"bytes,2,opt,name=created,proto3,oneof" json:"created,omitempty"`
	Name    string                 `protobuf:"bytes,4,opt,name=name,proto3" json:"name,omitempty"`
}
type Entity struct {
	Created *time.Time `json:"created,omitempty"`
	Name    string     `json:"name,omitempty"`
}

// EntityToPB copies a *Entity to a *EntityPB.
func EntityToPB(tE *EntityPB, fE *Entity) {
	// *EntityPB fields
	tE.Name = fE.Name
}

// EntityToPBCreated copies a *Entity, *timestamppb.Timestamp to a *EntityPB.
func EntityToPBCreated(tE *EntityPB, fE *Entity, fT *timestamppb.Timestamp) {
	// *EntityPB fields
	tE.state = fT.state
	tE.sizeCache = fT.sizeCache
	tE.unknownFields = fT.unknownFields
	tE.Created = fT
	tE.Name = fE.Name
}

// EntityToPBTag copies a *Entity to a *EntityPB.
func EntityToPBTag(tE *EntityPB, fE *Entity) {
	// *EntityPB fields
	tE.Created = tE.Name = fE.Name
}

// EntityToPBCreatedTag copies a *Entity, *timestamppb.Timestamp to a *EntityPB.
func EntityToPBCreatedTag(tE *EntityPB, fE *Entity, fT *timestamppb.Timestamp) {
	// *EntityPB fields
	tE.Created.Seconds = fT.Seconds
	tE.Created.Nanos = fT.Nanos
	tE.Created = tE.Name = fE.Name
}

Errors

	tE.state = fT.state

Copying private member fields isn't going to end well. ;-)

	tE.Created = tE.Name = fE.Name
// ...
	tE.Created = tE.Name = fE.Name

It seems like custom fields confuses the internal type-matching logic.

Operating System: darwin/arm64 or linux/amd64 Copygen Version: latest == v0.4.0

kb-sp avatar Aug 20 '23 07:08 kb-sp

Problem: Private Member Field Behavior

Copygen copies the private member fields using the automatcher because EntityPB.state has an equivalent name and type to the timestamppb.Timestamp.state field. This condition is also true the subsequent matches of private member fields in EntityToPBCreated.

So Copygen performs as expected in this case.

With that said, this issue proposes the question on whether Copygen should attempt to match these private member fields. The answer is that matching private member fields is applicable in certain cases, but not always applicable. Therefore, Copygen should not match (copy) private member fields from objects that aren't defined in the generated output package.

Solution

  • [ ] Update the matcher at https://github.com/switchupcb/copygen/blob/main/cli/matcher/matcher.go#L54C1-L55C1 to implement the intended private member field logic using field.Package and the generated outputPkgPath (similar to https://github.com/switchupcb/copygen/blob/main/cli/parser/parse.go#L75).
  • [ ] Add a test to multi.

Alternatively

  • [ ] Add a field option that indicates whether the field option should be used in the matcher.
  • [ ] Set this field option when the field.Package is being set at https://github.com/switchupcb/copygen/blob/main/cli/parser/parse.go#L190.
  • [ ] Check for this field option in the Match{} function.
  • [ ] Add a test to multi.

switchupcb avatar Aug 21 '23 17:08 switchupcb

Problem: Tag Behavior

I cannot confirm whether the specified tagging error exists as you are using an outdated version of Copygen; where tag logic was modified in https://github.com/switchupcb/copygen/commit/d4722f41f1ad3d263d4c28f3e9fa779d97bb1687.

Please update to the latest version using go install github.com/switchupcb/copygen@main or go install github.com/switchupcb/copygen@update and try again.

switchupcb avatar Aug 21 '23 17:08 switchupcb

Installing copygen at main doesn't seem to change the behavior with the above test case:

an error occurred while formatting the generated code.
/Users/kb/go/src/github.com/StackPackCo/shishito/cp/generated/generated.go:43:22: expected '==', found '=' (and 1 more errors)

kb-sp avatar Aug 21 '23 18:08 kb-sp

I will be able to address these issues in 1 - 3 weeks with the next update of Copygen.

switchupcb avatar Aug 28 '23 20:08 switchupcb