decodeObservationPayload silently drops numeric phenomenonTime; should reject symmetrically with resultTime

[Sam-Bolling]

Summary

decodeObservationPayload in internal/api/observation_handler.go reads phenomenonTime from the raw JSON map with raw["phenomenonTime"].(string). When the type assertion fails (e.g. client sent a numeric epoch), the field is silently dropped: no error returned, observation created with HTTP 201, phenomenonTime field discarded.

This is inconsistent with the sibling resultTime path, which catches the same wrong-type input via the late obs.ResultTime.IsZero() guard and returns 400 (see #3).

Surfaced during evaluation of #3.

Reproduction (HEAD, commit 4b994212)

curl -i -X POST "$BASE/datastreams/$DS/observations" \
  -H "Content-Type: application/json" \
  -d '{"resultTime":"2026-04-30T12:00:01Z","phenomenonTime":1773100000,"result":42}'
# → HTTP/2 201 — observation created
# GET shows phenomenonTime defaulted to resultTime (1773100000 was discarded)

Compare:

curl -i -X POST "$BASE/datastreams/$DS/observations" \
  -H "Content-Type: application/json" \
  -d '{"resultTime":1773100000,"result":42}'
# → HTTP/2 400 {"error":"resultTime is required"}

Same wrong-type input → opposite outcomes on sibling fields.

Code reference

// internal/api/observation_handler.go (around line 240)
if phenomenonTimeRaw, ok := raw["phenomenonTime"].(string); ok && phenomenonTimeRaw != "" {
    phenomenonTime, err := time.Parse(time.RFC3339, phenomenonTimeRaw)
    if err != nil {
        return nil, &decodeError{msg: "Invalid phenomenonTime format"}
    }
    obs.PhenomenonTime = &phenomenonTime
}
// ← no else; control falls through; no late "must be string" guard

Impact

• Clients sending numeric/legacy phenomenonTime get an apparently-successful POST but the actual observation timestamp is lost.
• Inconsistent behaviour across resultTime (rejected) and phenomenonTime (silently accepted then dropped) is the worst of both worlds — clients cannot infer one's behaviour from the other.

Suggested fix

Branch on type-assertion failure explicitly:

if v, present := raw["phenomenonTime"]; present && v != nil {
    s, ok := v.(string)
    if !ok {
        return nil, &decodeError{msg: "phenomenonTime must be an RFC 3339 string"}
    }
    if s != "" {
        t, err := time.Parse(time.RFC3339, s)
        if err != nil {
            return nil, &decodeError{msg: "Invalid phenomenonTime format: expected RFC 3339"}
        }
        obs.PhenomenonTime = &t
    }
}

Apply the same shape to resultTime (which simultaneously fixes the misleading error message in the separate UX follow-up).

Related

• Research: Time field encoding — strict ISO 8601 string requirement vs. numeric timestamp acceptance #3 — original research spike; full evaluation in docs/research/issue-evaluations/issue-003.md
• Sibling: separate follow-up filed for TimeRange.UnmarshalJSON silent-swallow on Datastream POST

[Sam-Bolling]

Verdict: validated — KEEP, P2 (data-integrity)

Independent re-validation against current HEAD 08cf161 (no drift in
internal/api/observation_handler.go between this HEAD and the cited
4b994212).

Live reproduction (HEAD 08cf161)

Test	Body	Status	GET back
T1	{"resultTime":"2026-04-30T12:00:01Z","phenomenonTime":1773100000,"result":42}	201 Created	phenomenonTime == resultTime (silent substitution)
T2	{"resultTime":1773100000,"result":42} (sibling control)	400 "resultTime is required"	—
T3	{"resultTime":"...","phenomenonTime":"not-a-time","result":42} (parse path control)	400 "Invalid phenomenonTime format"	—

T1 reproduces the issue body verbatim. T3 confirms the string-form parse
path is strict — the defect is exclusively in the type-assertion-failure
path.

Static analysis — symmetric decoder, asymmetric downstream visibility

internal/api/observation_handler.go:226-243 has two structurally
identical blocks:

if resultTimeRaw,    ok := raw["resultTime"]   .(string); ok && ... { ... }  // line 226
if phenomenonTimeRaw, ok := raw["phenomenonTime"].(string); ok && ... { ... } // line 236

The silent-drop on type-assertion failure is symmetric between the two
fields. The externally-visible asymmetry comes from the late guard at
line 272:

if obs.ResultTime.IsZero() {
    return nil, &decodeError{msg: "resultTime is required"}
}

This is possible only because obs.ResultTime is scalar time.Time (zero
value detectable). obs.PhenomenonTime is *time.Time so a skipped decode
legitimately leaves it nil. The 400 "resultTime is required" message is
misleading — the field WAS present, just wrong-typed.

Amplification by repository default-fill

internal/repository/observation_repository.go:22-25 silently substitutes
the dropped value:

if observation.PhenomenonTime == nil {
    t := observation.ResultTime
    observation.PhenomenonTime = &t
}

This makes the silent drop worse than null-leaving: the GET response is
plausibly correct (phenomenonTime == resultTime), so the client cannot
detect the data loss without comparing the original numeric input verbatim.

Adjacent finding — samplingFeature@id (not in original body)

Line 217 uses the same raw["samplingFeature@id"].(string); ok pattern.
A JSON number for that field would also be silently dropped. The
wrong-type case is narrower in practice (no repository default-fill;
field documented as string ID reference everywhere), but the same fix
shape (raw[...]; present && v != nil with explicit non-string → 400)
would harden it.

Spec authority

• CSAPI Part 1 §5.1 — server must validate received resources against the declared schema and reject non-conformant requests.
• SWE Common JSON encoding for time fields — RFC 3339 strings only; numbers are non-conformant.
• RFC 7493 (I-JSON) §3.4 — no silent type coercion of declared-string fields.

Suggested fix — extension of body's proposal

The proposed fix shape is correct. It should additionally:

1. Apply the same shape to resultTime (the body explicitly notes this), replacing "resultTime is required" with "resultTime must be an RFC 3339 string" for the wrong-type case (the late IsZero() guard remains for genuinely-missing).
2. Optionally apply to samplingFeature@id for consistency.

---

Evaluation artefacts (in our fork):

• docs/research/issue-evaluations/issue-019.md
• docs/research/evidence/issue-019/static-analysis-2026-04-30.md
• docs/research/evidence/issue-019/live-test-2026-04-30.md
• docs/research/evidence/issue-019/spec-authority-2026-04-30.md

[Sam-Bolling]

Closing as completed. The headline defect — decodeObservationPayload silently dropping numeric phenomenonTime and substituting resultTime via the repository default-fill — is fixed on upstream/main post-merge 22e8bcd. The adjacent samplingFeature@id finding from the original evaluation was not addressed; queued to the upstream-issues backlog.

Resolution

@SomethingCreativeStudios on the upstream triage pass:

"during validation, look at nilValues; they can be anything even a string like 'NaN'"

…which framed the broader observation-payload validation pass that landed as commit 1b2b614 (subject "Adding support for "latest" for TimeRange" — the commit message understates the diff scope; it also rewrote decodeObservationPayload's phenomenonTime/resultTime decoding and added the observation_schema_validation.go machinery sibling-tracked under #21).

Verification on upstream/main

The fix shape on internal/api/observation_handler.go is exactly the present && type-assertion-with-explicit-error pattern proposed in the issue body and the evaluation, applied symmetrically to both fields:

func decodeObservationPayload(r *http.Request) (*domains.Observation, error) {
    var raw map[string]any
    if err := json.NewDecoder(r.Body).Decode(&raw); err != nil {
        return nil, err
    }
    // ...

    if rtRaw, exists := raw["resultTime"]; exists {
        rtStr, ok := rtRaw.(string)
        if !ok {
            return nil, &decodeError{msg: `resultTime must be an ISO 8601 string (e.g. "2026-01-01T00:00:00Z")`}
        }
        if rtStr != "" {
            t, err := time.Parse(time.RFC3339, rtStr)
            if err != nil {
                return nil, &decodeError{msg: `resultTime must be an ISO 8601 string (e.g. "2026-01-01T00:00:00Z")`}
            }
            obs.ResultTime = t
        }
    }

    if ptRaw, exists := raw["phenomenonTime"]; exists {
        ptStr, ok := ptRaw.(string)
        if !ok {
            return nil, &decodeError{msg: `phenomenonTime must be an ISO 8601 string (e.g. "2026-01-01T00:00:00Z")`}
        }
        if ptStr != "" {
            t, err := time.Parse(time.RFC3339, ptStr)
            if err != nil {
                return nil, &decodeError{msg: `phenomenonTime must be an ISO 8601 string (e.g. "2026-01-01T00:00:00Z")`}
            }
            obs.PhenomenonTime = &t
        }
    }

    if obs.ResultTime.IsZero() {
        return nil, &decodeError{msg: "resultTime is required"}
    }

    return obs, nil
}

Two structural improvements over the pre-fix code:

1. Symmetric type-checking. Both fields now follow the same if rtRaw, exists := raw[...]; exists { ok-assert; ... } shape. The asymmetric "phenomenonTime silently dropped, resultTime incidentally caught by the late IsZero() guard" defect is closed.
2. Wrong-type vs missing distinguished. A numeric resultTime: 1773100000 now produces the descriptive "resultTime must be an ISO 8601 string (e.g. \"2026-01-01T00:00:00Z\")" instead of the misleading "resultTime is required". The late IsZero() guard remains, but only for the genuinely-missing case (now reachable only when resultTime is actually absent from the payload).

The three live-test repros from docs/research/evidence/issue-019/live-test-2026-04-30.md:

• T1 numeric phenomenonTime: 1773100000 → previously 201 + GET shows phenomenonTime == resultTime (silent substitution via the repository default-fill at observation_repository.go:22-25). Now: 400 with "phenomenonTime must be an ISO 8601 string...".
• T2 numeric resultTime: 1773100000 → previously 400 with the misleading "resultTime is required". Now: 400 with the precise wrong-type message.
• T3 malformed-string phenomenonTime: "not-a-time" → 400 (was already strict via time.Parse in the string-form path; behavior preserved).

The repository default-fill at observation_repository.go:22-25 (if obs.PhenomenonTime == nil { obs.PhenomenonTime = &obs.ResultTime }) is unchanged but is no longer reachable from a wrong-type phenomenonTime payload — the handler rejects at the door. It still serves its intended purpose for the legitimately-missing case (phenomenonTime is genuinely optional per CSAPI Part 1 §5.1).

Mapping fix → original claims

Original framing	Outcome
phenomenonTime: 1773100000 silently dropped, repo defaults to resultTime value, GET shows substituted value as if accepted	Fixed. 400 at decodeObservationPayload.
Asymmetric: resultTime wrong-type incidentally caught by late IsZero() guard with misleading "is required" message	Fixed. Wrong-type path surfaces precise message; IsZero() only fires for genuinely-missing.
Symmetric strict decode for both fields	Implemented exactly. Same shape applied to both.
RFC 7493 §3.4 / CSAPI Part 1 §5.1 / SWE Common JSON encoding compliance	Achieved. Wrong-typed JSON values are rejected, not coerced.
Severity P2 (data-integrity, silent substitution)	Justified; primary substitution pathway closed.

Residual gap — separate issue forthcoming on SomethingCreativeStudios/connected-systems-go

One adjacent finding from the original evaluation (§"Adjacent finding (not in original issue body)") that did not land in 1b2b614. Does not block #19's closure; queued in the upstream-issues backlog (docs/research/upstream-followup-backlog.md):

• samplingFeature@id retains the original raw[...].(string); ok silent-drop pattern. On upstream/main:

  if sfID, ok := raw["samplingFeature@id"].(string); ok && sfID != "" {
      obs.SamplingFeatureID = &sfID
  }

  A JSON number for that field is silently dropped. Narrower exposure than the time fields (no repository default-fill, so the GET response is null rather than a plausibly-correct substitution), but the same shape of bug. Consistent fix: apply the present && nil-check && type-assert-with-explicit-error pattern that phenomenonTime and resultTime now use.

Cross-references

• Original evaluation: docs/research/issue-evaluations/issue-019.md (symmetric-decoder static analysis, T1/T2/T3 live tests, repository default-fill mechanism, asymmetric IsZero() incidental-catch analysis).
• Static analysis: docs/research/evidence/issue-019/static-analysis-2026-04-30.md.
• Live tests: docs/research/evidence/issue-019/live-test-2026-04-30.md.
• Spec authority: docs/research/evidence/issue-019/spec-authority-2026-04-30.md.
• Fix-side commit: 1b2b614 (Adding support for "latest" for TimeRange) — diff scope exceeds subject; also rewrites decodeObservationPayload.
• Sibling closures collapsed onto the same observation-payload validation track per maintainer's triage: [P2 bug] Result validator ignores nilValues table — spec-idiomatic "NaN" / "-Infinity" strings rejected as wrong-type #21 (nilValues validation, observation_schema_validation.go).
• Adjacent: TimeRange.UnmarshalJSON silently swallows non-string and parse-failed times — Datastream POST stores empty TimeRange without error #18 (companion TimeRange.UnmarshalJSON strict decode), Misleading error 'resultTime is required' when field is present but wrong type #20 (broader error-handling overhaul providing the 400 surface).
• Follow-up: samplingFeature@id symmetric strict decode (queued in backlog).

Closing as completed.