Skip to content

SDK code spells absence as an explicit None, so ADCP_STRICT_VALIDATION=1 refuses payloads the SDK itself builds (4 sites) #1893

Open

Description

@KonstantinMirin

Measured on the PyPI wheel adcp 9.6.1 (CPython 3.12.13), and re-read against main at bef739e02 where the construction sites are unchanged.

What happens

NoExplicitNull (#1547) refuses a JSON null for an optional property whose schema does not admit one, and keeps T | None on the Python side so that Model(field=None) still reads as "not supplied" — unless ADCP_STRICT_VALIDATION is set, where _RefuseJsonNull makes the Python side strict too (src/adcp/types/base.py, python_schema=strict if _EXTRA_POLICY == "forbid" else nullable).

With that switch on, three of the SDK's own entry points refuse their own construction:

$ ADCP_STRICT_VALIDATION=1 python probe.py
_EXTRA_POLICY = forbid

create_mcp_webhook_payload(task_id=..., task_type=..., status=..., idempotency_key=..., operation_id=...)
  -> ValidationError: 5 validation errors for McpWebhookPayload
     notification_id  Input should be a valid string [type=string_type, input_value=None]
     (and message, context_id, token, protocol)

PaginationRequest(max_results=50, cursor=None)
  -> ValidationError: cursor  Input should be a valid string [type=string_type, input_value=None]

Error(code="INVALID_INPUT", message="m", field=None, details=None, suggestion=None)
  -> ValidationError: 3 validation errors for Error  (field, details, suggestion)

Every one of those is reached through SDK code with the caller supplying nothing:

site on main construction
src/adcp/webhooks.py:272-286 McpWebhookPayload.model_validate({... "notification_id": notification_id, "message": message, "context_id": context_id, "token": token ...}) — every key is named whether or not the caller passed it
src/adcp/feed_mirror.py:449 and :467 PaginationRequest(max_results=self._page_limit, cursor=cursor), where cursor is None on the first page
src/adcp/canonical_formats/advisory.py:141-149 Error(..., field=field, details=details, suggestion=suggestion)
src/adcp/server/principal.py:166 and :812 {..., "context": context} into SyncPrincipalResponse.model_validate

With the switch unset all four are accepted, so this is invisible in a default run.

Why it looks like a defect rather than a preference

src/adcp/types/base.py recommends the switch for exactly the case where it cannot be used:

Most useful during upgrades: right after a major spec revision, run tests with ADCP_STRICT_VALIDATION=1 to surface every place the upgrade dropped a renamed field, then ship with the flag unset for the forward-compat default.

A consumer who follows that advice gets failures in SDK code rather than in their own. The class is not small: NoExplicitNull appears 7,061 times across 667 generated modules on main.

And the documents agree with the strict reading. None of these properties admits a null:

document property type
core/mcp-webhook-payload.json message, context_id, notification_id, token {"type": "string"}
core/error.json field, suggestion {"type": "string"}
core/error.json details {"type": "object"}
core/pagination-request.json cursor {"type": "string"}

core/mcp-webhook-payload.json requires [idempotency_key, operation_id, task_id, task_type, status, timestamp], so every field above is optional-and-not-nullable: the document admits absence and refuses null.

The fix costs nothing on the wire

Omitting the key instead of naming it with None produces the same bytes, because AdCPBaseModel already serialises with exclude_none. Measured, same payload both ways:

key omitted      -> ['idempotency_key', 'operation_id', 'status', 'task_id', 'task_type', 'timestamp']
"message": None  -> ['idempotency_key', 'operation_id', 'status', 'task_id', 'task_type', 'timestamp']

and the attribute a caller reads does not move either — the field's default is None, so payload.message is None with the key omitted.

feed_mirror.py already does this three lines below the site above, for account and if_wholesale_feed_version: the key is built into the dict only when it has a value. The same shape at the four sites would make the switch usable. For webhooks.py it is one comprehension over the optional keys.

Reproduction

# ADCP_STRICT_VALIDATION must be set before `import adcp` — the policy is read at import time
import os; os.environ["ADCP_STRICT_VALIDATION"] = "1"
from adcp.webhooks import create_mcp_webhook_payload
create_mcp_webhook_payload(
    task_id="t-1", task_type="media_buy_delivery", status="completed",
    idempotency_key="abcdefghijklmnop", operation_id="op-1",
)

The question

Is ADCP_STRICT_VALIDATION meant to be usable by an adopter's CI on a tree that calls these helpers? Its docstring says yes, and today it is not. If the intent is instead that an explicit None from Python code is always "not supplied", then _RefuseJsonNull's coupling to _EXTRA_POLICY looks like the thing to reconsider rather than the four call sites — the switch is documented as being about unknown fields, and it also changes null handling.

Either way the omission at those four sites seems worth having on its own: it is byte-identical on the wire, it matches what feed_mirror.py already does for its sibling keys, and it is what the documents state.

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

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions