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

Disallow json.(Un)Marshaler/encoding.Text(Un)Marshaler on user-defined types #1247

Closed
Tracked by #1248
alecthomas opened this issue Apr 13, 2024 · 0 comments · Fixed by #1417
Closed
Tracked by #1248

Disallow json.(Un)Marshaler/encoding.Text(Un)Marshaler on user-defined types #1247

alecthomas opened this issue Apr 13, 2024 · 0 comments · Fixed by #1417
Assignees
Labels
intermediate A task requiring an intermediate level of knowledge techdebt Issue is technical debt

Comments

@alecthomas
Copy link
Collaborator

We added support for delegating to json.(Un)Marshaler/encoding.Text(Un)Marshaler only so that stdlib types such as time.Time would be correctly transcoded. Something I didn't think of at the time though, is that this would allow user types to override encoding, which is something that we do not want.

@alecthomas alecthomas added the intermediate A task requiring an intermediate level of knowledge label Apr 13, 2024
@github-actions github-actions bot added the triage Issue needs triaging label Apr 13, 2024
@alecthomas alecthomas added techdebt Issue is technical debt and removed triage Issue needs triaging labels Apr 15, 2024
deniseli added a commit that referenced this issue May 7, 2024
This PR:
* Rips out the existing textMarshaler and jsonMarshaler usage from
encoding.go. We may want to add those back for
#1296 down the road, but we
will need to be thoughtful about how we do that. Removing it for now
keeps the logic much more predictable.
* Moves the (un)marshaling logic for `ftl.Option` out of `option.go` and
into `encoding.go`.
* Special-cases both `time.Time` (the only stdlib type we currently
support) and `ftl.Option`. Also `json.RawMessage` for _just_ encoding to
preserve the existing `omitempty` behavior.
* Fixes some existing issues where the Pointer unmarshaling wasn't
actually working correctly
* [eww] Adds a rather grotesque alternative to `Peek()` in
`isNextTokenNull()` because json Decoder does not support Peek.
* [eww] Makes the ftl.Option struct fields public so that they are
settable by `encoding.go`.

Suggestions welcome for both counts of [eww] above :)

Fixes #1247.
Addresses most of #1262, except
`omitempty` is only working for json.RawMessage for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intermediate A task requiring an intermediate level of knowledge techdebt Issue is technical debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants