/diesel

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Our PG upsert API is clunky and we should fix it #1166

Closed
sgrif opened this Issue Sep 15, 2017 · 1 comment

Comments

Assignees

@alexcameron89 alexcameron89

Labels
Projects
None yet
Milestone
  1.0
2 participants
@sgrif @alexcameron89
@sgrif
Member

sgrif commented Sep 15, 2017

I've really disliked using our upsert API in practice. You basically always need to pull records.on_conflict(...) into a local variable since it's going to span multiple lines, and it's really hard to name that variable. https://github.com/rust-lang/crates.io/blob/9c8947c4d171706f490dfd8dfeb932a714d252be/src/download.rs#L43-L50 https://github.com/rust-lang/crates.io/blob/9c8947c4d171706f490dfd8dfeb932a714d252be/src/categories.rs#L125-L140

I'd like to change our API to something more fluent, which more naturally allows for line breaks where they're likely to occur. I'd like to make the following changes:

// OLD
insert(records.on_conflict_do_nothing()).into(table)

// NEW
insert(records).into(table).on_conflict_do_nothing()

// OLD
insert(records.on_conflict(target, do_nothing())).into(table)

// NEW
insert(records).into(table).on_conflict(target).do_nothing()

// OLD
insert(records.on_conflict(target, do_update().set(...))).into(table)

// NEW
insert(records).into(table).on_conflict(target).do_update().set(...)

// ALTERNATE
insert(records).into(table).on_conflict(target).do_update_set(...)

// ALTERNATE 2
insert(records).into(table).on_conflict(target).do_update(...)

@sgrif sgrif added breaking change discussion desired labels Sep 15, 2017

@sgrif sgrif added this to the 1.0 milestone Sep 15, 2017

@sgrif sgrif added the mentoring available label Sep 15, 2017

@alexcameron89

This comment has been minimized.

Sign in to view
Contributor

alexcameron89 commented Sep 15, 2017

I'll give this one a try 👍

@sgrif sgrif assigned alexcameron89 Sep 15, 2017

sgrif added a commit that referenced this issue Sep 21, 2017

@sgrif
Deprecate `insert` in favor of `insert_into` 
This change deprecates inserts in the form `insert(values).into(table)`
in favor of `insert_into(table).values(values)`.

There are several reasons for this change:

- The new API more closely mirrors the SQL it creates.
- The old API conflicted with `Into::into`, meaning that if the argument
  to `insert` was wrong, the error message would be that `into` expected
  0 arguments and got 1.
- Things like `default_values` fit more nicely with this API (especially
  after the upsert changes in #1166 land)
- The API for inserting a select statement with an explicit column list
  will work better with this API

I've replaced all instances of `insert` with the new API, except for the
doctests of the deprecated functions.

(As a reminder in case you're wondering what the 0.99.0 is, the plan is
that the next version will be 1.0. 0.99.0 will be identical to 1.0,
except 1.0 will have no deprecated code)

Fixes #1167.
Loading status checks…
307fdfd

@sgrif sgrif referenced this issue Sep 21, 2017

Merged

Deprecate `insert` in favor of `insert_into` #1182

sgrif added a commit that referenced this issue Sep 21, 2017

@sgrif
Deprecate `insert` in favor of `insert_into` (#1182) 
This change deprecates inserts in the form `insert(values).into(table)`
in favor of `insert_into(table).values(values)`.

There are several reasons for this change:

- The new API more closely mirrors the SQL it creates.
- The old API conflicted with `Into::into`, meaning that if the argument
  to `insert` was wrong, the error message would be that `into` expected
  0 arguments and got 1.
- Things like `default_values` fit more nicely with this API (especially
  after the upsert changes in #1166 land)
- The API for inserting a select statement with an explicit column list
  will work better with this API

I've replaced all instances of `insert` with the new API, except for the
doctests of the deprecated functions.

(As a reminder in case you're wondering what the 0.99.0 is, the plan is
that the next version will be 1.0. 0.99.0 will be identical to 1.0,
except 1.0 will have no deprecated code)

Fixes #1167.
Loading status checks…
73778af

sgrif added a commit that referenced this issue Sep 22, 2017

@sgrif
Deprecate `on_conflict`, `do_nothing`, and `do_update` 
This deprecates the remaining PG upsert API from 0.16, and moves it onto
`InsertStatement` instead. This leads to a more fluent API which allows
line breaks to be more naturally added.

Additionally, our error messages are much more descriptive, and appear
much earlier with this API.

Fixes #1166.
Loading status checks…
64926bb

@sgrif sgrif referenced this issue Sep 22, 2017

Merged

Deprecate `on_conflict`, `do_nothing`, and `do_update` #1189

sgrif added a commit that referenced this issue Sep 22, 2017

@sgrif
Deprecate `on_conflict`, `do_nothing`, and `do_update` 
This deprecates the remaining PG upsert API from 0.16, and moves it onto
`InsertStatement` instead. This leads to a more fluent API which allows
line breaks to be more naturally added.

Additionally, our error messages are much more descriptive, and appear
much earlier with this API.

Fixes #1166.
Loading status checks…
53ab975

@sgrif sgrif closed this in #1189 Sep 23, 2017

sgrif added a commit that referenced this issue Sep 24, 2017

@sgrif
Allow `filter` to be called on `UpdateStatement` and `DeleteStatement` 
This change is in the same spirit as #1166. By allowing the method to be
chained as part of the query, instead of on the argument to a function,
it becomes much easier to introduce line breaks naturally.

Why isn't this `impl FilterDsl`?
--------------------------------

Today `FilterDsl` is structured in such a way that it assumes that it's
only implemented on valid complete queries, and that it will return a
valid complete query (partially to ensure that the call to `filter`
can't change the SQL type). Since these structs are only complete
queries when they have a returning clause (and
`IncompleteUpdateStatement` is never a complete query), we would have to
loosen those bounds.

We may want to make that change eventually, but for now I opted to
implement these as inherent methods.

Why not add `find` as well?
---------------------------

We could. To me, `update(users).set(...).find(1)` just feels off. Given
that `find` is typically much shorter, I've opted to exclude it for now.
Loading status checks…
0e13217

@sgrif sgrif referenced this issue Sep 24, 2017

Merged

Allow `filter` to be called on `UpdateStatement` and `DeleteStatement` #1192

sgrif added a commit that referenced this issue Sep 25, 2017

@sgrif
Allow `filter` to be called on `UpdateStatement` and `DeleteStatement` ( 
#1192)

This change is in the same spirit as #1166. By allowing the method to be
chained as part of the query, instead of on the argument to a function,
it becomes much easier to introduce line breaks naturally.

Why isn't this `impl FilterDsl`?
--------------------------------

Today `FilterDsl` is structured in such a way that it assumes that it's
only implemented on valid complete queries, and that it will return a
valid complete query (partially to ensure that the call to `filter`
can't change the SQL type). Since these structs are only complete
queries when they have a returning clause (and
`IncompleteUpdateStatement` is never a complete query), we would have to
loosen those bounds.

We may want to make that change eventually, but for now I opted to
implement these as inherent methods.

Why not add `find` as well?
---------------------------

We could. To me, `update(users).set(...).find(1)` just feels off. Given
that `find` is typically much shorter, I've opted to exclude it for now.
Loading status checks…
947afbd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment