Skip to content
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

Faillible Queryable - Non-generic Queryable version experiment #2559

Closed
wants to merge 11 commits into from

Conversation

Ten0
Copy link
Member

@Ten0 Ten0 commented Nov 9, 2020

One approach to fixing #2523

@weiznich
Copy link
Member

Just to drop a note here: I'm currently busy with other work related stuff, so I will likely not be able to look at this for the next week or so.

@weiznich weiznich added this to the 2.0 milestone Nov 27, 2020
@Ten0
Copy link
Member Author

Ten0 commented Dec 18, 2020

So the test fails there seems to highlight a compatibility break with type inference where the compiler cannot infer "oh there's a single type I know for deserializing BigInt, that's i64, let me just use that" anymore, though I'm not sure why it has this impact.

If going for this approach, readmes and diesel_cli would still have to be updated.

@Ten0 Ten0 mentioned this pull request Dec 21, 2020
3 tasks
@weiznich
Copy link
Member

Another important change here is that we cannot anymore write different impls depending on the type returned by some query or the backend. Doing that is basically required to port diesel_cli. As consequence I would like to see how that could be solved before looking any further into this.

@Ten0
Copy link
Member Author

Ten0 commented Dec 28, 2020

Another important change here is that we cannot anymore write different impls depending on the type returned by some query or the backend. Doing that is basically required to port diesel_cli. As consequence I would like to see how that could be solved before looking any further into this.

From #2523 (comment):

it would probably always be implemented without an additional layer of genericity, so it could probably use FromSqlRow instead of Queryable, as the conflicting impl error wouldn't trigger.

As it turns out, we're actually using a bit of genericity here:

impl<DB> FromSqlRow<(sql_types::Text, sql_types::Text, sql_types::Text), DB> for ColumnInformation
where
DB: Backend + UsesInformationSchema,
(String, String, String):
FromStaticSqlRow<(sql_types::Text, sql_types::Text, sql_types::Text), DB>,

but for some reason it still works. (Maybe thanks to the local UsesInformationSchema trait.)

@Ten0
Copy link
Member Author

Ten0 commented Dec 28, 2020

One more downside of the non-generic version: rust error messages suggest less correct approaches to fixing FromSqlRow<ST, DB> is not implemented for T:
https://github.com/diesel-rs/diesel/pull/2559/checks?check_run_id=1617158750#step:6:258

-error[E0277]: the trait bound `UserWithToFewFields: diesel::Queryable<(diesel::sql_types::Integer, diesel::sql_types::Text, diesel::sql_types::Nullable<diesel::sql_types::Text>), _>` is not satisfied
+error[E0277]: the trait bound `(i32, std::string::String): diesel::deserialize::FromSql<(diesel::sql_types::Integer, diesel::sql_types::Text, diesel::sql_types::Nullable<diesel::sql_types::Text>), _>` is not satisfied

@weiznich
Copy link
Member

Closed as I would like to go with #2599

@weiznich weiznich closed this Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants