Enhancement: `form.Marshaler` and `form.Unmarshaler` #63

tigh-latte · 2024-05-09T11:30:16Z

Fixes Or Enhances

Add support for custom marshalling and unmarshalling to a struct.

The motivation behind this mainly comes from using generics, when looking to register a custom encoder/decoder, you have to register one for every generic implementation of a struct, so for example:

type MyGenericStruct[T any] struct {
    T T
}

decoder := form.NewDecoder()
decoder.RegisterCustomTypeFunc(func(x any) ([]string, error) {
    return // values
}, MyGenericStruct[Type1]{})
decoder.RegisterCustomTypeFunc(func(x any) ([]string, error) {
    return // values
}, MyGenericStruct[Type2]{})
// etc...

This isn't really great, so instead with these marshaller interfaces we can simply do:

func (g *MyGenericStruct[T]) UnmarshalForm(ss []string) error {
    // Custom handling
    return nil
}

func (g MyGenericStruct[T]) MarshalForm() ([]string, error) {
    s := []string{}
    // build s
    return s, nil
}

Make sure that you've checked the boxes below before you submit PR:

Tests exist or have been written that cover this particular change.

@go-playground/admins

coveralls · 2024-05-09T11:32:21Z

coverage: 99.646% (-0.2%) from 99.814%
when pulling b045670 on tigh-latte:enhancement/custom-marshaller
into 8785d3c on go-playground:master.

tigh-latte · 2024-05-09T11:34:17Z

Looks like gofumpt also made a number of formatting edits. Let me know if this is something yous don't want.

deankarn · 2024-05-11T15:49:41Z

Thank you for the PR @tigh-latte! I think this would be an amazing addition and will take a deeper look at it this weekend.

I already took a cursory look and code looks solid, I am only thinking about the interface and its signature function/method name.

tigh-latte · 2024-05-11T16:38:10Z

Sure thing, let me know if you have any better/preferred suggestions.

deankarn

Hey @tigh-latte

I was able to find time to more thoroughly review the code and think about the interface names.

I like the interface names as is :)
I left a comment to see if changing the unmarshalling logic slightly was possible only to keep the flow of the program using the existing Ptr logic.

And again TY for the PR, can't wait to merge this.

deankarn · 2024-05-12T15:27:57Z

encoder.go

@@ -102,6 +98,20 @@ func (e *encoder) setFieldByType(current reflect.Value, namespace []byte, idx in
 			return
 		}
 	}
+	{


I'm not sure the extra scope is necessary.

deankarn · 2024-05-12T15:33:15Z

encoder.go

+		if t := v.Type(); t.Kind() == reflect.Ptr && v.IsNil() {
+			return
+		}


Is this check necessary?

Below this logic reflect.Ptr is handled and returned.

What is someone wanted to set a default value when it was nil from the custom marshaller, would this prevent that?

So I've played around with this, and could get something sorted that didn't risk a nil pointer in some form.

For example, by removing this check and making one or two other modifications, I ended up allowing this following to work:

func (c *custom) MarshalForm() ([]string, error) { if c == nil { return // some default value } return // real handling } func main() { var c *custom NewEncoder().Encode(c) }

However, if MarshalForm is changed to have a value receiver, then we'd end up with a nil panic because the nil *custom passes the form.Marshaler implementation check, then have its MarshalForm function called, causing the panic once dereferenced to the receiver.

All this playing about ended up having me take a read of the json.Marshaler implementation in the standard library. It's behaviour is basically just, if the item being marshalled is nil, write "null" (see here, and here).

The way the std lib handles its reflection checks is much better than what I've done here so I'm gonna do a small rewrite, but do let me know what behaviour you would prefer regarding this.

deankarn · 2024-05-12T15:34:47Z

decoder.go

@@ -199,6 +194,27 @@ func (d *decoder) setFieldByType(current reflect.Value, namespace []byte, idx in
 			}
 		}
 	}
+	{


I'm not sure the extra scope is needed.

This was added so that v could be shadowed and modified without changing the code later on, but I'll just assign all modifications to a new var instead.

deankarn · 2024-05-12T15:37:14Z

form_decoder.go

@@ -8,6 +8,11 @@ import (
 	"sync"
 )

+// Unmarshaler is the interface implemented by an object that can unmarshal a form representation of itself.
+type Unmarshaler interface {
+	UnmarshalForm(ss []string) error


Suggested change

UnmarshalForm(ss []string) error

UnmarshalForm([]string) error

deankarn · 2024-05-12T15:41:29Z

decoder.go

+		v := v // deliberately shadow v as to not modify
+		t := v.Type()
+		if t.Kind() != reflect.Ptr && v.CanAddr() {
+			v = v.Addr()
+		}
+		if um, ok := v.Interface().(Unmarshaler); ok {
+			// if receiver is a nil pointer, set before calling function.
+			if t.Kind() == reflect.Ptr && v.IsNil() {
+				t = t.Elem()
+				v.Set(reflect.New(t))
+				um = v.Interface().(Unmarshaler)
+			}


Is there any way to avoid doing this/duplicating the Ptr logic of creating the new element?

below this same logic exists for Ptr already https://github.com/go-playground/form/pull/63/files#diff-2a967f84f1af0d52111b8dbfbeadc44bfeb1092dacebb2bc0648a0e57284c04bR226-R230.

It would be great if it's possible to keep the flow of the program using that same logic.

I'll have a look at this and see if anything can be done

tigh-latte added 5 commits May 9, 2024 12:00

add FormMarshaler and FormUnmarshaler

97397eb

encoder error tests

2d15be0

rename marshaller interface to prevent stutter

ab25a15

update README

ef112dc

remove unnecessary shadow in encoder, use already define var in decoder

eab201f

tigh-latte added 4 commits May 9, 2024 12:58

slice tests

e904ed6

fix pointer test

6270911

map encoding tests

54d6dbf

map encoder tests, more decoder tests

bc69006

deankarn reviewed May 12, 2024

View reviewed changes

tigh-latte added 4 commits May 13, 2024 15:55

standard lib ideas, tidier handling

58d5d48

have the encoder marshal funcs return an error

8eda55b

use legacy functions

98b9046

found, fixed, and tested a nil panic

b045670

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: `form.Marshaler` and `form.Unmarshaler` #63

Enhancement: `form.Marshaler` and `form.Unmarshaler` #63

tigh-latte commented May 9, 2024

coveralls commented May 9, 2024 •

edited

Loading

tigh-latte commented May 9, 2024

deankarn commented May 11, 2024

tigh-latte commented May 11, 2024

deankarn left a comment

deankarn May 12, 2024

tigh-latte May 13, 2024

deankarn May 12, 2024

tigh-latte May 13, 2024

deankarn May 12, 2024

tigh-latte May 13, 2024

deankarn May 12, 2024

deankarn May 12, 2024

tigh-latte May 13, 2024

	UnmarshalForm(ss []string) error
	UnmarshalForm([]string) error

Enhancement: form.Marshaler and form.Unmarshaler #63

Are you sure you want to change the base?

Enhancement: form.Marshaler and form.Unmarshaler #63

Conversation

tigh-latte commented May 9, 2024

Fixes Or Enhances

coveralls commented May 9, 2024 • edited Loading

tigh-latte commented May 9, 2024

deankarn commented May 11, 2024

tigh-latte commented May 11, 2024

deankarn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enhancement: `form.Marshaler` and `form.Unmarshaler` #63

Enhancement: `form.Marshaler` and `form.Unmarshaler` #63

coveralls commented May 9, 2024 •

edited

Loading