PoC: change PUT bodies to require all fields by david-crespo · Pull Request #10673 · oxidecomputer/omicron

david-crespo · 2026-06-24T20:47:40Z

This prototype came out of the discussion at #10566 (comment). PUT request bodies in the external API do not follow a consistent pattern. Most of them behave like PATCH: they have optional fields that, when left out, leave the value of that field unchanged. We have discussed many times (e.g., #6406) picking a pattern and sticking to it, and generally people have preferred that we switch to what is considered standard PUT semantics, where the body is the same as what you get if you fetch the resource (or at least, it has the full the subset of fields which are mutable) and none of the fields are optional. This has the advantage of avoiding ambiguity around the semantics of leaving a field out — when a field is optional, I always have to check the docs because I never feel confident about what I'm supposed to do with that.

Changes

This PR converts four PUT endpoints, each chosen to to represent a distinct conversion shape so the plan is proven out before converting the remaining ~15:

project_update — identity-only, the most common case (~14 endpoints flatten identity): make name and description required
vpc_subnet_update — mixed: required name/description plus Nullable custom_router
silo_quotas_update — required quota values, no identity stuff
support_bundle_update — clearable-only Nullable user_comment; also corrects a pre-existing clear-on-omit bug

Also updated the necessary integration tests, and added two new regression tests that exercise the old API version. It occurred to me while doing this that it's probably bad that we don't have any integration tests for old versions of endpoints?

Full list of update endpoints to convert

★ marks the four converted.

Endpoint	Body type	Category	Change
project_update ★	ProjectUpdate	D · identity-only	`name`→req `T`, `description`→req (old clients omit via merge)
affinity_group_update	AffinityGroupUpdate	D · identity-only	same
anti_affinity_group_update	AntiAffinityGroupUpdate	D · identity-only	same
external_subnet_update	ExternalSubnetUpdate	D · identity-only	same
floating_ip_update	FloatingIpUpdate	D · identity-only	same
system_ip_pool_update	IpPoolUpdate	D · identity-only	same
system_subnet_pool_update	SubnetPoolUpdate	D · identity-only	same
vpc_router_update	VpcRouterUpdate	D · identity-only	same
vpc_subnet_update ★	VpcSubnetUpdate	D · identity + clearable	identity req; `custom_router`→`Nullable`
vpc_update	VpcUpdate	D · identity + extra	identity req; `dns_name`→req `T`
webhook_receiver_update	WebhookReceiverUpdate	D · identity + extra	identity req; `endpoint`→req `T`
vpc_router_route_update	RouterRouteUpdate	D · identity + extra	identity req (`destination`/`target` already req)
instance_network_interface_update	InstanceNetworkInterfaceUpdate	D · identity + trivial	identity req (`primary`/`transit_ips` already value-shaped)
silo_quotas_update ★	SiloQuotasUpdate	C · operational scalars	`cpus`/`memory`/`storage`→req `T` (real behavior tightening)
support_bundle_update ★	SupportBundleUpdate	C · clearable-only	`user_comment`→`Nullable` (corrects today's clear-on-omit bug)
instance_multicast_group_join	InstanceMulticastGroupJoin	C · (likely out of scope)	a join/create action, not a resource update
system_networking_settings_update	SystemNetworkingSettingsUpdate	B · trivial	drop `#[serde(default)]` on one bool
vpc_firewall_rules_update	VpcFirewallRuleUpdateParams	B · trivial	drop `default`; already replace-semantics

Bucket legend: B = trivial (drop default/make required, no identity); C = non-identity, needs per-field Nullable-vs-required judgment; D = flattens IdentityMetadataUpdateParams (the 14-endpoint identity question is the real crux).

Not in scope — already conformant, zero work (15): instance_update (the model), auth_settings_update, the four *_policy_update (SiloRolePolicy/ProjectRolePolicy/FleetRolePolicy), physical_disk_enable_adoption, sled_set_provision_policy, system_ip_pool_silo_update, system_subnet_pool_silo_update, networking_allow_list_update, networking_inbound_icmp_update, networking_bgp_announce_set_update, system_update_recovery_finish, target_release_update. (Plus system_update_repository_upload, which has no JSON body.)

API version compatibility

Old versions can keep working like they did before — we keep the datastore logic exactly the same (changesets can leave fields out to leave them unchanged). The difference is simply that the new endpoints always include all the fields in the changeset.

🤖 notes (gist)

Initially had these in the diff at bot-notes/, but obviously that is it not a long term solution. I'm experimenting with this gist approach now. Basically I want a way for them to be semi-permanently attached to the PR and easily viewed with a click (rules out file attachments) without being checked into the repo.

… support_bundle)

…ersions crate)

…sons in plan notes

…g in plan

david-crespo · 2026-06-24T20:49:03Z

+#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
+pub struct ProjectUpdate {
+    pub name: Name,
+    pub description: String,


We're hard-coding these fields here, but once we convert them all we can just switch IdentityMetadataUpdateParams to look like this and use it flattened like we currently do.

Added IdentityMetadataUpdateParamsStrict in 26aa0b5. Once all the endpoints are converted, we can rename it to IdentityMetadataUpdateParams.

david-crespo · 2026-06-24T20:53:11Z

        opctx: &OpContext,
        project_lookup: &lookup::Project<'_>,
-        new_params: &project::ProjectUpdate,
+        updates: db::model::ProjectUpdate,


A couple of things are ugly about this and project_update_v2025_11_20_00 below: I don't love changing this to take the model — I think I'd rather it continue to take params. I also don't like adding version-aware logic here in the app layer. I think I'd rather have one app layer function that we can just call from the old and new endpoint handlers. Working on it.

Edit: Changed in 890a156

Restores the params-object convention at the handler/app boundary: a single version-agnostic app method takes the prior (lenient) wire type; the latest strict body down-converts to it via From. Drops the per-version app methods and the db-model dependency on the versions crate.

david-crespo · 2026-06-25T20:32:26Z

      },
      "ProjectUpdate": {
-        "description": "Updateable properties of a `Project`",
+        "description": "Updateable properties of a `Project`\n\nA `PUT` replaces the resource, so every field is required: `name` and `description` must both be present.",


In #10675 I undid these changes to the comments. They are pointless.

`api-diff` is a lovely tool (if I may say so) for seeing the effect of OpenAPI schema changes in Omicron on the generated TS client. Until this PR it can only look at GitHub. This is useful for pointing it at a PR number. However, while working on oxidecomputer/omicron#10673 and oxidecomputer/omicron#10675, which cause large diffs in the schema, I found I wanted to be able to see changes before I push them. This PR adds a `--local` flag that lets you run it against local Omicron commits. While I think I wrote the original version of this script by hand, it's gone through a few iterations of full vibe-coding, so while I've skimmed the code, I am really not especially concerned about the details. The way this PR works is it adds a concept of `Source`, which is either GitHub or a local repo, and each has its corresponding methods for listing schema files and reading the schema contents, etc. Nice work from the robot. ```ts /** * A source of schema files. The remote source reads from GitHub; the local * source reads from the git repo in the current directory. Both expose the * same primitives so the diff logic doesn't care where schemas come from. */ type Source = { /** Resolve a ref (PR number, SHA, branch, jj revset, or undefined for the default) to a commit id */ resolveCommit: (ref?: string | number) => Promise<string> /** List schema filenames under openapi/nexus at a commit, or null if the dir is absent */ listSchemaNames: (commit: string) => Promise<string[] | null> /** Read the filename that nexus-latest.json points to at a commit */ readLatestPointer: (commit: string) => Promise<string> /** Read spec content at a commit (the remote source follows gitstub references) */ readSpec: (commit: string, specPath: string) => Promise<string> } ``` This implementation is also `jj`-aware — if the repo in question is a jj repo it will use jj revs instead of git, which is relevant because when you have a colocated jj/git repo, by default (without a rev specified) you probably want to look at `@`, but git's HEAD is at `@-`. If that doesn't mean anything to you, ignore it. <img width="788" height="507" alt="image" src="https://github.com/user-attachments/assets/5c180f0e-5772-46a3-98b1-b8a72ac334c6" />

david-crespo added 6 commits June 23, 2026 17:01

PoC: value-semantics update bodies (project, vpc_subnet, silo_quotas,…

a0d328a

… support_bundle)

Move wire->changeset conversion to nexus side (drop db-model dep on v…

4aefdfd

…ersions crate)

Make integration tests compile against value-semantics update bodies

164a1f8

move notes into versioned dir for now

a48bb28

Add prior-version regression test for project_update; capture PoC les…

743ad61

…sons in plan notes

Add session handoff note; record clearable-vs-preserve-on-omit findin…

25e029b

…g in plan

david-crespo commented Jun 24, 2026

View reviewed changes

david-crespo added 4 commits June 24, 2026 18:53

add temporary IdentityMetadataUpdateParamsStrict

26aa0b5

rename LLM-ish update_value_semantics to strict_put_bodies

e5381fe

delete bot-notes/ (moved to gist)

ccfda2f

david-crespo mentioned this pull request Jun 25, 2026

Convert all PUT bodies to require all fields #10675

Draft

david-crespo commented Jun 25, 2026

View reviewed changes

david-crespo mentioned this pull request Jun 26, 2026

tools: api-diff --local oxidecomputer/console#3264

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

PoC: change PUT bodies to require all fields#10673

PoC: change PUT bodies to require all fields#10673
david-crespo wants to merge 10 commits into
mainfrom
value-update-poc

david-crespo commented Jun 24, 2026 •

edited

Loading

Uh oh!

david-crespo Jun 24, 2026

Uh oh!

david-crespo Jun 25, 2026 •

edited

Loading

Uh oh!

david-crespo Jun 24, 2026 •

edited

Loading

Uh oh!

david-crespo Jun 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

david-crespo commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

API version compatibility

🤖 notes (gist)

Uh oh!

david-crespo Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

david-crespo Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

david-crespo commented Jun 24, 2026 •

edited

Loading

david-crespo Jun 25, 2026 •

edited

Loading

david-crespo Jun 24, 2026 •

edited

Loading