jmattheis / goverter

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

Converting struct with time.Time fields generate invalid converter #4

Closed matdurand closed 3 years ago

matdurand commented 3 years ago

Describe the bug When generating a converter between two struct with time.Time fields, the generated converter doesn't compile.

To Reproduce Here is the code to generate the issue

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

import "time"

// goverter:converter
type Converter interface {
    Convert(source []Input) []Output
}

type Input struct {
    Name     string
    Birthday time.Time
    Age      int
}

type Output struct {
    Name     string
    Birthday time.Time
    Age      int
}

Expected behavior Since the source and destination types are both time.Time, I would expect a simple assignment to be done in the converter, but instead, I have a timeLocationToTimeLocation and timeTimeToTimeTime methods generated and trying to play with private fields of the time type.

matdurand commented 3 years ago

I was able to bypass the issue by registering a custom converter

// goverter:converter
// goverter:extend TimeToTime
type Converter interface {
  ...
}

func TimeToTime(t time.Time) time.Time {
    return t
}
jmattheis commented 3 years ago

Yes, this is exactly how it should be done (:. goverter only converts primitive types and "data" structs, if you've a struct with private field you most likely need to create a custom converter function.

matdurand commented 3 years ago

But could it be possible to give an error instead of generating invalid code? Generated code cannot play with private fields anyway because the converter is not in the same package so is there cases where this kind of generated code would work?

jmattheis commented 3 years ago

Yeah, an error would be useful. Currently goverter treats the time.Time struct like every other struct. It tries to map exported fields and ignores private fields.

matdurand commented 3 years ago

Seems to me like it also tries to map private fields? Here is the generated converter for time.Time

func (c *ConverterImpl) timeTimeToTimeTime(source time.Time) time.Time {
    var timeTime time.Time
    timeTime.wall = source.wall
    timeTime.ext = source.ext
    var pTimeLocation *time.Location
    if source.loc != nil {
        timeLocation := c.timeLocationToTimeLocation(*source.loc)
        pTimeLocation = &timeLocation
    }
    timeTime.loc = pTimeLocation
    return timeTime
}

We can see it tries to access .wall, .ext and .loc which are all private fields. Am I missing something?

jmattheis commented 3 years ago

goverter will now fail, if there are unexported fields:

https://github.com/jmattheis/goverter/blob/0b2e20a09b20ff4a9a35612b7795c44aaab71cf2/scenario/3_struct_unexported.yml#L20-L55

matdurand commented 3 years ago

It works great. Thanks!