oxidecomputer / oxidecomputer/omicron
`DatastoreCollection::insert_resource` with an empty vec of resources fails weirdly and badly
Nobody has claimed this yet.
- 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?
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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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