Support for getters & setters (for gRPC Opaque API)

[applejag]

Is your feature request related to a problem? Please describe.
Support for this:

type Foo struct {
	name string
}

func (f *Foo) GetName() string { return f.name }

func (f *Foo) SetName(name string) { f.name = name }


type Bar struct {
	Name string
}


type Converter interface {
	ConvertFoo(foo Foo) Bar
	ConvertBar(bar Bar) Foo
}

This is the general idea of how Go's new gRPC Opaque API works: https://go.dev/blog/protobuf-opaque

But when trying this I get this:

func (c *ConverterImpl) ConvertFoo(source models.Foo) models.Bar {
	var modelsFoo models.Foo
	_ = source
	return modelsFoo
}

Describe the solution you'd like
goverter should try look for Get{{ .Name }} and Set{{ .Name }} if no field is found.

Describe alternatives you've considered
nothing comes to mind

Please 👍 this issue if you like this functionality. If you have a specific use-case in mind, feel free to comment it.

[jmattheis]

yeah, goverter should support getter and setters.

These questions are more of a list of todos for me, you don't have to answer them, but opinions are welcome (:.

• Do we want to map both fields and setter if they are present, or do we want to skip mapping the setter if there are struct fields.
  • maybe we can have a setting something like, "prefer field", "prefer setter", and "map both setter and fields"
    in "prefer field", we map by field, and if there aren't any, try to map by setter.
• Do we want to use setter when there is a getter missing?
• Should this be behind a feature flag? (I guess "prefer field", would be a sensible default which would cover your use-case)
• Should the pattern for getter / setter be configure (Get.* and Set.*)?
• Should we support the Is{{.name}} pattern for booleans.

[applejag]

yeah, goverter should support getter and setters.

These questions are more of a list of todos for me, you don't have to answer them, but opinions are welcome (:.

• Do we want to map both fields and setter if they are present, or do we want to skip mapping the setter if there are struct fields.

I think if the field is available then prefer the field. Mixing sounds like a bad default. Having it configurable would be good though.

• maybe we can have a setting something like, "prefer field", "prefer setter", and "map both setter and fields"
  in "prefer field", we map by field, and if there aren't any, try to map by setter.

Ye that would be good for when you have a mix of exported fields and some fields behind getters and setters. By default it should prefer fields if available though.

• Do we want to use setter when there is a getter missing?

Yes.

• Should this be behind a feature flag? (I guess "prefer field", would be a sensible default which would cover your use-case)

I think it should work out of the box and try find a field or a setter. Having to opt in to setters everywhere would become tedious. Better with an opt out strategy IMO.

• Should the pattern for getter / setter be configure (Get.* and Set.*)?

Yes, as that would open this feature up to allow other strategies. Maybe it could be added into the // goverter:map as its quite similar conceptually, instead of adding a new //goverter:getter config.

It's quite common to have the getter just be the field name and the setter has the "Set" prefix. So by default it should look for both with and without the "Get" prefix.

• Should we support the Is{{.name}} pattern for booleans.

As a fallback this might actually make sense. So goverter would pick an Is... function if it can't find a field, getter with "Get" prefix, nor getter without prefix. That could allow fewer comments.

---

Additionally, I came up with 2 new considerations:

1. I wonder how to have stuff like context variables support? Would probably require you to annotate the setters/getters, but that is not always possible with third-party or generated code (as with gRPC code). Spontaneously I don't have any ideas for that.
2. If both source and target uses setters & getters then figuring out which methods are for "fields" and which are just plain functions?

---

Great tool btw! I'm considering contributing as I'd like to have this tool more "plug and play", as today it feels quite limiting without going down the route of adding a bunch of comments. I'll create some issues of my pain point that I got from trying it out.

[jmattheis]

I think it should work out of the box and try find a field or a setter. Having to opt in to setters everywhere would become tedious. Better with an opt out strategy IMO.

Currently, goverter can convert unexported fields given the correct output package is specified where these fields can be accessed. It could be unclear if goverter should error with a field cannot be accessed error, or try to use setters. Especially if manual goverter:map are defined for unexported fields. Tho I'm not sure if that's actually used.

It's quite common to have the getter just be the field name and the setter has the "Set" prefix. So by default it should look for both with and without the "Get" prefix.

Goverter already uses methods on the struct if the name is the same. So basically the first Foo -> Bar conversion already does work with a goverter:map

// goverter:converter
type Converter interface {
	// goverter:map GetName Name
	ConvertFoo(foo Foo) Bar
}

Maybe it could be added into the // goverter:map as its quite similar conceptually

How do you mean this, could you give an example?

I wonder how to have stuff like context variables support? Would probably require you to annotate the setters/getters, but that is not always possible with third-party or generated code (as with gRPC code). Spontaneously I don't have any ideas for that.

Yeah, I think this should be ignored for now, as it probably is used pretty rarely, and there are other ways to solve this with existing functionality.

If both source and target uses setters & getters then figuring out which methods are for "fields" and which are just plain functions?

I think goverter will try to satisfy every setter on the target struct, if there is something missing it will error, and then it either has to be goverter:ignore'd or mapped correctly.

Great tool btw! I'm considering contributing as I'd like to have this tool more "plug and play", as today it feels quite limiting without going down the route of adding a bunch of comments. I'll create some issues of my pain point that I got from trying it out.

Awesome! Feel free to put this in one issue, and then we can split it into smaller ones after discussing the use-cases.

[applejag]

It's quite common to have the getter just be the field name and the setter has the "Set" prefix. So by default it should look for both with and without the "Get" prefix.

Goverter already uses methods on the struct if the name is the same. So basically the first Foo -> Bar conversion already does work with a goverter:map

// goverter:converter
type Converter interface {
// goverter:map GetName Name
ConvertFoo(foo Foo) Bar
}

Perfect! Though what I'm advocating for is that we should not need the map option for this super common case.

When working with gRPC opaque types generated code then we'd need to have one // goverter:map for each field, which kind of defeats the purpose of using goverter when we have to specify everything explicitly.

Maybe it could be added into the // goverter:map as its quite similar conceptually

How do you mean this, could you give an example?

Just wanted to make clear that it'd probably be better to stick with // goverter:map instead of introducing a new config/option thing.

For example if the user for some reason has OverrideFoo instead of SetFoo then you'd just use // goverter:map OverrideFoo Foo instead of something like // goverter:setter OverrideFoo.

Although if you're onboard with the idea of goverter finding getters/setters automatically without the need for any // goverter:map ... options, then it could perhaps make sense to allow configuring a "default pattern" on a more global level.

Such as:

// goverter:converter
// goverter:getter "Gimme{{ .Name }}"
// goverter:setter "Override{{ .Name }}"
type Converter interface {
	ConvertFoo(foo Foo) Bar
	ConvertBar(bar Bar) Foo
}

type Foo struct {
	name string
}

func (f *Foo) GimmeName() string { return f.name }

func (f *Foo) OverrideName(name string) { f.name = name }

type Bar struct {
	Name string
}

^ where goverter:setter config would default to // goverter:setter "Set{{ .Name }}". For supporting multiple patterns then the getter default would probably have to be: // goverter:getter "{{ .Name }}" "Get{{ .Name }}"

If both source and target uses setters & getters then figuring out which methods are for "fields" and which are just plain functions?

I think goverter will try to satisfy every setter on the target struct, if there is something missing it will error, and then it either has to be goverter:ignore'd or mapped correctly.

Yeah, nice then we're on the same page on that. But what I meant was this:

// goverter:converter
type Converter interface {
	ConvertSource(source Source) Target
}

type Source struct {
	name string
}

func (s *Source) GetName() string { return s.name }

type Target struct {
	name string
}

func (t *Target) SetName(name string) string { t.name = name }

This should generate something like this:

func (c ConverterImpl) ConvertSource(source Source) Target {
	var target Target
	target.SetName(source.GetName())
	return target
}

But the question is how would goverter know that Name is a field to map?

Perhaps this could be ignored in the initial iteration with a note in the docs stating that getters/setters are not supported if both the source and target use getters/setters.

[jmattheis]

Perfect! Though what I'm advocating for is that we should not need the map option for this super common case.

Yeah, agreed. Only wanted to showcase that field methods already are supported internally, so that only the automatic mapping is needed there.

For example if the user for some reason has OverrideFoo instead of SetFoo then you'd just use // goverter:map OverrideFoo Foo instead of something like // goverter:setter OverrideFoo.

Understood, and agree. It also must be included into :map, because Custom Methods must continue to work with method setters / getters.E.g.

// goverter:converter
type Converter interface {
    // goverter:map Foo OverrideFoo | AdjustFoo
    SomeConvert(foo Foo) Bar
}

func AdjustFoo(value any) any {
    // ...
}

Although if you're onboard with the idea of goverter finding getters/setters automatically without the need for any // goverter:map ... options, then it could perhaps make sense to allow configuring a "default pattern" on a more global level.

Yeah definitly, the other usecase I think could be common is With{{.Name}} as pattern for builders, and making the pattern configurable shouldn't be much added effort.

I'd imagine the settings either to be a prefix

// goverter:map:setter:prefix Set
// goverter:map:getter:prefix Get

Or a pattern and a format, to both get the name from a setter, and create a setter from a name.

// goverter:map:setter:regex Set(.*)
// goverter:map:setter:format Set%s
// goverter:map:getter:regex Get(.*)
// goverter:map:getter:format Get%s

prefix seems to be easier to understand but less powerful than the regex, but probably good enough. And regex way could be added in the future while still supporting the prefix settings.

But the question is how would goverter know that Name is a field to map?

It'll probably have a logic like this:

switch mappingStragegy {
case "prefer field":
    fields := collectFieldNames(targetStruct)
    if len(fields) == 0 {
        fields = collectSetters(targetStruct)
    }
case "prefer setters"
    fields := collectSetterNames(targetStruct)
    if len(fields) == 0 {
        fields = collectFields(targetStruct)
    }
case "both":
    fields := collectSetters(targetStruct)
    fields = append(fields, collectFields(targetStruct))
}

func collectSetterNames(structType) {
    var setters []string
    for _, method := range structType.Methods() {
        if match := method.Name.Matches(/Set(.*)/); matches {
            setters = append(match.group(1))
        }
    }
    return setters
}

the fields list will then be tried to map,

• if no field exists on the source struct, then goverter tries Get{{.Name}}
• if no field exists on the target struct, then goverter tries Set{{.Name}}

Perhaps this could be ignored in the initial iteration

Without having the possibility to get all possible field names from the setters of a struct, goverter cannot determine if all setters are satisfied, so I see this as required in the initial iteration.

[pot-code]

seems its impossible to map getter().field to another name, i have the situation where the target value could be set from the result struct field returned from getter

[jmattheis]

@pot-code please create a new issue for this with a concrete example.

[pot-code]

@pot-code please create a new issue for this with a concrete example.

hmmm...i can add another method to make it work so it's not a big issue, i thought it was not implemented for some reason and does it turn out to be a bug?

[jmattheis]

Not a bug, just not implemented yet.

[morigs]

Hello, @jmattheis
Thanks for your work!

Do I understand correctly that there is currently workaround by using // goverter:map? However it requires specifying getter name for every field? IMO this makes it useless because when you have to specify each field it's easier just write mapper manually.

What's required to make it lookup for getters automatically? Is it hard? If not perhaps I could contribute

[morigs]

Also I have a question regarding mapping of oneof in grpc. Is this supported?

[jmattheis]

@morigs Yeah, that's currently the only way. It's kinda hard to find a good solution that functions intuitively and will work together with other proposed features. I don't accept contributions for this feature.

Also I have a question regarding mapping of oneof in grpc. Is this supported?

I'm not sure what's needed for this. Please create a new issue with information how oneof works and how it should be converted.