Check field order of DB models against DB column order by david-crespo · Pull Request #10800 · oxidecomputer/omicron

david-crespo · 2026-07-10T21:58:45Z

While reviewing #10785 I noticed the order of the fields on the AlertReceiver DB model struct do not match the column order in schema.rs. This is only a problem if you try to use it with check_if_exists or some other DB thing which wants those to match, which runs into a deserialization error.

The big test in this PR that turns up the structs with mismatches is fine and not a big deal to keep around — the problem is that there's no way to guarantee you test all the relevant structs. The trait constraint on check_if_exists is designed to at least guarantee (or, let's say, make it hard to mess up) that you have a test for the column order on any struct that gets passed to check_if_exists.

failing models

     Summary [   0.214s] 239 tests run: 204 passed, 35 failed, 78 skipped
        FAIL [   0.015s] nexus-db-model column_order::tests::alert
        FAIL [   0.015s] nexus-db-model column_order::tests::alert_glob
        FAIL [   0.013s] nexus-db-model column_order::tests::audit_log_entry_init
        FAIL [   0.012s] nexus-db-model column_order::tests::bgp_config
        FAIL [   0.012s] nexus-db-model column_order::tests::bgp_peer_view
        FAIL [   0.013s] nexus-db-model column_order::tests::blueprint
        FAIL [   0.013s] nexus-db-model column_order::tests::bp_omicron_zone
        FAIL [   0.014s] nexus-db-model column_order::tests::db_host_phase2_desired_slots
        FAIL [   0.011s] nexus-db-model column_order::tests::db_omicron_measurements
        FAIL [   0.013s] nexus-db-model column_order::tests::device_auth_request
        FAIL [   0.011s] nexus-db-model column_order::tests::disk_runtime_state
        FAIL [   0.011s] nexus-db-model column_order::tests::instance_auto_restart
        FAIL [   0.012s] nexus-db-model column_order::tests::instance_runtime_state
        FAIL [   0.013s] nexus-db-model column_order::tests::inv_config_reconciler_status
        FAIL [   0.013s] nexus-db-model column_order::tests::inv_nvme_disk_firmware
        FAIL [   0.011s] nexus-db-model column_order::tests::inv_omicron_file_source_resolver
        FAIL [   0.011s] nexus-db-model column_order::tests::inv_remove_mupdate_override
        FAIL [   0.012s] nexus-db-model column_order::tests::inv_sled_agent
        FAIL [   0.013s] nexus-db-model column_order::tests::ip_pool
        FAIL [   0.012s] nexus-db-model column_order::tests::lldp_link_config
        FAIL [   0.015s] nexus-db-model column_order::tests::network_interface
        FAIL [   0.014s] nexus-db-model column_order::tests::physical_disk_adoption_request
        FAIL [   0.012s] nexus-db-model column_order::tests::reporter
        FAIL [   0.011s] nexus-db-model column_order::tests::resources
        FAIL [   0.012s] nexus-db-model column_order::tests::role_assignment
        FAIL [   0.013s] nexus-db-model column_order::tests::router_route
        FAIL [   0.012s] nexus-db-model column_order::tests::silo_utilization
        FAIL [   0.011s] nexus-db-model column_order::tests::sled_underlay_subnet_allocation
        FAIL [   0.013s] nexus-db-model column_order::tests::subnet_pool_member
        FAIL [   0.014s] nexus-db-model column_order::tests::switch_port_link_config
        FAIL [   0.012s] nexus-db-model column_order::tests::switch_port_settings_group
        FAIL [   0.012s] nexus-db-model column_order::tests::tuf_repo
        FAIL [   0.012s] nexus-db-model column_order::tests::vpc
        FAIL [   0.011s] nexus-db-model column_order::tests::vpc_firewall_rule

🤖 Summary

Diesel’s Queryable decoding is positional. A mismatch matters whenever a query returns columns in table order but decodes them into a model in field order. That includes check_if_exists, ordinary .load/.get_result calls without Model::as_select(), and insert/update .get_result calls without Model::as_returning(). If the mismatched columns have different types, this usually produces a deserialization error. If they have compatible types, it can silently put values into the wrong fields (demonstrated in test_queryable_silently_swaps_same_typed_columns and test_default_selection_silently_swaps_same_typed_model_fields).

We already test that the column order in schema.rs matches the dbinit-produced CockroachDB schema. This draft checks the other side of that invariant by comparing the SQL generated for Model::as_select() with the table’s all_columns().

Of the 239 existing selectable models, 204 match exactly. The 35 failures break down into:

22 full-row models with the same columns in a different order
13 deliberate projection models that only select a subset of their table

The projection models need to be classified separately rather than “fixed.” For the 22 actual order mismatches, reordering the struct fields could itself break custom queries whose select lists were written to match the current field order, so each model’s positional decode sites need to be audited before changing it.

The FullRowModel trait in this draft is an experiment in preventing the test list from becoming stale for check_if_exists: the helper requires a registered model associated with the same table, and registration also generates its order test. It does not provide exhaustive future coverage for the other implicit-selection and default-RETURNING paths. Doing that would require a lint or meta-test that discovers model declarations and verifies that each one is classified as either a full-row model or a projection.

Example of how this can go wrong

CREATE TABLE omicron.public.dns_version (
    dns_group omicron.public.dns_group NOT NULL,
    version INT8 NOT NULL,
    time_created TIMESTAMPTZ NOT NULL,
    creator TEXT NOT NULL,
    comment TEXT NOT NULL,
    PRIMARY KEY (dns_group, version)
);

Correct production model:

#[derive(Queryable, Insertable, Clone, Debug, Selectable)]
#[diesel(table_name = dns_version)]
pub struct DnsVersion {
    pub dns_group: DnsGroup,
    pub version: Generation,
    pub time_created: DateTime<Utc>,
    pub creator: String,
    pub comment: String,
}

Model with two same-typed fields accidentally reversed:

#[derive(Queryable)]
struct MisorderedDnsVersion {
    dns_group: DnsGroup,
    version: Generation,
    time_created: DateTime<Utc>,
    comment: String,
    creator: String,
}

The query does not specify a column order:

let row = dsl::dns_version
    .find((DnsGroup::Internal, version))
    .get_result_async::<MisorderedDnsVersion>(&*conn)
    .await?;

Diesel implicitly selects in table order:

SELECT dns_group, version, time_created, creator, comment
FROM dns_version
WHERE dns_group = $1 AND version = $2;

Given:

creator = "creator value"
comment = "comment value"

the decoded model contains:

assert_eq!(row.comment, "creator value");
assert_eq!(row.creator, "comment value");

Both columns are TEXT/String, so the query type-checks and deserializes successfully.

hawkw

I wonder if it is worth trying to factor out just the fix for the incorrect models in alert_rx and dns, so that we can merge the fix separately while the test stuff is still under review?

hawkw · 2026-07-10T22:16:52Z

+    };
+}
+
+full_row_models! {


i love that we are automatically generating tests for this. i wonder if there's also a way to detect models that don't have these tests, so that we can also get a warning when one has forgotten to write such tests. perhaps there is a way to use the list of tables that nexus-db-schema generates for test builds for this?

Heh yes, I just added a second paragraph to the intro above — the trait is designed to at least ensure we've checked any struct that gets used with check_if_exists, but we would have to get elaborate to really check all relevant structs.

🤖's idea on how to do this

The designed-but-unimplemented fix is a meta-test in nexus-db-model (deliberately deferred; it's elaborate and the marginal value is guarding against future forgetfulness, not finding present bugs):

Add syn as a dev-dependency.

#[test] walks nexus/db-model/src/**/*.rs via CARGO_MANIFEST_DIR, parses each file with syn, and collects every struct with #[diesel(table_name = ...)] that derives Queryable/Selectable — exactly the logic already validated in the scratchpad script (it found 241 structs: 239 Selectable + 2 Insertable-only).

Assert every collected struct appears in exactly one of two macros:

full_row_models! — table-keyed (1:1 with tables once projections are out), exact column-order test + FullRowModel impl.

projections! — the "exempt list with teeth": no trait impl, but a weaker generated test asserting the model's columns are an in-order subsequence of the table's columns. Projections feed parents via #[diesel(embed)], so their relative order still matters. This makes exemption a verified claim rather than a comment.

Simplest comparison wiring: have the macros also emit a static array of (struct_name, table_name) string pairs for the meta-test to diff against, avoiding a second parse of column_order.rs.

Misclassification is bounded: a full-row model wrongly listed as a projection passes the subsequence test only until the table grows a column it lacks, and check_if_exists rejects it regardless (no trait impl).

Resulting chain when implemented: meta-test forces every model into a list → lists generate order tests → order tests pin model ↔ schema.rs → crdb_alignment_test pins schema.rs ↔ dbinit-produced database. Residual trust: none of the classification is external — a wrong "projection" label is caught late (see 4) but not never.

the trait is designed to at least ensure we've checked any struct that gets used with check_if_exists,

Oh, I see, making that a requirement of check_if_exists seems good enough honestly.

hawkw · 2026-07-10T22:18:19Z

    }

+    #[tokio::test]
+    async fn test_queryable_silently_swaps_same_typed_columns() {


do we really need regression tests for this since (IIUC) the new column_order tests should also catch regressions here? i presume this test was written to demonstrate the original bug that motivated this...

Yeah, I don't think these are to keep, this was to prove the problem is real.

david-crespo · 2026-07-10T22:23:49Z

For sure, I think some or most of the order mismatches can just be fixed directly in the meantime.

david-crespo · 2026-07-10T22:25:48Z

🤖 audit of whether we are actually hitting the bug (no)

No current production path triggers the mismatch.

Two structs could silently corrupt values under implicit table-order decoding:

PhysicalDiskAdoptionRequest: swaps model and serial.
InvNvmeDiskFirmware: rotates three small integers when next_active_slot is non-null.

Their current reads use as_select() and returns use as_returning(), so neither is presently vulnerable. The other 20 mismatches encounter incompatible types and would fail rather than silently decode.

Custom collection-insert paths for Vpc, RouterRoute, NetworkInterface, VpcFirewallRule, and WebhookSecret are also safe: the helper generates RETURNING from ResourceType::as_returning().

SwitchPortSettingsGroup has no query-layer use, and none of the 22 uses check_if_exists.

🤖 full audit

Goal

Audit the 22 full-row Diesel models with field order differing from schema.rs in PR #10800. Determine which mismatches can silently swap same-typed values and whether production query paths decode those models positionally (implicit table selection, default RETURNING, or an explicit select list matching model field order).

Method

For each model:

Compare model field order with table column order.
Identify displaced columns and whether their Rust/SQL types are mutually compatible.
Find all query and mutation result-decoding sites for the model.
Classify each site as model-directed (as_select/as_returning), implicit table-order positional, explicit positional, or not a decode site.
Record whether a real path can error or silently misassign values.

Findings

No current production query decodes any of the 22 models from an implicit
table-order selection or default RETURNING list.

Two models are capable of a successful but incorrect full-row positional
decode:

Model	Table/model mismatch	Result under table-order decode	Current decode path
`PhysicalDiskAdoptionRequest`	Table has `model, serial`; model has `serial, model`	Always swaps the two `String` values	All reads use `as_select`; insert uses `as_returning`
`InvNvmeDiskFirmware`	Table has `number_of_slots, active_slot, next_active_slot`; model has `active_slot, next_active_slot, number_of_slots`	With non-null `next_active_slot`, rotates three compatible small integers silently; with null `next_active_slot`, decoding into non-null `number_of_slots` errors	The only read uses `as_select`; insertion does not return a row

The other 20 mismatches eventually put incompatible SQL representations in a
model field, so a complete default table-order decode would fail rather than
silently return a model:

Model	First decisive incompatible displacement
`Alert`	timestamp / alert-class enum
`BgpConfig`	UUID / nullable text
`Blueprint`	timestamp / integer generation
`BgpPeerView`	the middle integer fields are compatible, but the final ASN/router-lifetime displacement is integer-width incompatible
`BpOmicronZone`	UUID / disposition enum
`DeviceAuthRequest`	text / UUID
`InvSledAgent`	integer byte count / CPU-family enum
`IpPool`	integer generation / IP-version enum
`LldpLinkConfig`	timestamp / nullable network address
`NetworkInterface`	small integer / nullable network address
`RoleAssignment`	text and UUID fields cross positions
`RouterRoute`	UUID / route-kind enum
`SiloUtilization`	integer CPU count / boolean discoverability
`SledUnderlaySubnetAllocation`	UUID / small integer
`SwitchPortLinkConfig`	text name / nullable UUID
`SwitchPortSettingsGroup`	UUID / text name
`TufRepo`	artifact hash / nullable timestamp
`VpcFirewallRule`	protocol array / action enum
`Vpc`	text DNS name / integer VNI
`WebhookSecret`	nullable timestamp / UUID

Decode-site audit

Alert, BgpConfig, BgpPeerView, Blueprint, BpOmicronZone,
DeviceAuthRequest, InvNvmeDiskFirmware, InvSledAgent, IpPool,
LldpLinkConfig, PhysicalDiskAdoptionRequest, RoleAssignment,
SiloUtilization, SledUnderlaySubnetAllocation, SwitchPortLinkConfig,
TufRepo, and direct reads/updates of NetworkInterface, RouterRoute,
VpcFirewallRule, Vpc, and WebhookSecret use Model::as_select() or
Model::as_returning().
NetworkInterface, RouterRoute, Vpc, VpcFirewallRule, and
WebhookSecret also pass through the collection-insert CTE. That helper
constructs its RETURNING list from ResourceType::as_returning(), so these
paths are model-ordered despite ending in get_result_async::<ResourceType>.
The guarded alert insert receives an inner insert query whose RETURNING
list is Alert::as_returning().
SwitchPortSettingsGroup has no query-layer references; only its model
declaration, conversion implementation, and the new order-test registration
refer to it.
None of the 22 is used with check_if_exists.

Conclusion

The mismatches are latent hazards, not evidence of current data corruption.
Reordering the two silent-capable structs is safe with respect to all current
query sites. Reordering the other 20 also does not conflict with a custom
select list written in their old field order: their model decode sites are
generated from as_select/as_returning, or return no model row.

check column order of all full-row models

21f9b79

david-crespo force-pushed the check-column-order branch from c42b718 to a71f6ca Compare July 10, 2026 21:59

test positional DB model decoding

8db321e

david-crespo force-pushed the check-column-order branch from a71f6ca to 8db321e Compare July 10, 2026 22:07

hawkw reviewed Jul 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Check field order of DB models against DB column order#10800

Check field order of DB models against DB column order#10800
david-crespo wants to merge 2 commits into
fix-deleted-resource-updatesfrom
check-column-order

david-crespo commented Jul 10, 2026 •

edited

Loading

Uh oh!

hawkw left a comment

Uh oh!

hawkw Jul 10, 2026

Uh oh!

david-crespo Jul 10, 2026 •

edited

Loading

Uh oh!

hawkw Jul 11, 2026

Uh oh!

hawkw Jul 10, 2026

Uh oh!

david-crespo Jul 10, 2026

Uh oh!

david-crespo commented Jul 10, 2026 •

edited

Loading

Uh oh!

david-crespo commented Jul 10, 2026

Goal

Method

Findings

Decode-site audit

Conclusion

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

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

Uh oh!

🤖 Summary

Example of how this can go wrong

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

🤖's idea on how to do this

Uh oh!

hawkw Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

hawkw Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

david-crespo Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

david-crespo commented Jul 10, 2026

🤖 audit of whether we are actually hitting the bug (no)

Goal

Method

Findings

Decode-site audit

Conclusion

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

david-crespo commented Jul 10, 2026 •

edited

Loading

david-crespo Jul 10, 2026 •

edited

Loading

david-crespo commented Jul 10, 2026 •

edited

Loading