confusion with WithUnmarshalers

[elv-gilles]

I wanted to install multiple unmarshalers and wrote the wrong code below:

	err := json.UnmarshalDecode(dec, c,
		json.WithUnmarshalers(
			json.UnmarshalFuncV2(func(dec *jsontext.Decoder, val *Conf, opts json.Options) error {
				// do something
				return json.SkipFunc
			})),
		json.WithUnmarshalers(
			json.UnmarshalFuncV2(func(dec *jsontext.Decoder, val *SubConf, opts json.Options) error {
				// do something
				return json.SkipFunc
			})))

This code returns with no error, but the first unmarshalers is ignored.

Instead, after navigating in the source code of json-experiment, I discovered I should have written:

	err := json.UnmarshalDecode(dec, c,
		json.WithUnmarshalers(
			json.NewUnmarshalers(
				json.UnmarshalFuncV2(func(dec *jsontext.Decoder, val *Conf, opts json.Options) error {
					// do something
					return json.SkipFunc
				}),
				json.UnmarshalFuncV2(func(dec *jsontext.Decoder, val *SubConf, opts json.Options) error {
					// do something
					return json.SkipFunc
				}),
			)))

Looking at the code, I also see that options are merged through *Struct.Join.

I think it's currently easy to be tripped and (at least) one of the below - in order of increased preference - would be of interest:

1. improve the comment of UnmarshalDecode or Options to prominently say that only the last option of a given kind will be used (if that comment exists I missed it), and/or
2. have *Struct.Join return an error in case where an option would overwrite a previously set one, and/or
3. have *Struct.Join handle such case and do what NewUnmarshalers does (for marshalers/unmarshalers)

[dsnet]

For case 1, we currently document on JoinOptions:

Properties set in later options override the value of previously set properties.

but we can make this more explicit elsewhere in the documentation.

If this does get merged into the stdlib, one could imagine a go vet check for obviously incompatible sets of provided options.

The downside of case 2 is that we would need to return an error in more cases where we take in ...Option. For example, NewEncoder, NewDecoder, Encoder.Reset and Decoder.Reset would all need to return an error now.

The downside of case 3 is that it complicates what JoinOpions means for different option parameters. Always overriding is a relatively simple rule, while merging under some special cases can get confusing.

[elv-gilles]

Looks good, I guess (now - that I know - that seems obvious :-)

[MarkRosemaker]

I faced a similar issue.

I read the documentation @dsnet, but I understood it to mean that an unmarshal function for a struct will override a previous one for the same struct, not that an UnmarshalFuncV2 for one struct (e.g., SubConf) will override an UnmarshalFuncV2 for another (e.g., Conf).

Would it be feasible for the options to merge those definitions smartly? This would be particularly useful for my use case, where users can add marshalers/unmarshalers dynamically.

Incidentally, I had to implement the following workaround to manage dynamic addition of marshalers/unmarshalers:

type JSONConfig struct {
	mu           sync.Mutex
	marshalers   []*json.Marshalers
	unmarshalers []*json.Unmarshalers
	other        []json.Options
}

func (c *JSONConfig) AddMarshaler(m *json.Marshalers) {
	c.mu.Lock()
	defer c.mu.Unlock()

	c.marshalers = append(c.marshalers, m)
}

func (c *JSONConfig) AddUnmarshaler(u *json.Unmarshalers) {
	c.mu.Lock()
	defer c.mu.Unlock()

	c.unmarshalers = append(c.unmarshalers, u)
}

func (c *JSONConfig) AddOption(o json.Options) {
	c.mu.Lock()
	defer c.mu.Unlock()

	c.other = append(c.other, o)
}

func (c *JSONConfig) Options() json.Options {
	return json.JoinOptions(append(
		c.other,
		json.WithMarshalers(json.NewMarshalers(c.marshalers...)),
		json.WithUnmarshalers(json.NewUnmarshalers(c.unmarshalers...)),
	)...)
}


// in action:

var cfg = &JSONConfig{}

....

	err := json.Unmarshal(in, &v, cfg.Options())

Having such functionality built into the JSON library would greatly simplify managing marshalers/unmarshalers dynamically and avoid the need for custom configurations like mine.

Additionally, the plural method names like json.WithUnmarshalers seem a bit misleading since they take just one argument. Perhaps reconsidering the naming or allowing multiple arguments directly could help clarify their usage.

[dsnet]

@MarkRosemaker, you bring up a legitimate use case of wanting to merge marshalers.

I've been thinking about this and I couldn't come up with a solution that is consistently good in all areas. For the sake of discussion below, I'll just talk about caller-specified marshalers, but this equally applies to unmarshalers.

Today you can do:

m1, _ := json.GetOption(opts, json.WithMarshalers)
opts = json.JoinOptions(opts, json.WithMarshalers(json.NewMarshalers(m1, m2)))

This is the minimal code you need to merge m1 with m2 (instead of having m2 entirely delete the existence of m1).

Let's suppose we make it such that JoinOptions automatically merges marshalers such that the following is equivalent to the above:

opts = json.JoinOptions(opts, m2)

but then this opens up a different problems:

1. The data model for options is now inconsistent where marshalers are treated different from other types of options.
2. You can no longer replace the set of marshalers

For example, a call like:

json.Marshal(v, opts, json.DefaultOptionsV1())

should entirely use v1 semantics (i.e., no caller-specified marshalers), but would now still use them since we lost the ability to replace any marshalers in opts with the empty set of marshalers. We would need to invent the concept of a "anti" marshaler (which clears any prior marshalers).

It seems to me that the current semantics is more consistent and flexible. Merging is at least possible even if it's not the default behavior (and whether merging of marshalers should be default behavior is also debatable).

[dsnet]

As another idea, I considered a generic WithMarshaler option:

func WithMarshaler[T any](func(dec *jsontext.Decoder, val *Conf, opts json.Options) error) Options

where each Marshaler option is keyed to a particular T. Thus, Marshaler[T] would only replace prior instances of Marshaler[T], but not prior instances of Marshaler[R] because they are distinctly different option keys. This would solve the problem @elv-gilles originally reported.

However, this approach also runs into problems:

1. We lack the ability to replace the entire set of all marshalers.
2. It is unclear how interface types and concrete types interact. For example, what should Join(Marshaler[T], Marshaler[I]) do if concrete type T implements interface type I? Do we say that marshalers of concrete types take precedence over marshalers or interface types? What if I explicitly wanted the opposite precedence order?
3. Sometimes you don't want Marshaler[T] to override previous instances of Marshaler[T] since the former one might return SkipFunc, deferring evaluation to the next marshaler in the chain.

Today's API approach is still more flexible because NewMarshalers creates an ordered set, where ordered-ness of the set allows author more flexibility.

The downside of this flexibility are the issues that @elv-gilles and @MarkRosemaker are reporting.