jmattheis/goverter

Mapping different subsets of properties on the same types

Opened this issue · 5 comments

isinyaaa commented

Have you read the project documentation?

  • Yes, but it does not include related information regarding my question.
  • Yes, but the steps described do not work.
  • Yes, but I am having difficulty understanding it and want clarification.

Describe your question

I'm trying to create mapping functions for merging instances, so, e.g. I have a

type Wrapper[
	M Model,
] struct {
	Existing *M
	Update   *M
}

and now, on the same converter instance, I'd like to have two separate methods for merging Update into Existing, and the other way around.

Now I notice that as soon as I have another method for wrapping a specific Model (say MyModel) one of them goes missing, so e.g. when specifying something like

	// Ignore all fields that can't be updated
	// goverter:default InitWithExisting
	// goverter:autoMap Update
	// goverter:ignore Id CreateTime LastUpdateTime Name
	UpdateExistingMyModel(source Wrapper[MyModel]) (MyModel, error)

	// Ignore all fields that ARE editable
	// goverter:default InitWithUpdate
	// goverter:autoMap Existing
	// goverter:ignore Description ExternalId CustomProperties State Owner
	OverrideNotEditableForMyModel(source Wrapper[MyModel]) (MyModel, error)

I get an error that the goverter struct doesn't properly implement the interface.

Source code
Provided above.

Errors

cannot use &generated.OpenAPIConverterImpl{} (value of type *generated.OpenAPIConverterImpl) as type converter.OpenAPIConverter in struct literal:
        *generated.OpenAPIConverterImpl does not implement converter.OpenAPIConverter (missing OverrideNotEditableForRegisteredModel method)
jmattheis commented

Good catch! Goverter uses the method signature to identify methods. While parsing the methods from the interface, the second method will override the first one, causing a missing method and not satisfying the interface.

This is mostly because of method re-usage. E.g. given this

// goverter:converter
type Converter interface {
	Convert(Input) Output
	Convert2(Input2) Output

	ConvertNested(NestedInput) NestedOutput
}

type Input struct {
	Name   string
	Nested NestedInput
}
type Input2 struct {
	Name   string
	Nested NestedInput
}

type Output struct {
	Name   string
	Nested NestedOutput
}

type NestedInput struct{ Value string }
type NestedOutput struct{ Value string }

goverter will generate this code

func (c *ConverterImpl) Convert(source nestedstruct.Input) nestedstruct.Output {
	var exampleOutput nestedstruct.Output
	exampleOutput.Name = source.Name
	exampleOutput.Nested = c.ConvertNested(source.Nested)
	return exampleOutput
}
func (c *ConverterImpl) Convert2(source nestedstruct.Input2) nestedstruct.Output {
	var exampleOutput nestedstruct.Output
	exampleOutput.Name = source.Name
	exampleOutput.Nested = c.ConvertNested(source.Nested)
	return exampleOutput
}
func (c *ConverterImpl) ConvertNested(source nestedstruct.NestedInput) nestedstruct.NestedOutput {
	var exampleNestedOutput nestedstruct.NestedOutput
	exampleNestedOutput.Value = source.Value
	return exampleNestedOutput
}

The generated method ConvertNested is used by both conversion methods. When there would be two methods with the same signature similar to your example, then goverter doesn't know which method to use.

Supporting multiple root methods with the same signature can probably be done without too much effort and then goverter could error when the described ambiguity occurs to prevent unexpected behavior. Would this cover your use-case?

Along with #80 goverter need a kind of method flavoring, to have different versions of methods with the same signature and have a way to choose which methods should be used when doing nested conversions. These flavored methods could be used to chose between multiple methods when there is an ambiguity.

Your Wrapper generic looks like you want to copy values to an existing instance of a type. Do you think a "copy to" feature along the lines of this would be useful to you?

    // goverter:ignore Id CreateTime LastUpdateTime Name
    CopyTo(source *MyModel, target *MyModel) (error)

Goverter would apply all non ignored properties from source to target.

isinyaaa commented

Good catch! Goverter uses the method signature to identify methods. While parsing the methods from the interface, the second method will override the first one, causing a missing method and not satisfying the interface.

This is mostly because of method re-usage.
...
The generated method ConvertNested is used by both conversion methods. When there would be two methods with the same signature similar to your example, then goverter doesn't know which method to use.

Thanks for the clarification! Happy to know I guessed on the right direction. This behavior is pretty cool for a generator :)

Supporting multiple root methods with the same signature can probably be done without too much effort and then goverter could error when the described ambiguity occurs to prevent unexpected behavior. Would this cover your use-case?

Along with #80 goverter need a kind of method flavoring, to have different versions of methods with the same signature and have a way to choose which methods should be used when doing nested conversions. These flavored methods could be used to chose between multiple methods when there is an ambiguity.

Your Wrapper generic looks like you want to copy values to an existing instance of a type. Do you think a "copy to" feature along the lines of this would be useful to you?

    // goverter:ignore Id CreateTime LastUpdateTime Name
    CopyTo(source *MyModel, target *MyModel) (error)

Goverter would apply all non ignored properties from source to target.

I'd say that a "copy to" would actually be better than extending support for multiple root (flavored) methods. To me it looks cleaner to have that interface. Although that brings me to another question: I see that in the generated code for nested structs goverter only checks if the high-level ref is not nil, but will map the fields independently of them being nil. So it actually wouldn't work for my purposes even if I define two separate converters. Is there a way to handle that use-case currently?

jmattheis commented

I'd say that a "copy to" would actually be better than extending support for multiple root (flavored) methods. To me it looks cleaner to have that interface.

I've created #147 for the copy to functionality.

Is there a way to handle that use-case currently?

With the current release that's not possible. This limitation was talked about in #96 (comment) and #97 tracks the issue. I've a wip branch from a couple of month ago for this called assign-not-nil, could you check if this fixes your problem?

  • go install github.com/jmattheis/goverter/cmd/goverter@assign-not-nil
  • or go run github.com/jmattheis/goverter/cmd/goverter@assign-not-nil [pattern]
  • or go get github.com/jmattheis/goverter@assign-not-nil

I think there is one bug left or I'm not really sure what to do there. If you convert a map[K]*T to map[K]*T then the new impl won't copy the value to the target map, if the *T on the source map entry is nil. See Convert2 in https://github.com/jmattheis/goverter/blob/assign-not-nil/scenario/gomap_primitive_pointer.yml I'd expect it to similar to:

func (c *ConverterImpl) Convert2(source map[string]*int) map[string]*int {
    var mapStringPInt map[string]*int
    if source != nil {
        mapStringPInt = make(map[string]*int, len(source))
        for key, value := range source {
            if value != nil {
                xint := *value
                mapStringPInt[key] = &xint
            } else {
                mapStringPInt[key] = nil
            }
        }
    }
    return mapStringPInt
}
isinyaaa commented

I'd say that a "copy to" would actually be better than extending support for multiple root (flavored) methods. To me it looks cleaner to have that interface.

I've created #147 for the copy to functionality.

Thanks! Already upvoted.

Is there a way to handle that use-case currently?

With the current release that's not possible. This limitation was talked about in #96 (comment) and #97 tracks the issue. I've a wip branch from a couple of month ago for this called assign-not-nil, could you check if this fixes your problem?

  • go install github.com/jmattheis/goverter/cmd/goverter@assign-not-nil
  • or go run github.com/jmattheis/goverter/cmd/goverter@assign-not-nil [pattern]
  • or go get github.com/jmattheis/goverter@assign-not-nil

It works fine!

I think there is one bug left or I'm not really sure what to do there. If you convert a map[K]*T to map[K]*T then the new impl won't copy the value to the target map, if the *T on the source map entry is nil. See Convert2 in https://github.com/jmattheis/goverter/blob/assign-not-nil/scenario/gomap_primitive_pointer.yml I'd expect it to similar to:

func (c *ConverterImpl) Convert2(source map[string]*int) map[string]*int {
    var mapStringPInt map[string]*int
    if source != nil {
        mapStringPInt = make(map[string]*int, len(source))
        for key, value := range source {
            if value != nil {
                xint := *value
                mapStringPInt[key] = &xint
            } else {
                mapStringPInt[key] = nil
            }
        }
    }
    return mapStringPInt
}

Maybe you could only override if mapStringPInt[key] is not present? That should work when using a default as well. Or having a flag in case we want to preserve a nil override.

jmattheis commented

It works fine!

Great. I'll release this soon, likely on the weekend.

Maybe you could only override if mapStringPInt[key] is not present?

Yeah, sounds valid, but I'll wait until someone has a concrete problem for this before adding a setting. For now I'll let goverter override existing default values with nil in maps because this is also how goverter behaves currently, then there is no breaking change.

  • 🎉1