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

Issue with using default tag with pointer type #47

Closed
hnimtadd opened this issue Dec 5, 2024 · 8 comments · Fixed by #49
Closed

Issue with using default tag with pointer type #47

hnimtadd opened this issue Dec 5, 2024 · 8 comments · Fixed by #49

Comments

@hnimtadd
Copy link

hnimtadd commented Dec 5, 2024

TL;DR: When using the mod:"default" tag with a field of type *string, if the field is nil, the library assigns it to a pointer referencing an empty string ("") instead of the defined default value.


Context:

Hi team, thank you for the great work on this library!

While integrating it into my project, I encountered an unexpected behavior and am seeking your advice.

Here’s a simplified example to illustrate the issue:

package main

import (
	"context"
	"fmt"
	"log"

	"github.com/go-playground/mold/v4/modifiers"
)

var conform = modifiers.New()

type User struct {
	Name *string `mod:"default=abc"`
}

func main() {
	user := User{}
	fmt.Println("Name:", user.Name) // Output: nil (as expected)

	err := conform.Struct(context.Background(), &user)
	if err != nil {
		log.Panic(err)
	}

	fmt.Println("Conformed Name:", *user.Name) // Output: "" (unexpected, expected "abc")
}

Problem:

The Name field in the User struct is of type *string and is configured with the mod:"default=abc" tag.

However, if the field is initially nil, the library assigns it to a pointer referencing an empty string ("") instead of a pointer referencing the expected default value ("abc").


Expected Behavior:

If the Name field is nil, it should be assigned to a pointer referencing the default value ("abc") as defined by the mod:"default=abc" tag


Investigation and proposal:

I traced the issue to the following section of code in the modifiers/multi.go file:

	case reflect.Ptr:
		fl.Field().Set(reflect.New(fl.Field().Type().Elem()))
	}

This code initializes a pointer field to point to an empty value (e.g., an empty string).

However, it does not account for the configured default value defined in the mod:"default=..." tag. As a result, the pointer field ends up pointing to an empty string instead of the specified default value.


To fix this, I suggest updating the logic to use the configured default value when initializing the pointer. Here’s a proposed modification:

case reflect.Ptr:
	value := fl.Param() // Retrieve the configured default value
	fl.Field().Set(reflect.ValueOf(&value)) // Set the field to point to the default value
}

Please let me know if this behavior is intentional or if there is a recommended way to achieve the expected behavior. Thanks in advance!

@deankarn
Copy link
Contributor

deankarn commented Dec 5, 2024

@hnimtadd given your thorough example it does appear to be a bug, but I’ll also have to check it’s been a while since I’ve touched/looked at this code 😅

It might be a while until I can check thought, I’m in the middle of moving and everything including my computers are still packed so may be a week or two before I can.

if that long goes by and I haven’t please poke me to remind me 🙏

@hnimtadd
Copy link
Author

hnimtadd commented Dec 8, 2024

Hi @deankarn , would you mind if I create a PR, and then you could review it? Since I have already implemented it in my vendor code.

@deankarn
Copy link
Contributor

deankarn commented Dec 8, 2024

Yes by all means a PR would be helpful

@hnimtadd
Copy link
Author

Hi @deankarn, could you grant me permission to create a PR in this repository?

I am unable to push my code to any feature branch. 🙇

@deankarn
Copy link
Contributor

@hnimtadd you need to fork the repository, make the changes push to your branch and the make upstream PR

@hnimtadd
Copy link
Author

hnimtadd commented Dec 15, 2024

Hi @deankarn. The PR is ready here

@hnimtadd
Copy link
Author

Hi @deankarn , is there any new update on this?

deankarn added a commit that referenced this issue Dec 29, 2024
This fixes #47 where `default`, and other set tags, were not setting
pointer types.
@deankarn
Copy link
Contributor

This should be working in Release https://github.com/go-playground/mold/releases/tag/v4.5.1

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

Successfully merging a pull request may close this issue.

2 participants