Add support for MySql

[weiznich]

This one should be quite easy:

• [ ] Add corresponding feature flags to the wundergraph_derive, wundergraph and wundergraph_cli crate
• [ ] Generate an impl for LoadingHandler for diesel::mysql::Mysql in wundergraph_derive/src/wundergraph_entity.rs
• [ ] Generate an impl for WundergraphBelongsTo for diesel::mysql::Mysql in wundergraph_derive/src/belonging_to.rs
• [ ] Generate an impl for BuildFilterHelper for diesel::mysql::Mysql in wundergraph_derive/src/build_filter_helper.rs
• [ ] Implement HandleInsert and HandleBatchInsert for diesel::mysql::Mysql in wundergraph/src/query_builder/mutations/insert
• [ ] Implement ApplyOffset for diesel::mysql::Mysql in wundergraph/src/query_builder/selection/offset.rs
• [ ] See if you get at least wundergraph_example running with the mysql backend. This includes porting the SQL migrations here to the MySQL dialect.

[p-alik]

@weiznich, I finished all the steps except following one easily.

[ ] Implement HandleInsert and HandleBatchInsert for diesel::mysql::Mysql in wundergraph/src/query_builder/mutations/insert

Without possibility to get id of inserted item, which mysql/mariadb offers by mysql_insert_id I can hardly implement handle_insert and handle_batch_insert reliably.

Sure, it is possible to implement both methods in style of Sqlite https://github.com/weiznich/wundergraph/blob/b0cf967c7579b0936f990fcfcdc2be3e109ae0c7/wundergraph/src/query_builder/mutations/insert/sqlite.rs#L55 but I rather reluctant to do that. In my opinion it would be good to do an amendment in diesel crate to support in guide described get_result approach, which implemented for PostgreSQL back end only.

For instance DBIc::Class::ResultSet->create uses mysql_insert_id (over DBD::mysql) to return a complete entry on insert. I'm deeply missing this feature in diesel.

[weiznich]

First of all a big thanks for working on this.It's great to see that this is done 👍

Without possibility to get id of inserted item, which mysql/mariadb offers by mysql_insert_id I can hardly implement handle_insert and handle_batch_insert reliably. Sure, it is possible to implement both methods in style of Sqlite but I rather reluctant to do that. In my opinion it would be good to do an amendment in diesel crate to support in guide described get_result approach, which implemented for PostgreSQL back end only.

This situation is quite unfortunate, as neither SQlite nor MySQL do provide a reliable way to get back the inserted (or updated) column in such queries. The following statement more or less describes the reasoning on diesel side: There is LAST_INSERT_ID() which returns the last inserted id for a given statement, but as far as I'm aware that function can have issues if there are multiple concurrent inserts involved. So that's no general solution. The next best thing that someone can do is the order + limit hack done in the SQLITE backend. This isn't much nicer but coupled with an transaction this should at least return reliable results, while it communicates the general tradeof much clearer. As both solutions have their short comings and restrictions the diesel team has decided to not hack some solution together that only works most of the time reliably, but make this an explicit user decision (especially as this involves transactions which are potential blocking/expensive/…). On the other hand PostgreSQL does support RETURNING clauses which basically allows you to return an arbitrary select like clause from your insert statement. Those are used to implement get_results() and co via a single query for PostgreSQL. As far as I'm aware neither Sqlite nor Mysql do support a similar concept. (It is interesting that MariaDB does support returning clauses since a few versions. Some changes like this are basically the reason why I would like to see a distinct backend for MariaDB in diesel itself, as we could then add support for such constructs there.)

Hopefully that explains at least a bit why things are as they are.

[p-alik]

There is LAST_INSERT_ID() which returns the last inserted id for a given statement, but as far as I'm aware that function can have issues if there are multiple concurrent inserts involved.

Indeed, LAST_INSERT_ID() is SQL equivalent to API function mysql_insert_id. Well, it's not the same what PostgreSQL offers, butt it's good enough for single or sequential inserts. According to documentation

For LAST_INSERT_ID(), the most recently generated ID is maintained in the server on a per-connection basis. It is not changed by another client.

I think both handle_insert and handle_batch_insert could be implemented with LAST_INSERT_ID()

[weiznich]

Diesel does not provide a LAST_INSERT_ID() query ast not out of the box, but you can simply define one in wundergraph using the no_arg_sql_function! macro

[p-alik]

Thank you for pointing it out to me. I'm struggling with type casting yet. Unfortunately I couldn't figure out by my self how to solve this issue. May I ask your help for that?

[weiznich]

So that's a really tricky one. On the surface that question is simple as we would only need to adjust those trait bounds to make this work. Below the surface there is a much more fundamental problem. Now that I look at that code again I remember why I've written the sqlite code that way and have not used LAST_INSERT_ID(). The issue there is basically, if we use LAST_INSERT_ID() we would restrict the implementation to entities with single integer primary key, not for all tables with some possible composite primary key. Getting that working for the general case in Sqlite is quite simple, as there is an "hidden" table column called rowid which is basically a INTEGER PRIMARY KEY AUTOINCREMENT primary key like column, so we can just order by that column and be done.

For MySQL it's unfortunately not possible because there is no such hidden column. Now we could try to emulate this behavior using the ROW_NUMBER() window function, but that does require sorting by id, which would loose insert order for the general case (Think of a Text/Uuid primary key here).

That all written down I'm not really sure what's the right way forward here. Possible solutions are:

1. Do not provide out of the box HandleInsert/HandleBatchInsert support for MySQL as it requires specific knowledge over the table
2. Restrict the HandleInsert/HandleBatchInsert impls to only work with tables with integer primary keys for MySQL. In this case we would need to check that it's possible to provide manual impl's for all other cases and document that accordingly. In this case you would need to change the constraints in such a way that they require the following:

T::PrimaryKey: Expression<SqlType = Integer>,
<&'static L as Identifiable>::Id: UnRef<'static, UnRefed = i32>,

• remove everything related to the generic Id parameter + fix all compiler error messages.

3. Find some general solution for this problem, I've not came up with yet.

[p-alik]

1. is the cheapest approach
2. is impossible for me yet because I've only few months of Rust experiences
3. 52759f93d76c88ea94698ecf1f9ffe982b3cb24a contains an attempt to implement HandleInsert triet, which compiles error-free. Unfortunately even wundergraph_example can't be compiled on top of it. I'll appreciate any assistance in this regard

[weiznich]

Your solution is basically that what I've proposed with my second point :wink: The consequences that tables with composite keys are not supported are kind of expected...

[p-alik]

Your solution is basically that what I've proposed with my second point

Indeed, but I wasn't aware of the consequences for wundergraph_example.

[p-alik]

Do you have any advice how this attempt to implement HandleInsert trait could be improved?

[weiznich]

I think you don't need nearly as many generic constraints as you've used there to make this work for a concrete type. See this impls for example.

[p-alik]

Indeed, this seems to be the right approach.

[p-alik]

2. Restrict the HandleInsert/HandleBatchInsert impls to only work with tables with integer primary keys for MySQL. In this case we would need to check that it's possible to provide manual impl's for all other cases and document that accordingly. In this case you would need to change the constraints in such a way that they require the following:

T::PrimaryKey: Expression<SqlType = Integer>,
<&'static L as Identifiable>::Id: UnRef<'static, UnRefed = i32>,

In case of https://github.com/weiznich/wundergraph/blob/ffbb883eea169d1750659e038d7051a84b21f6e7/wundergraph_example/src/lib.rs#L104-L109 for instance it would cause E0119

It looks like to move forward with #48 the only option I have:

1. Do not provide out of the box HandleInsert/HandleBatchInsert support for MySQL as it requires specific knowledge over the table

and provide implementation of HandleInsert/HandleBatchInsert traits for all wundergraph_cli- and wundergraph_example-tables.

Do you agree?

[weiznich]

If I remember correctly the PR to your repo I've linked above does choose a hybrid approach: It does implement HandleInsert/HandleBatchInsert for all tables that have an integer primary key + it provides specific impls in wundergraph_example for all tables that do not have a single integer key. It would be great to generate those, but for the first iteration I'm fine with just having some documentation that it could be required to write those manually and show how it is done.

[p-alik]

If I remember correctly the PR to your repo I've linked above does choose a hybrid approach

I beg your pardon, I've completely overseen it is a PR. Such a pity! It would have save me some hours

[p-alik]

In my opinion at least for CI sake wundergraph_bench should support MySql also. I've started appropriate implementation but couldn't move further because I'm not aware how to deal with https://github.com/weiznich/wundergraph/blob/ffbb883eea169d1750659e038d7051a84b21f6e7/wundergraph_bench/src/api.rs#L555-L557

Souldn't the struct provide both two other fields like FilmActor? https://github.com/weiznich/wundergraph/blob/ffbb883eea169d1750659e038d7051a84b21f6e7/wundergraph_bench/src/api.rs#L288

[weiznich]

Sorry for taking that long to answer, but life is currently really busy here.

In my opinion at least for CI sake wundergraph_bench should support MySql also. I've started appropriate implementation but couldn't move further because I'm not aware how to deal with

:+1:

Souldn't the struct provide both two other fields like FilmActor?

Yes that struct should provide also the other two keys. Seems like this is a bug in wundergraph-cli, we should probably not skip not autoincrement primary keys anywhere. Could you fill an issue for that? Beside of that is fine for the mysql PR to just manually add the missing fields there.

[p-alik]

There is absolutely no reason to say sorry! I see how busy you are with diesel, which obviously has higher priority. Could you add bug label to the issue #55, please.