Skip to content

A failing UnmarshalText or UnmarshalJSON is discarded, so the reported error names the wrong cause #79

Description

@creasty

What happens

setField offers the tag to the type's own unmarshaler before parsing by kind. That happens in unmarshalByInterface, which returns a bool and drops the error:

https://github.com/creasty/defaults/blob/272ac74/defaults.go#L268-L284

So when UnmarshalText rejects the tag, one of two things follows.

1. The kind-based parse succeeds, and the rejection is invisible. TestSet_FailingUnmarshalerFallsBackToKind pins this: a string-kinded type whose UnmarshalText always fails ends up holding the raw tag, parsed as a plain string, with no error.

2. The kind-based parse also fails, and the error reported since #59 is the kind path's — not the unmarshaler's. For a time.ParseDuration-backed wrapper:

type Duration struct{ time.Duration }

func (d *Duration) UnmarshalText(text []byte) error {
	var err error
	d.Duration, err = time.ParseDuration(string(text))
	return err
}

struct {
	Timeout Duration `default:"garbage"`
}

Set reports

field Timeout: invalid default "garbage": invalid character 'g' looking for beginning of value

where the cause the author wants is time: invalid duration "garbage". The JSON parser gets reported because the type is struct-kinded and json.Unmarshal was the fall-back — a parser that never had a chance on this input, and one the author never opted into.

Why it matters

Two closed PRs were written to get that error out. #45 proposed a new TaggedSetter interface whose method returns an error; #56 proposed calling UnmarshalText from Set and returning its error directly. Both were declined on other grounds, but the complaint underneath them is real, and this is the narrow version of it.

Options

  1. Keep the fall-back; report the unmarshaler's error when the kind path also fails. Remember the error instead of discarding it, and prefer it in the message. No case that succeeds today changes, and TestSet_FailingUnmarshalerFallsBackToKind keeps passing — only the message when everything fails.
  2. Report the unmarshaler's error immediately. What tagged defaults #45 and support custom defaults on types that implement encoding.TextUnmarshaler  #56 wanted. This flips TestSet_FailingUnmarshalerFallsBackToKind: the fall-back is load-bearing for a type whose UnmarshalText is stricter than its kind, so tags that work today would start failing.
  3. Leave it, document it.

I would take 1. The fall-back is deliberate and worth keeping; it is the misattributed cause that actually misleads.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions