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

add codec option to use encoding.Text/Binary(Un)Marshaler when present #2666

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

lucix-aws
Copy link
Contributor

Adds opt-in use of encoding.TextMarshaler / encoding.BinaryMarshaler (and unmarshal versions):

+
+       // When enabled, the encoder will use implementations of
+       // encoding.TextMarshaler and encoding.BinaryMarshaler when present on
+       // marshaled values.
+       //
+       // Implementations are checked in the following order:
+       //   - [Marshaler]
+       //   - encoding.TextMarshaler
+       //   - encoding.BinaryMarshaler
+       //
+       // The results of a MarshalText call will convert to string (S), results
+       // from a MarshalBinary call will convert to binary (B).
+       UseEncodingMarshalers bool
+
+       // When enabled, the decoder will use implementations of
+       // encoding.TextUnmarshaler and encoding.BinaryUnmarshaler when present on
+       // unmarshaling targets.
+       //
+       // If a target implements [Unmarshaler], encoding unmarshaler
+       // implementations are ignored.
+       //
+       // If the attributevalue is a string, its underlying value will be used to
+       // call UnmarshalText on the target. If the attributevalue is a binary, its
+       // value will be used to call UnmarshalBinary.
+       UseEncodingUnmarshalers bool

Closes #2596

@lucix-aws lucix-aws requested a review from a team as a code owner June 4, 2024 16:32
if u != nil {
return u.UnmarshalDynamoDBAttributeValue(av)
}
return d.decodeNull(v)
}

u, v = indirect(v, indirectOptions{})
v0 := v
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you using the same technique described by Go source code? link

func indirect(v reflect.Value, decodingNull bool) (Unmarshaler, encoding.TextUnmarshaler, reflect.Value) {
	// Issue #24153 indicates that it is generally not a guaranteed property
	// that you may round-trip a reflect.Value by calling Value.Addr().Elem()
	// and expect the value to still be settable for values derived from
	// unexported embedded struct fields.
	//
	// The logic below effectively does this when it first addresses the value
	// (to satisfy possible pointer methods) and continues to dereference
	// subsequent pointers as necessary.
	//

	// After the first round-trip, we set v back to the original value to
	// preserve the original RW flags contained in reflect.Value.
	v0 := v

If so, aren't we missing the last part of the method?

if haveAddr {
	v = v0 // restore original value after round-trip Value.Addr().Elem()
	haveAddr = false
} else {
	v = v.Elem()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this is different - this is just preserving the original reflect value so we can re-traverse to look for the new marshalers.

t.Errorf("got unexpected error %v for input %v", err, testCase.input)
}
if diff := cmpDiff(testCase.expected, actual); len(diff) != 0 {
t.Errorf("expected match but got: %s", diff)
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] should we print the expected case as well, like "expected %s but got %s"?

@lucix-aws lucix-aws merged commit 882127d into main Jun 5, 2024
13 checks passed
@lucix-aws lucix-aws deleted the feat-avencodingmarshalers branch June 5, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants