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

TextUnmarshaler support for binding #3045

Closed
wants to merge 1 commit into from

Conversation

ssoroka
Copy link

@ssoroka ssoroka commented Feb 1, 2022

  • Adds encoding.TextUnmarshaler checking to support unmarshaling custom non-struct types.
  • Adds binding.BindUnmarshaler to support unmarshaling custom structs (from the suggestion in Use UnmarshalText in URI binding #2673)

As of this PR you can support any kind of custom type without breaking support for time.Time and other structs that might implement TextUnmarshaler

Closes #2673
Closes #2631

@ssoroka ssoroka changed the title TextUnmarshaller support for binding TextUnmarshaler support for binding Feb 1, 2022
binding/binding.go Outdated Show resolved Hide resolved
binding/form_mapping.go Outdated Show resolved Hide resolved
@@ -198,6 +199,14 @@ func setByForm(value reflect.Value, field reflect.StructField, form map[string][
}

func setWithProperType(val string, value reflect.Value, field reflect.StructField) error {
if value.Kind() != reflect.Struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you need this condition? I think it's perfectly fine to have structs that implement TextUnmarshaler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I would consider checking both value and value.Addr. It's possible that someone is already using a pointer in the struct that you're binding into:

var out struct {
    MyParam *MyType `form:"param"`
}

Copy link
Author

@ssoroka ssoroka Mar 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so TextUnmarshaler is used by some internal go structs, like time.Time. If we catch that here, we screw up the time handling. TextUnmarshaler is here as a convenience but UnmarshalParam is the safer choice

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for the pointer type and it seems to work as is

@ssoroka
Copy link
Author

ssoroka commented Mar 18, 2022

@kszafran I think I've addressed all your feedback. Mind taking another look?

kszafran
kszafran previously approved these changes Mar 18, 2022
Copy link
Contributor

@kszafran kszafran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I'm not a maintainer, though. You'll need to ping the maintainers if you want to get this merged.

@ssoroka
Copy link
Author

ssoroka commented Mar 21, 2022

@thinkerou when you have a chance, could you review/merge/tag another maintainer?

amahiwa-jp
amahiwa-jp previously approved these changes Apr 6, 2022
@amahiwa-jp
Copy link

Looks like an essential feature to have.
@ssoroka Could you squash your commits as requested per CONTRIBUTING.md?
@thinkerou @appleboy Could you check out this when you have time?

@ssoroka
Copy link
Author

ssoroka commented Apr 9, 2022

@amahiwa-jp I've squashed the commits into a single commit

@dcrystalj
Copy link

ETA for this?

@duaneking
Copy link

Updates?

@brokeyourbike
Copy link

Any update on when it can be merged?

@steveadams
Copy link

Is there anything I can contribute to help get this merged?

@thinkerou thinkerou modified the milestones: v1.9, v1.10 Jan 17, 2023
@AnatolyUA
Copy link

To make it work with arrays []uuid.UUID we might add to func setWithProperType:

	case reflect.Array:
		if a, e := trySetCustom(val, value); a || e != nil {
			return e
		}
		return errUnknownType

@eloyekunle
Copy link

Any update on this?

@minghan9456
Copy link

Any update?

@duaneking
Copy link

What are the perf changes for this update?

How much does it slow down binding for other things?

@phenixrizen
Copy link

@appleboy @thinkerou @javierprovecho Any update on this?

@duaneking
Copy link

Again, What is the perf change of this?

Have you done the work to assure it does not slow other things down?

@ssoroka
Copy link
Author

ssoroka commented Sep 7, 2023

I don't see any performance impact to this. I've been using this in production for a year and a half at this point. Maybe someone could pick it up, update it, and merge it?

@appleboy appleboy modified the milestones: v1.10, v1.11 Mar 21, 2024
@appleboy
Copy link
Member

appleboy commented Apr 6, 2024

@ssoroka I don't know why I can't review the PR? Can you create a new PR?

@ssoroka
Copy link
Author

ssoroka commented Apr 15, 2024

@appleboy this is way off my radar (it's been 2 years and I've switched to huma). Feel free to take the change over and apply it in a new PR.

@morremeyer
Copy link

I'm happy to help out here if anything is needed. Just let me know.

@miclle
Copy link

miclle commented Apr 25, 2024

Any update on this?

@appleboy
Copy link
Member

appleboy commented May 1, 2024

I can't handle the PR now (maybe no permission to review and trigger unit testing). Please move to New PR: #3933

@appleboy appleboy closed this May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use UnmarshalText in URI binding BindQuery to custom type