jmattheis / goverter

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

Flatten nested fields #3

Closed matdurand closed 3 years ago

matdurand commented 3 years ago

Have you read the project readme?

Describe your question

If I want to map the following scenario, do I need to go custom or is there a way to do that using goverter:map ? I basically want to flatten the nested Address field at the root of APIPerson.

// goverter:converter
type Converter interface {
    ConvertPerson(source Person) APIPerson
}

type Person struct {
    Name    string
    Address Address
}

type Address struct {
    Civic  string
    Street string
    City   string
}

type APIPerson struct {
    Name   string
    Civic  string
    Street string
    City   string
}
jmattheis commented 3 years ago

You could let goverter generate a converter from Address to APIPerson and then manually set .Name on the return value.

// goverter:converter
type Converter interface {
    // goverter:ignore Name
    ConvertAddress(source Address) APIPerson
}

Goverter could support flatting, but I'd only do this if this is a general use-case.

It probably could be implemented with a separate comment flag like this:

// goverter:converter
type Converter interface {
    // goverter:flatten
    ConvertAddress(source Person) APIPerson
}
matdurand commented 3 years ago

Thanks for the quick answer, and great job with the project btw.

Honestly I'm not sure that flattening is a generic enough use case to have it's own flag. Coming from the Java world, I used Mapstruct in the past (which does exactly the same thing you did with goverter), and nested mapping is what I had in mind (see https://mapstruct.org/documentation/stable/reference/html/#controlling-nested-bean-mappings).

Could we do something like this with the :map flag?

type Converter interface {
        //goverter:map Address.Civic Civic
        //goverter:map Address.Street Street
        //goverter:map Address.City City
    ConvertPerson(source Person) APIPerson
}
jmattheis commented 3 years ago

Yeah, this seems like a better solution. I'm currently thinking about how pointer to structs should be handled.

If you f.ex. have something like this:

// goverter:converter
type Converter interface {
    //goverter:map Address.Civic Civic
    //goverter:map Address.Street Street
    //goverter:map Address.City City
    ConvertPerson(source Person) APIPerson
}

type Person struct {
    Name    string
    Address *Address
}

type Address struct {
    Civic  string
    Street string
    City   string
}

type APIPerson struct {
    Name   string
    Civic  *string
    Street *string
    City   *string
}

Should this work, or fail because the types in Address.Street & APIPerson.Street doesn't match.

matdurand commented 3 years ago

On the top of my head, if Person.Address is nil, I would expect APIPerson.Civic,Street,City to be set to nil. But if we were to change APIPerson to just this

type APIPerson struct {
    Name   string
    Civic  string
    Street string
    City   string
}

then does it make sense to set to empty string if Person.Address is nil? I'm not sure ...

I just tested with your current implementation and it seems that I'm able to map from string to string pointer, but not the reverse. Is there a reason for that?

jmattheis commented 3 years ago

Yes, the reason would be that you'd lose information, a nil string isn't equal to a empty string. If you want such a conversion, then you'd have to create a custom mapping function

func StringPToString(value *string) string {
    if value == nil {
        return ""
    }
    return value
}

I think, I'll first add the map function for nested structs without for support that the nested elements can be pointers.

jmattheis commented 3 years ago

@matdurand Could you give https://github.com/jmattheis/goverter/releases/tag/v0.2.0 a try? I've added your example as test -> https://github.com/jmattheis/goverter/blob/main/scenario/3_struct_mapping_nested.yml

matdurand commented 3 years ago

Hi,

it works with my initial use case, but it fails when I try with slices instead.

Works:

// goverter:converter
type Converter interface {
    // goverter:map Address.Civic Civic
    // goverter:map Address.Street Street
    // goverter:map Address.City City
    ConvertPerson(source Person) APIPerson
}

Doesn't work:

// goverter:converter
type Converter interface {
    // goverter:map Address.Civic Civic
    // goverter:map Address.Street Street
    // goverter:map Address.City City
    ConvertPerson(source []Person) []APIPerson
}

But nice job, and thanks for the quick update!

jmattheis commented 3 years ago

No problem (:. goverter:map is only allowed on structs. You can define both methods, the ConvertPersons method will use ConvertPerson.

// goverter:converter
type Converter interface {
    // goverter:map Address.Civic Civic
    // goverter:map Address.Street Street
    // goverter:map Address.City City
    ConvertPerson(source Person) APIPerson

    ConvertPersons(source []Person) []APIPerson
}
matdurand commented 3 years ago

Tried it and it works. We can close this issue then.

Thanks again.

matdurand commented 3 years ago

Hey @jmattheis,

It seems like the reverse case doesn't work

//go:generate go run github.com/jmattheis/goverter/cmd/goverter github.com/jmattheis/goverter/example/simple
package simple

// goverter:converter
type Converter interface {
    // goverter:map Civic Address.Civic
    // goverter:map Street Address.Street
    // goverter:map City Address.City
    ConvertPerson(source Person) APIPerson
}

type Person struct {
    Name   string
    Civic  string
    Street string
    City   string
}

type APIPerson struct {
    Name    string
    Address APIAddress
}

type APIAddress struct {
    Civic  string
    Street string
    City   string
}

I'm getting:

| github.com/jmattheis/goverter/example/simple.Person
|
|
|
source.???
target.Address
|      |
|      | github.com/jmattheis/goverter/example/simple.APIAddress
|
| github.com/jmattheis/goverter/example/simple.APIPerson
jmattheis commented 3 years ago

Yes, this doesn't work because there are just too many edge cases to handle. Goverter would need to verify that all properties of address are set and there could be name clashes if Person also had an Address property.

matdurand commented 3 years ago

Any way we could register a Person->APIAddress converter and reuse that in the main Person->APIPerson converter?

jmattheis commented 3 years ago

You can define the method yourself:

// goverter:converter
type Converter interface {
    ConvertAddress(source Person) APIAddress
}

func ConvertPerson(c Converter, source Person) APIPerson {
    address := c.ConvertAddress(source)
    return APIPerson{
        Address: address,
        Name:    source.Name,
    }
}

If you extend the converter with your ConvertPerson custom impl, then goverter will use it. F.ex this will allow you to convert lists of Person -> APIPerson.

// goverter:converter
// goverter:extend ConvertPerson
type Converter interface {
    ConvertAddress(source Person) APIAddress
    ConvertPersons(source []Person) []APIPerson
}
matdurand commented 3 years ago

Ok, but I would say, not good enough, because if there was more field that just name, then I have to basically code a whole converter by hand, which is what I want to avoid using goverter in the first place.

I trust you when you say that there is too many edge cases to handle, so instead, could we instruct goverter to use a specific converter for a specific field? Something like the extend instruction, but for specific fields?

// goverter:converter
type Converter interface {
    // goverter:map Address use:ConvertAddress
    ConvertPerson(source Person) APIPerson
    ConvertAddress(source Person) APIAddress
}

The beauty is that there is zero custom code to write. ConvertAddress is just fine, because each fields has a one-to-one match with Person, and ConvertPerson would also be happy, because the Address field would have a match.

I'm not saying we should do it exactly like that, because you probably don't want to introduce new features like that without thinking about consequences, but I cannot see any downsides to that method after 10 min thinking about it.

jmattheis commented 3 years ago

Let me think a little about this problem.

jmattheis commented 3 years ago

@matdurand I've released v0.3.0 with support for goverter:mapIdentity see https://github.com/jmattheis/goverter#struct-identity-mapping

switchupcb commented 2 years ago

@matdurand This is simple to do in copygen:

kliko9 commented 2 years ago

@jmattheis this goverter:mapIdentity looks promising, however how can I additionally use name mapping in case of Person.Street -> APIPerson.Address.StreetName for example.

jmattheis commented 2 years ago

@kamilez Goverter will traverse the types until it finds primitive types which can be directly assigned. If you use mapIndentity, goverter will create / use methods that match the new signature. In this case APIPerson.Address is of type APIAddress, so you can add a method with a goverter:map with a conversion of Person to APIAddress.

// goverter:converter
type Converter interface {
    // goverter:mapIdentity Address
    ConvertPerson(source Person) APIPerson

    // goverter:map Name StreetName
    ConvertAddress(source Person) APIAddress
}

type Person struct {
    Name   string
    Street string
    City   string
}

type APIPerson struct {
    Name    string
    Address APIAddress
}

type APIAddress struct {
    StreetName string
    City       string
}