Skip to content

defaultValidator emits if v, ok := raw[""] for synthetic AdditionalProperties field with default — dead code in generated UnmarshalJSON #565

Description

@plheide

Summary

When a schema declares additionalProperties as a typed object with properties (e.g. {"additionalProperties":{"type":"object","properties":{...}}}), the generator:

  1. Synthesizes an AdditionalProperties field on the struct (no JSON tag, only mapstructure:",remain")
  2. Computes a default value of map[string]interface{}{} for it via defaultPropertyValue
  3. Attaches a defaultValidator keyed on the field's JSONName (which is "")

The defaultValidator emits its presence check using v.jsonName — but jsonName == "" for this synthetic field, so the generated code becomes:

if v, ok := raw[""]; !ok || v == nil {
    plain.AdditionalProperties = map[string]interface{}{}
}

Two problems:

  1. raw[""] is dead code in practice — JSON objects with a literal empty-string key are vanishingly rare. The check almost always passes (returns !ok = true), so the init runs unconditionally for typical input.
  2. Surprising semantics if the user IS using empty-string keys — a JSON object with key "" would suppress the AdditionalProperties default initialization, leaving it nil instead of {}.

Where in code

  • pkg/generator/validator.go ~ line 176 — defaultValidator.generate:
    out.Printlnf(`if v, ok := %s["%s"]; !ok || v == nil {`, varNameRawMap, v.jsonName)
    When v.jsonName == "", this emits raw[""].
  • pkg/generator/schema_generator.go ~ line 417 — the defaultValidator{...} is appended for every struct field with a DefaultValue, regardless of whether the field has a meaningful JSON name. The synthetic AdditionalProperties field with JSONName == "" slips through.

When introduced

The defaultValidator emit pattern itself: commit d173d053 (2022-10-13).

The interaction that exposes the bug — attaching defaultValidator to fields by f.JSONName AND the defaultPropertyValue function that computes a default map[string]interface{}{} for AdditionalProperties — both landed together in commit 33ec5597 (2023-11-11). That's when the buggy raw[""] line started appearing in goldens.

Reproduction

In-tree fixture: tests/data/core/additionalProperties/objectWithPropsAdditionalProperties (lines 33 and 73 of the golden).

Schema:

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "type": "object",
  "additionalProperties": {
    "type": "object",
    "properties": { "property1": { "type": "string" } }
  },
  "properties": { "foo": { "type": "string" } }
}

Suggested fix directions

Two options:

  1. Don't attach a defaultValidator to the synthetic AdditionalProperties field — skip the validator when f.JSONName == "" at the attachment site in schema_generator.go:417, OR special-case f.Name == "AdditionalProperties". Initialization of the catch-all map is already handled by the subsequent mapstructure.Decode(raw, &plain.AdditionalProperties) call in the shared unmarshal body; the redundant default-fill can simply go away. Cleanest: removes both the dead code and the empty-string-key surprise, and follows the existing "synthetic field is special" pattern (e.g., the AdditionalProperties skip in unmarshal_body.go's tag-stripping loop).

  2. Make defaultValidator.generate skip emission when v.jsonName == "" — smaller diff (1-3 lines), but only hides the symptom; doesn't address why a default-handling validator is being attached to a field that has no meaningful JSON name in the first place.

Recommended: Option 1

Option 1 is the right fix because the bug's root cause is that the synthetic AdditionalProperties field doesn't fit the "field with a JSON name and a user-declared default" model that defaultValidator was built for. The mapstructure.Decode call already populates the field from the catch-all keys; if no extras exist, the field stays nil — and that's a defensible API (Go's nil map is read-safe, and explicit-empty-vs-nil distinction is a separate consideration). Verify behavior of mapstructure.Decode against a nil destination map before committing to skipping the init entirely; if mapstructure leaves it nil and a non-nil empty map is preferred for downstream code, do the unconditional init right after the Plain decode without any raw[...] gate.

Option 2 leaves the architectural smell in place (a synthetic field going through user-default machinery) and would need to be revisited if the validator ever grows logic that depends on jsonName.

Surfaced by CodeRabbit

Flagged on tests/data/core/additionalProperties/objectWithPropsAdditionalProperties.go line 33 in plheide's fork PR (the file regenerated as part of an unrelated feature's golden refresh, which is what made CR re-scan it). The bug is pre-existing since 2023-11-11 and affects every existing fixture that has both inline AdditionalProperties (with declared properties) AND a default — not specific to the PR that surfaced it.

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