Skip to content

Bug: typed additionalProperties initialization checks raw[""] instead of plain.AdditionalProperties == nil #562

@plheide

Description

@plheide

Problem

When a schema declares additionalProperties as a typed sub-schema (anything other than false or omitted), the generated UnmarshalJSON body emits a check against an empty-string KEY in the raw map instead of inspecting the catch-all field directly:

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

The intent is clearly "if mapstructure didn't populate AdditionalProperties, give it an empty map" — but the actual check is "if there's no JSON key literally named the empty string (or it's null), initialize the map". The two are not equivalent.

Failure modes

  1. Silent default skip: the rare-but-legal JSON input {"": "anything"} (a key that's the empty string) would prevent the empty-map initialization, leaving AdditionalProperties as whatever mapstructure produced — possibly nil if no other extra keys were present.
  2. Misleading code: anyone reading the generated code is led to believe there's significance to an empty-string key in the schema, when really there isn't.

In practice the bug rarely produces wrong output today (because the delete(raw, name) pruning loop runs after this check, and most inputs don't have a "" key), but the logic is wrong on its face.

Reproduction

The shipped fixture demonstrates the bug:

Run a unit test that decodes input {"": "value"} against this generated type and observe AdditionalProperties stays nil instead of being initialized to map[string]interface{}{"": "value"} (which is what mapstructure would have set).

Root cause

In pkg/generator/schema_generator.go (around lines 401-423), the field-defaults loop unconditionally constructs a defaultValidator for any struct field with a non-nil DefaultValue:

for _, f := range tt.Fields {
    if f.DefaultValue != nil {
        ...
        validators = append(validators, &defaultValidator{
            jsonName:         f.JSONName,    // "" for the synthetic AdditionalProperties field
            fieldName:        f.Name,         // "AdditionalProperties"
            ...
        })
    }
}

defaultValidator.generate then emits:

if v, ok := raw["%s"]; !ok || v == nil { ... }

— with %s being the empty f.JSONName, producing raw[""].

Suggested fix

Special-case the synthetic AdditionalProperties field in the defaults loop. Either:

  • (a) Skip generating a defaultValidator for f.Name == additionalProperties and inline the initialization logic at a different point in the body that checks plain.AdditionalProperties == nil, OR
  • (b) Add a new field == additionalProperties branch inside defaultValidator.generate (or a dedicated additionalPropertiesDefaultValidator) that emits the correct if plain.AdditionalProperties == nil { ... } check.

(a) is structurally cleaner; (b) keeps the validator-list dispatch uniform.

Test plan

  • New regression test under tests/data/core/additionalProperties/ that decodes JSON {"": "x"} and asserts plain.AdditionalProperties contains the """x" entry (currently, with the bug, the field is initialized to an empty map instead).
  • Existing goldens in tests/data/core/additionalProperties/ regenerate to use the corrected check.

Discovered via

CodeRabbit flagged the same line on a downstream fork PR (review-thread r3212631459, scope: same objectWithPropsAdditionalProperties.go:33 line) — called out as "Suspicious empty-string key check". The bug is pre-existing on main, not introduced by that PR; the fork PR just regenerated the golden as part of unrelated changes, surfacing the existing line in CodeRabbit's diff view.

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