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

Mapping context values #68

Closed
jmattheis opened this issue Mar 13, 2023 · 14 comments
Closed

Mapping context values #68

jmattheis opened this issue Mar 13, 2023 · 14 comments
Labels
feature New feature or request

Comments

@jmattheis
Copy link
Owner

jmattheis commented Mar 13, 2023

Goverter should allow passing a mapping context through converter methods for custom conversions. This allows more customizability when the conversion requires other state. See also https://mapstruct.org/documentation/stable/api/org/mapstruct/Context.html

Example:

// goverter:converter
// goverter:extend LookupUser
type Converter interface {
    Convert(source Input, context UserContext) (Output, error)
}

type UserContext struct {
    UserNames map[int]string
}

func LookupUser(userID int, context UserContext) (User, error) {
    user := User{ID: userID}
    name, ok := context.UserNames[userID]
    if !ok {
        return user, fmt.Errorf("user with id %d not found", userID)
    }
    user.Name = name
    return user, nil
}

type Input struct {  UserIDs []int }
type Output struct { Users []User  }
type User struct {
    ID int
    Name string
}

Conversion contexts are additional parameters that are passed into the conversion methods (See Convert method from Converter), and they should be passed into custom methods (See LookupUser).

This feature is WIP, see #68 (comment) for instructions to try it out.

Please 👍 this issue if you like this functionality. If you have a specific use-case in mind, feel free to comment it.

@pavelpatrin
Copy link
Contributor

@jmattheis can I hope that this feature will be implemented once upon a time?

@jmattheis
Copy link
Owner Author

@pavelpatrin Yes, but it doesn't have a high priority on my to-do list.

@becash
Copy link

becash commented Jun 27, 2024

Hi, a made a beta version for that , it add "arguments", is a custom structure, that can be used in internal functions, say me a opinion, maybe some improvements, for me is all what i need

https://github.com/becash/goverter
jmattheis

@jmattheis
Copy link
Owner Author

Hey @becash, it seems like the beta version has hardcoded the domain.Arguments type with a fixed position in the conversion method calls. I find this solution to inflexible, with the features #143 and #147 goverter needs a way to define arguments in a certain usage like source, target, and context.

If any of these tickets are implemented they should built the foundation to support this otherwise it probably has to be rewritten afterwards.

@becash
Copy link

becash commented Jul 4, 2024

of course is a beta,
@jmattheis, i see problems that you mention, what you thing as idea? can be improved, or maybe replaced ?

about type name , is need to put in config? also need to indicate import path, maybe can you recomend a beter way
also about "switch sig.Params().Len()" ... that im modify, i`m also thing it can me make better,
if you thing that my ideea can live, i can improve, to add in repository, but if you not agree, for me it work normal

@jmattheis
Copy link
Owner Author

Types that are context should be inferred from the conversion method signature.

// goverter:converter
type Converter interface {
	// goverter:arg:context ctxDB
	// goverter:arg:context ctxUsers
	// goverter:arg:source myInput
	Convert(ctxDB Database, myInput []Input, ctxUsers UserContext) []Output
}

Internally we can mark the Converter interface is context by default, to keep backwards compatibility.

@jmattheis
Copy link
Owner Author

The feature is WIP, you can try out it out by getting the context branch from this repository. E.g.

  • go install github.com/jmattheis/goverter/cmd/goverter@context
  • or go run github.com/jmattheis/goverter/cmd/goverter@context [pattern]
  • or go get github.com/jmattheis/goverter@context

With this version you can specify additional parameters with the prefix ctx or context to all conversion methods and extend, default and map TARGET functions. See the signature documentation here: https://github.com/jmattheis/goverter/blob/context/docs/reference/signature.md
Context params are identified by type, so you cannot add two context parameters with the same type. (this isn't validated yet)

Here is an example:

package example

import "time"

// goverter:converter
// goverter:extend FormatTime
type Converter interface {
	Convert(source map[string]Input, ctxFormat string) map[string]Output
}

func FormatTime(t time.Time, ctxFormat string) string {
	return t.Format(ctxFormat)
}

type Input struct {
	Name      string
	CreatedAt time.Time
}
type Output struct {
	Name      string
	CreatedAt string
}

You can find more examples here: https://github.com/jmattheis/goverter/blob/context/docs/reference/signature.md

It'll probably take some time until this will be merged, as I likely will include #147 in the same release because this feature also adjusts how goverter parses the conversion method signature.

Feedback is always welcome (:.

@jmattheis
Copy link
Owner Author

Fixed with v1.6.0. See Guide: Pass context to functions

@pavelpatrin
Copy link
Contributor

But why you used argument names? It is super counter intuitive.

@pavelpatrin
Copy link
Contributor

Your original proposal was about explicit data structures, but now it is about naming conventions which affects runtime. Naming should NEVER affect behavior.

@becash
Copy link

becash commented Dec 10, 2024

it can be a my mistake, i found this way first

also i think speficific strcture is easier, also is more productive, in converter appear many iterattions and there i think use some value like "arguments.Name" much faster that :

name,err := ctx.GetValue("name")
if err....
........ name.........

@becash
Copy link

becash commented Dec 10, 2024

But why you used argument names? It is super counter intuitive.
it can be a my mistake, i found this way first

@jmattheis
Copy link
Owner Author

But why you used argument names? It is super counter intuitive.

Go doesn't really have that many features to reflect the type (source, context) of a variable. If possible, I prefer a solution like

alias Context[T any] = T

func FormatTime(t time.Time, format Context[string]) string {
	return t.Format(format)
}

this seems to be working now in go1.23 with two debug flags, but isn't fully supported yet.

Generally goverter needs a solution that works with the conversion interface
and extend functions, meaning we need to know what parameters of a top level
functions are of type context. E.g.

func FormatTime(t time.Time, ctxFormat string) string {
	return t.Format(ctxFormat)
}

how do we know that ctxFormat is a context variable. It would be possible to
parse goverter comments on these methods like goverter:context, but
goverter currently doesn't do this anywhere for funcs, so this would be some effort.

Doing it by variable name, is easier internally, and has a similar end result.

@jmattheis
Copy link
Owner Author

After some thinking I changed the default matching by name to be consistent with update. There is now the setting context for defining an argument as context. This Guide was updated with the new version.

Matching by argument name is disabled by default but you can get the v1.6.0 behavior by configuring goverter:arg:context:regex ^ctx|^context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants