Extended returning support

[marlon-sousa]

In this pull request, we implement a feature eco-system which enables returning statements to be used in different back-ends.

We also configure returning method to take a list of columns instead of a full select expression, because in majority of cases omne will be asking for either none, * (all) or a list of columns of changed rows to be returned instead.

Associated issue is #156

[tyt2y3]

Thank you so much for the contribution.

First, I think Returing should be named as ReturningClause. It is nice to have its own data structure indeed, instead of reusing SelectExpr.
Second, while SQLite supports returning in latest version, it's support is not universal.
I think that it is the best to have MySQL always ignore the returning clause, no matter whether returning is enabled.

That say, I don't think the use of feature guard here is correct.
The problem is, cargo features are supposed to be independent. Imagine within a dependency tree, one crate turned on postgres while another crate turned on sqlite. It would subtly alter the generated query, such that the SQLite user would be surprised because it never turned on the returning feature, but it was turned on as a side effect of postgres.

I would sugguest to remove the feature guard.

All in all, nice work!

[marlon-sousa]

Hello,

Thank you so much. This is my first pull request for a project in rust language and I have been learning a lot lately.

It seems we have support for returning in MariaDB. Take a look at https://mariadb.com/kb/en/insertreturning/#:~:text=RETURNING%20returns%20a%20resultset%20of,alternatively%2C%20the%20specified%20SELECT%20expression.

If this support exists, we should be handling it accordingly. As far as I understand, mysql / maria db support are kind of unified, so this is why.

Secondly, I do agree with your view on cargo tools. But, never-the-less, I still need to be able to enable or disable returning in a way which is always enabled for postgres, conditionally enabled for other back ends. This is not so much important to sea-query because one using returning is supposed to know if their database supports it or not, but for other clients such as sea-orm we need a way of enabling / disabling it. Any suggestions?

[tyt2y3]

Sure. You are definitely welcomed by the Rust community )

Thank you for pointing that out with MariaDB.
Then we'd simply let the user decide. If the statement has a returning clause, just output it no matter what.
Do you have other concerns?

A note of reference on cargo feature rust-lang/cargo#4328

[marlon-sousa]

Feature guard has been removed.

[marlon-sousa]

I have choosen to not alter the version of the crate because you are controlling the release process.

[tyt2y3]

There is actually never a select * within sea-query. Do you have a specific reason you need it?
(It's hardly useful in Rust, because we aren't able to deserialize arbitrary select list anyway)

At the same time, Returning::Nothing is redundant too?

So, I'd suggest having a Returning::columns method (instead of cols) which is more consistent with Query::select()::columns()

[marlon-sousa]

Hello,

Here is why I have chosen at first to represent this as an enum:

"returning *" is a very very common pattern, which specifies that you want to return every single column of changed rows when performing delete, update and insert.

I thought that putting on users the burden of having to specify (list) every time all columns was just not ergonomic enough. Plus, the user would have to rewrite the code whenever the entity has columns added / removed, whereas using ::all this would not need to be changed.

Examples of potential users for ::All is the save method on sea orm, which will have to return the updated record anyways, or folks wanting to get ActiveModels with all columns filled for changed records.

Also, making a query with "returning *" is shorter and thus more performant than sending a query with "returning tablename.a, tablename.b, ... tablename.n", if you understand me.

For deserializing the framework would have to match on the returning enum to see the columns expected in the result set to deserialize the result.

match statement.returning {
  All => Deserialize(originalEntity::get_columns(),
  colls(c) => deserealize(c)
  Nothing => do_nothing_because_result_set_will_be_empty(),
}

The nothing was thought by another reason: suppose that one uses any kind of builder that implies that you want to have either the id or even all columns returned, but for your particular use-case you don't need this data. You could still use the builder and then, before performing the execution, just overwrite the returning to specify nothing instead, so that you still can keep your code fluent and alleviate database workload.

The cols helper just take and iterator and fill a list of columns.

I just wanted to show my point of view, but will of course code what you define. I just need a clearer definition on how the feature should be working.

Thank you for reviewing!

[tyt2y3]

I do not actually disagree on all or nothing. But the API style is slightly inconsistent with the rest of the library. They are perfectly fine on their own, and there is no good or bad. It's just nicer to be consistent.

So the following

.returning(Returning::Columns(vec![Glyph::Id.into_column_ref()]))

would be better to be

.returning(Query::returning().column(Glyph::Id))
.returning(Query::returning().columns(vec![Glyph::Id, Glyph::Name]))
.returning(Query::returning().all())
.returning(Query::returning()) // this is nothing

Basically a mirror of the SelectStatement interface.

[marlon-sousa]

A ... ok, I see. Well, I will implement that and let you know. I am not so familliar with sea-query so it is good to see governance over the public api.

[tyt2y3]

Now that SeaORM 0.3 has been released, we can focus on this task now.
Do you also happens to have an update?

[RomainMazB]

@marlon-sousa
Just a note on this one if you are still working on it: I submitted a SELECT * feature here: #219 .
I didn't follow the entire conversation but I noticed that this may ease the implementation of this feature.

[ikrivosheev]

@marlon-sousa do you want to finish this PR?

[ikrivosheev]

I will continue to work: #317