Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flatten nested fields #3

Closed
1 of 3 tasks
matdurand opened this issue Aug 12, 2021 · 19 comments
Closed
1 of 3 tasks

Flatten nested fields #3

matdurand opened this issue Aug 12, 2021 · 19 comments
Labels
question Further information is requested

Comments

@matdurand
Copy link

Have you read the project readme?

  • 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

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
}
@matdurand matdurand added the question Further information is requested label Aug 12, 2021
@jmattheis
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

@matdurand
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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

Thanks again.

@matdurand
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

matdurand commented Aug 26, 2021

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

@jmattheis
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

Let me think a little about this problem.

@jmattheis
Copy link
Owner

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

@switchupcb
Copy link

@matdurand This is simple to do in copygen:

@kliko9
Copy link

kliko9 commented Apr 26, 2022

@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
Copy link
Owner

@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
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants