oxidecomputer / oxidecomputer/omicron

`DatastoreCollection::insert_resource` with an empty vec of resources fails weirdly and badly

Open
#10,788 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug database
Dominant language
Rust
Stars
572
Forks
97
Avg merge
2d 12h
Merged PRs (30d)
96

Description

What would you expect the following code to do?

https://github.com/oxidecomputer/omicron/blob/b3ca115ef5846bc297369eee62ce18edd628c2df/nexus/db-queries/src/db/collection_insert.rs#L670-L679

It turns out that it doesn't do that. It does a different, and much worse, thing:

thread 'db::collection_insert::test::test_insert_no_entries_into_collection' (3259368) panicked at nexus/db-queries/src/db/collection_insert.rs:680:10:
called `Result::unwrap()` on an `Err` value: DatabaseError(DatabaseError(Unknown, "at or near \"returning\": syntax error"))
stack backtrace:
    0: __rustc::rust_begin_unwind
                at /rustc/31fca3adb283cc9dfd56b49cdee9a96eb9c96ffd/library/std/src/panicking.rs:689:5
    1: core::panicking::panic_fmt
                at /rustc/31fca3adb283cc9dfd56b49cdee9a96eb9c96ffd/library/core/src/panicking.rs:80:14
    2: core::result::unwrap_failed
                at /rustc/31fca3adb283cc9dfd56b49cdee9a96eb9c96ffd/library/core/src/result.rs:1867:5
    3: unwrap<alloc::vec::Vec<nexus_db_queries::db::collection_insert::test::Resource, alloc::alloc::Global>, nexus_db_queries::db::collection_insert::AsyncInsertError>
                at /nix/store/69fxclbafjqhxl6rqfb8sim4hm9q664a-rust-default-1.96.1/lib/rustlib/src/rust/library/core/src/result.rs:1233:23
    4: {async_block#0}
                at ./src/db/collection_insert.rs:680:10

I'm not entirely sure how this manages to produce a SQL syntax error, but I guess whatever Diesel generates if you say diesel::insert(table).values(Vec::new()) must result in invalid syntax when wrapped in the CTE. This seems not great.

I'm not totally sure what we would prefer it to do if you tried to insert an empty vec into a collection. Either it should just silently do nothing and return an empty Vec without talking to the database at all (so that it also doesn't i.e. bump the collection object's rcgen), or it could return a special error type that says "hey, you tried to insert a whole bunch of nothing, probably don't do that".

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in nexus/db-queries/src/db/collection_insert.rs around lines 670-680 and read test_insert_no_entries_into_collection. Reproduce the empty-resource insertion case, then determine the intended behavior for the collection and rcgen before adding regression coverage. Done means the empty input no longer produces invalid SQL or panics, with the chosen behavior verified by the test.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
database
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.