jmattheis / goverter

Generate type-safe Go converters by simply defining an interface
https://goverter.jmattheis.de/
MIT License
525 stars 47 forks source link

Mapping different subsets of properties on the same types #146

Open isinyaaa opened 5 months ago

isinyaaa commented 5 months ago

Have you read the project documentation?

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 5 months ago

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 5 months ago

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 5 months ago

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 https://github.com/jmattheis/goverter/pull/96#discussion_r1381985194 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?

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 5 months ago

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 5 months ago

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.

jmattheis commented 4 days ago

This will be partly fixed by #166. Goverter will disallow defining converter methods with the same signature. This limitation could be lifted in a later release, but for now this is unsupported.

With #166 there is a workaround allowing the functionality but you have to provide two different context types. E.g.

type UpdateMarker struct{}
type OverrideMarker struct{}

// goverter:converter
type Converter interface {
    // goverter:map One Target
    ConvertFromOne(source Input, ctx UpdateMarker) Output
    // goverter:map Two Target
    ConvertFromTwo(source Input, ctx OverrideMarker) Output
}

type Input struct {
    One int
    Two int
}
type Output struct {
    Target int
}
Generated Code (click me)

```go import example "goverter/example" type ConverterImpl struct{} func (c *ConverterImpl) ConvertFromOne(source example.Input, context example.UpdateMarker) example.Output { var exampleOutput example.Output exampleOutput.Target = source.One return exampleOutput } func (c *ConverterImpl) ConvertFromTwo(source example.Input, context example.OverrideMarker) example.Output { var exampleOutput example.Output exampleOutput.Target = source.Two return exampleOutput } ```

What exactly is the type of the ctx doesn't matter but it must be different between the two methods.