Database queries with Diesel #192

david-crespo · 2021-08-08T19:02:57Z

~~This PR does the minimum required to fetch a project with Diesel. This test surfaced a number of little problems we would have to solve to use Diesel to replace most of our SQL wrangling code.~~

This PR converts all of our database queries to use Diesel.

It's worth noting that the "unreleased changes" part of the Diesel changelog was referred to somewhere as Diesel 2.0 (can't remember where). Here's the Diesel 2.0 milestone. It has been in the works since 2019, though, so I don't think there's much reason to expect it to come out in the next month or two.

We're using Diesel master because it makes the async stuff much easier. We may be able to help move Diesel 2.0 along.

Things to solve

Note: this was an early to-do list and is therefore not even close to comprehensive.

david-crespo · 2021-08-08T19:05:41Z

omicron-common/src/api/external/error.rs

+        }
+    }
+}
+


this probably won't work out because in order to produce our Error, we need other info from context that DieselError can't give us

Yeah, that might be okay - just means that we can define more featureful converters from DieselError to Error, depending on the type.

I imagine that NotFound triggers more work for the caller to add context, and everything else is more-or-less passthrough.

I've gone ahead and updated these with two explicit constructions that provide enough context to pass all tests.

This is great! Exactly what I was picturing, but I didn't know how to do it.

david-crespo · 2021-08-08T19:06:40Z

omicron-common/src/sql/dbinit.sql

+-- ) VALUES (
+--     'schema_version',
+--     '1.0.0'
+-- );


comment this out because the table doesn't have a primary key and diesel doesn't like that. real solution would be to just give it a primary key

david-crespo · 2021-08-08T19:11:14Z

omicron-nexus/src/db/diesel_schema.rs

+        time_modified -> Timestamptz,
+        time_deleted -> Nullable<Timestamptz>,
+    }
+}


This file is generated by the diesel setup. The only one I'm using is project, but I left the rest here to show what it looks like.

Dumb question, but what input file is disel reading to emit this output?

not a dumb question at all, I believe this is generated by talking to the running database. yikes

david-crespo · 2021-08-08T19:12:28Z

omicron-nexus/src/lib.rs

+        let manager = r2d2::ConnectionManager::<diesel::PgConnection>::new(
+            &config.database.url.url(),
+        );
+        let dpool = r2d2::Pool::new(manager).unwrap();


These two lines took me so long to figure out. The documentation is not so good.

david-crespo · 2021-08-08T19:14:25Z

omicron-nexus/src/db/model.rs

+    pub time_created: DateTime<Utc>,
+    pub time_modified: DateTime<Utc>,
+    pub time_deleted: Option<DateTime<Utc>>,
+}


Note that these properties are not nested under identity like in the existing Project because I don't know how to tell Diesel to do that. It wants columns in the DB to map straight to struct fields. There may well be a way to annotate this so that nesting under identity works.

david-crespo · 2021-08-08T20:59:57Z

omicron-nexus/src/nexus.rs

+        project
+            .filter(name.eq(name_.as_str()))
+            .first::<db::model::DieselProject>(&*conn)
+            .map_err(|e| e.into())


If we import db::diesel_schema::project instead of the dsl thing we have to write

project::table .filter(project::columns::name.eq(name.as_str()))

smklein · 2021-08-09T13:58:25Z

omicron-common/src/api/external/error.rs

+        }
+    }
+}
+


Yeah, that might be okay - just means that we can define more featureful converters from DieselError to Error, depending on the type.

I imagine that NotFound triggers more work for the caller to add context, and everything else is more-or-less passthrough.

smklein · 2021-08-09T13:59:48Z

omicron-common/src/api/external/mod.rs

@@ -43,6 +43,7 @@ pub type CreateResult<T> = Result<T, Error>;
 pub type DeleteResult = Result<(), Error>;
 /** Result of a list operation that returns an ObjectStream */
 pub type ListResult<T> = Result<ObjectStream<T>, Error>;
+pub type ListResultVec<T> = Result<Vec<T>, Error>;


I'm not 100% sure this is a type we should be adding.

I thought the reason for having ListResult defined like this was to say "if you're going to return a list, do it via an ObjectStream so we can stream the results back to the caller".

yeah, this is just a placeholder to make it work. diesel doesn't support streams

smklein · 2021-08-09T14:01:50Z

omicron-nexus/src/db/diesel_schema.rs

+        time_modified -> Timestamptz,
+        time_deleted -> Nullable<Timestamptz>,
+    }
+}


Dumb question, but what input file is disel reading to emit this output?

smklein · 2021-08-09T14:02:46Z

omicron-nexus/src/db/model.rs

+pub struct DieselProject {
+    pub id: Uuid,


I'm assuming that we would probably just end up calling this Project if there's no ambiguity, right? No need to have db::model::Project and db::model::DieselProject simultaneously?

Yeah definitely. This is just to have this working without breaking everything else

smklein · 2021-08-09T18:12:07Z

Just pushed the conversions for Name, demonstrating what's necessary to get ToSql / FromSql for one of these "primitive" types. Seems to compose well, and also allows for the "String -> Name" conversion to throw a deserialization error if something goes wrong.

…atastore

omicron-nexus/src/db/datastore.rs

smklein

So projects + instances are using diesel here, tests are passing, and errors are getting propagated successfully.

This might be a good point to stop and get some preliminary feedback before the rest of the world gets converted: what do we think about the shape of this?

FWIW, if I continue the conversion, I am confident the following files can be removed entirely:

omicron-nexus/db/sql_operations.rs
omicron-nexus/db/sql.rs
omicron-nexus/db/operations.rs
omicron-nexus/db/schema.rs

migrations/00000000000000_diesel_initial_setup/up.sql

smklein · 2021-08-13T05:07:54Z

omicron-common/src/api/external/error.rs

+        }
+    }
+}
+


I've gone ahead and updated these with two explicit constructions that provide enough context to pass all tests.

omicron-nexus/src/db/datastore.rs

smklein · 2021-08-13T05:10:25Z

omicron-nexus/src/db/datastore.rs

+            .filter(dsl::time_deleted.is_null())
+            .limit(pagparams.limit.get().into())
+            .into_boxed();
+        let query = match pagparams.direction {


This is the "new" version of pagination. Can probably be refactored into a generic function, but the TL;DR is:

limit query to pagparams.limit

filter based on the marker, if one is supplied

apply order to query

smklein · 2021-08-13T05:11:34Z

omicron-nexus/src/db/datastore.rs

+        diesel::update(dsl::project)
+            .filter(dsl::time_deleted.is_null())
+            .filter(dsl::name.eq(name))
+            .set(&updates)


The neat part about diesel changesets is that they view Option arguments as "basically not supplied", so the whole set of things-which-can-be-changed can just be passed directly here.

smklein · 2021-08-13T05:17:18Z

omicron-nexus/src/db/datastore.rs

+            .on_conflict(dsl::id)
+            .do_nothing()


The implementation of "insert_unique_idempotent_and_fetch" seems pretty trivial here - on conflict, don't do anything, and get the result always.

omicron-nexus/src/db/datastore.rs

smklein · 2021-08-13T05:19:12Z

omicron-nexus/src/db/datastore.rs

+        let instance = diesel::update(dsl::instance)
+            .filter(dsl::time_deleted.is_null())
+            .filter(dsl::id.eq(instance_id))
+            .filter(dsl::instance_state.eq_any(vec![stopped, failed]))


There are a couple different ways of stating it, but this is a valid way to check for "instance state being in any of the following valid states".

smklein · 2021-08-13T05:20:35Z

omicron-nexus/src/http_entrypoints_external.rs

-    let view_list =
-        to_list::<db::model::Project, Project>(project_stream).await;
-    Ok(HttpResponseOk(ScanByNameOrId::results_page(&query, view_list)?))
+    // TODO filter out non-ok things like to_list does


@david-crespo not sure I understand this comment - projects is already a vec of Projects, so it exclusively contains "OK things".

Ah ok. If the maybe_object.is_ok() was merely an artifact of unwrapping stuff out of an async stream, then this is not needed.

…ateParams

smklein · 2021-08-26T19:16:38Z

FYI, there is a corresponding steno change here: oxidecomputer/steno#11

smklein · 2021-08-30T19:44:07Z

FYI, there is a corresponding steno change here: oxidecomputer/steno#11

Ended up closing that change, implementing via newtype pattern within Omicron only.

david-crespo · 2021-08-31T21:09:09Z

since I can't approve my own PR

smklein

Nice job, @david-crespo 😁

david-crespo added 5 commits August 7, 2021 15:17

diesel successfully generates a schema.rs

3e85aa3

successfully make a dang query

121204b

actually filter by name + test for that

4ee670e

placeholder Into<DieselError> for Error so we can be cool

c62a807

delete datastore project_fetch to show it's been replaced

0d418fa

david-crespo commented Aug 8, 2021

View reviewed changes

Project2 -> DieselProject for clarity

5c9c727

david-crespo commented Aug 8, 2021

View reviewed changes

david-crespo added 3 commits August 8, 2021 16:57

diesel 1.4.4 -> 1.4.6 (latest). not sure where I got 1.4.4 from

644b4d1

uhhh it's not actually async

ad7d5e1

get projects list works

ab3ab9e

david-crespo force-pushed the diesel-test branch from d1ef487 to ab3ab9e Compare August 9, 2021 13:57

smklein reviewed Aug 9, 2021

View reviewed changes

Add ToSql/FromSql for Name

b353504

david-crespo mentioned this pull request Aug 9, 2021

SQL query logging #194

Closed

smklein added 3 commits August 11, 2021 14:55

Fix pagination order

39ed898

Unify 'DieselProject' and 'Project', move connection pool access to d…

efe71ce

…atastore

Avoid leaking DB abstraction into nexus.rs

520411f

david-crespo commented Aug 11, 2021

View reviewed changes

omicron-nexus/src/db/datastore.rs Outdated Show resolved Hide resolved

smklein added 4 commits August 12, 2021 19:37

Convert Instances to using Diesel

880ddd1

Fix tests

842314b

Fix error handling, pagination. Tests should ACTUALLY pass now

6b34785

Update time_modified when updating project

fbb56a4

smklein reviewed Aug 13, 2021

View reviewed changes

[bugfix] Don't need to set time_modified twice! Already in ProjectUpd…

fcf5070

…ateParams

smklein mentioned this pull request Aug 26, 2021

Add diesel conversions for "single column types" oxidecomputer/steno#11

Closed

smklein added 2 commits August 28, 2021 08:43

Async works, but using a breaking change on 1.4.7

76ddf44

Diesel 2.0 - building, tests passing

f6de7ca

david-crespo mentioned this pull request Aug 30, 2021

PoC - Absorbing views and params into Nexus #191

Closed

smklein added 4 commits August 30, 2021 12:07

Diesel 2.0, full async support, improved trait bounds

330c82c

fmt

46548ed

Define steno / DB conversions within Omicron using newtype pattern

c4c5f35

Clippy, newtype wrappers

cc824a1

smklein added 5 commits August 30, 2021 15:46

... actually fix clippy lints

ed1f050

Merge branch 'main' into diesel-test

43fc587

Minor cleanups, docs

e1baefc

s/instance_state/state

6ce512c

Readme

c59c9fa

fmt

a68d4ba

smklein marked this pull request as ready for review August 31, 2021 21:17

smklein approved these changes Aug 31, 2021

View reviewed changes

smklein added 4 commits September 1, 2021 09:43

Improved Mac Address type safety

654ccf6

Simplify connection pool

f494e77

Clarify TODO in saga_recovery

0b01683

Comment cleanup

de45714

This was referenced Sep 1, 2021

[diesel][cleanup] Make use of Diesel "Selectable" to cut down on repetition #209

Closed

[diesel] CTE should reference columns explicitly #210

Open

smklein merged commit ee3fa44 into main Sep 1, 2021

smklein deleted the diesel-test branch September 1, 2021 15:21

davepacheco mentioned this pull request Sep 1, 2021

build fails due to missing libpq #213

Closed

davepacheco mentioned this pull request Oct 20, 2021

Instance deletion produces incorrect status codes #324

Closed

davepacheco mentioned this pull request Dec 8, 2022

[db] Audit usage of "ON CONFLICT DO NOTHING RETURNING ..." #1168

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Database queries with Diesel #192

Database queries with Diesel #192

david-crespo commented Aug 8, 2021 •

edited

Loading

david-crespo Aug 8, 2021

smklein Aug 9, 2021

smklein Aug 13, 2021

david-crespo Aug 13, 2021 •

edited

Loading

david-crespo Aug 8, 2021

david-crespo Aug 8, 2021

smklein Aug 9, 2021

david-crespo Aug 9, 2021

david-crespo Aug 8, 2021

david-crespo Aug 8, 2021 •

edited

Loading

david-crespo Aug 8, 2021

smklein Aug 9, 2021

smklein Aug 9, 2021

david-crespo Aug 9, 2021

smklein Aug 9, 2021

smklein Aug 9, 2021

david-crespo Aug 9, 2021

smklein commented Aug 9, 2021

smklein left a comment

smklein Aug 13, 2021

smklein Aug 13, 2021

smklein Aug 13, 2021

smklein Aug 13, 2021

smklein Aug 13, 2021

smklein Aug 13, 2021

david-crespo Aug 13, 2021

smklein commented Aug 26, 2021

smklein commented Aug 30, 2021

david-crespo commented Aug 31, 2021

smklein left a comment

Database queries with Diesel #192

Database queries with Diesel #192

Conversation

david-crespo commented Aug 8, 2021 • edited Loading

Things to solve

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-crespo Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-crespo Aug 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smklein commented Aug 9, 2021

smklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smklein commented Aug 26, 2021

smklein commented Aug 30, 2021

david-crespo commented Aug 31, 2021

smklein left a comment

Choose a reason for hiding this comment

david-crespo commented Aug 8, 2021 •

edited

Loading

david-crespo Aug 13, 2021 •

edited

Loading

david-crespo Aug 8, 2021 •

edited

Loading