Support nested anonymous inherited structs

[Vilsol]

Is your feature request related to a problem? Please describe. Currently there is no way to convert the input to output from this example:

type Input struct {
	Hello string
	World int
}

type A struct {
	Hello string
}

type B struct {
	A
	World int
}

type Output struct {
	B
}

Describe the solution you'd like Being able to add a dot . separator for anonymous inherited structs via goverter:mapIdentity (like goverter:map for regular fields) would make this possible:

// goverter:converter
type Converter interface {
	// goverter:mapIdentity B B.A
	ConvertBase(source Input) Output
}

Describe alternatives you've considered You can resolve a single depth nested struct using this method, but it will error that it is unable to map the identity to the A struct:

// goverter:converter
type Converter interface {
	// goverter:mapIdentity B
	ConvertBase(source Input) Output
}

Error while creating converter method:
    func (db/internal/types.Converter).ConvertBase(source db/internal/types.Input) db/internal/types.Output

| db/internal/types.Input
|
|      | db/internal/types.Input
|      |
|      |
|      |
source.<mapIdentity>.???
target.B            .A
|      |             |
|      |             | db/internal/types.A
|      |
|      | db/internal/types.B
|
| db/internal/types.Output

Cannot set value for field A because it does not exist on the source entry.

[Vilsol]

Slight update:

Looks like defining the convertors below seem to fix it, however that adds a lot of bulk if you have deeply nested structs that are used all over the place.

// goverter:converter
type Converter interface {
	// goverter:mapIdentity A
	ConvertB(source Input) B

	// goverter:mapIdentity B
	ConvertBase(source Input) Output
}

[Vilsol]

Going through the codebase, it seems like the easiest solution would be to add this:

if ctx.IdentityMapping != nil {
	namedType := target.NamedType
	if target.Pointer {
		namedType = target.PointerInner.NamedType
	}

	if namedType != nil {
		prefix := namedType.Obj().Name() + "."
		method.IdentityMapping = make(map[string]struct{})
		for k, v := range ctx.IdentityMapping {
			if strings.HasPrefix(k, prefix) {
				method.IdentityMapping[k[len(prefix):]] = v
			}
		}
	}
}

to here: https://github.com/jmattheis/goverter/blob/main/generator/generator.go#L290

That would make sure it will always only trim the target prefix from the expected mappings, and then add all trimmed ones to the new identity mapping.

I am willing to make a PR of this with any changes requested.

[jmattheis]

Hey,

the current optimal solution would be the one you posted in your update:

// goverter:converter
type Converter interface {
	// goverter:mapIdentity A
	ConvertB(source Input) B

	// goverter:mapIdentity B
	ConvertBase(source Input) Output
}

The fix you proposed will work for the easier cases, but with a complex converter structure there would be a lot of undefined / buggy behaviour.

F.ex. what would take precendence, the ignore or the mapIdentity.

// goverter:converter
type Converter interface {
	// goverter:mapIdentity B B.A
	ConvertBase(source Input) Output

	// goverter:ignore A
	ConvertBase(source Input) Output
}

In short: goverter comments should not interfere with sub methods, as this may create duplicated definitions.

I don't think there is a good fix for this, without rewriting goverter to support multiple implementation with the same method signature and rewriting it just for this feature would be overkill.

[Vilsol]

In those cases maybe it would be possible to detect if a definition of the type conversion exists, and then merge their contexts?

[jmattheis]

Sure, but some definitions contradict each other meaning they can't be merged. like the one from above:

// goverter:converter
type Converter interface {
	// goverter:mapIdentity B B.A
	ConvertBase(source Input) Output

	// goverter:ignore A
	ConvertBase(source Input) Output
}

[Vilsol]

I'm not sure I understand why someone would define a double definition for the same conversion?

[jmattheis]

It doesn't really matter that someone would define it, my point that it is possible. I don't want goverter to be like "if you don't do something stupid it will probably work" I want it to be solid and predictable this means that there shouldn't be the possibility to miss configure it without any clear errors. The example above is a simple case, but it could be much more complex with a lot of nesting, then it wouldn't be this obvious.

I don't think it is worth to support this feature as merging such definitions isn't that obvious and thus, not that predictable.

[Vilsol]

Indeed this would add a lot of potential magic. However my issue is that if you are converting from a flat struct to deeply N nested anonymous structure that is reused a lot of times means every single input type will need have N handcrafted extra conversions.

Maybe an option would be to check if there is a handcrafted conversion then use that, if not, resolve a nested mapping. If both are defined, error out?

I also don't fully understand your example. Your both method signatures are identical including the name, input and output types. Assuming that the identical names are a mistake, I don't see how that example applies to the issue at hand? Did you mean to set the bottom method signature to be something like the following?

// goverter:converter
type Converter interface {
	// goverter:mapIdentity B B.A
	ConvertBase(source Input) Output

	// goverter:ignore A
	ConvertBaseToB(source Input) B
}

In which case it could error out, as you are trying to use mapIdentity on a hand-defined conversion.

[jmattheis]

Did you mean to set the bottom method signature to be something like the following?

Yes.

Maybe an option would be to check if there is a handcrafted conversion then use that, if not, resolve a nested mapping. If both are defined, error out?

This would be possible, but I don't like it. I'd be open to consider including this in goverter if more people want this feature but as of now I don't think it's worth the effort to maintain because it is a little more complicated that your example implementation above. Example which doesn't work:

// goverter:converter
type Converter interface {
	// goverter:mapIdentity WithB WithB.WithA
	ConvertBase(source Input) Output
}

type Input struct {
	Hello string
	World int
}

type A struct {
	Hello string
}

type B struct {
	WithA A
	World int
}

type Output struct {
	WithB B
}

I'll keep this issue open for more user feedback.

[Vilsol]

Indeed that example does not work, as I was only working with the presumption of anonymous nested structs.

Maybe an option would be to maintain a parent inheritance tree throughout the context (similar to Go's STD Context) which can then be queried for the current path and then match against an identity mapping?

[jmattheis]

If it would be implemented, it probably should be done similar to how you can add // goverter:map to methods which take a pointer to a struct. See https://github.com/jmattheis/goverter/commit/e921977029a98515cae5188332a3603a339dad06

[switchupcb]

@Vilsol This is possible to do in copygen, in a number of ways:

• Using the automatcher: automatch example
• Here is an example using nested structs (albeit a test case) with the automatcher: cyclic test case.
• Using the map directive: manual example

[jmattheis]

I've thought about it, with a recent change the pointer behavior described in https://github.com/jmattheis/goverter/issues/7#issuecomment-930391960 was fixed in favor of a more predictable approach, goverter:map isn't inherited to sub methods anymore. Because the goverter generation was adjusted.

Adding this would need such behavior, and I don't think it's worth supporting.