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

Migrate to Diesel 2.0.0 #3928

Closed
10 of 14 tasks
neysofu opened this issue Sep 14, 2022 · 2 comments · Fixed by #5002
Closed
10 of 14 tasks

Migrate to Diesel 2.0.0 #3928

neysofu opened this issue Sep 14, 2022 · 2 comments · Fixed by #5002
Assignees

Comments

@neysofu
Copy link
Member

neysofu commented Sep 14, 2022

Official announcement: https://diesel.rs/news/2_0_0_release.html
Changelog: https://diesel.rs/changelog.html
Migration guide: https://diesel.rs/guides/migration_guide.html

Migrating to Diesel 2.0.0 is desirable because we'd enjoy support for the latest features, as well as up-to-date community examples and guides. Some minor performance improvements are expected as well.

I'm providing a list of the main migration painpoints that are relevant for graph-node, identified during exploratory work in the filippo/diesel-2 branch. This list may not be complete and I haven't delved deep into some of these, thus I'm not sure how much more hidden complexity there is, if any. Checked items are those that I managed to fully resolve, unchecked items need more work (and they're by far the most complex).

  • Derive attributes: all attributes in the form of #[sql_type = "diesel::sql_types::Numeric"] must become #[diesel(sql_type = diesel::sql_types::Numeric)]. This is trivial and can be done via search and replace.

  • RunQueryDsl methods (and many others) now require mutable references to connections. I'd imagine this requires to turn Connection::conn into a MaybeOwnedMut and changing most self and conn references to mutable; we can also explore less intrusive solutions like a RefCell.

  • .eq(any(...)) calls should be replaced with .eq_any(...). See what the migration guide says.

  • Update diesel-derive-enum to a Diesel 2.0.0 -compatible version (2.0.0.-rc.0 is currently out, with 2.0.0 expected soon).

  • Update diesel-dynamic-schema to >= 0.2.0.

  • Update diesel_migrations and diesel_derives to >= 2.0.0.

  • QueryFragment::walk_ast now takes extra lifetime parameters, e.g.:

    impl<'a> QueryFragment<Pg> for BlockRangeLowerBoundClause<'a> {
        fn walk_ast<'b>(&'b self, out: AstPass<'_, 'b, Pg>) -> QueryResult<()> {
            out.unsafe_to_cache_prepared();
    
            out.push_sql("lower(x) = ");
            // ...
    
            Ok(())
        }
    }

    This creates a problem when binding values, e.g.: out.push_bind_param::<Text, _>(&s). While Diesel 1.4 would in this example require a String, Diesel 2.0 requires a &'b String. As far as I can tell, this means we must precompute all variables that we wish to bind and store them alongside Self upon its instantiation.

    Most of this also applies to all of our own methods and functions that take an AstPass, even outside of QueryFragment implementations.

  • Slightly different type signature for the ToSql and FromSql traits. While there's migration examples in the migration guide, I suspect there to be lifetime issues similar to QueryFragment.

  • Literal SQL (diesel::dsl::sql) now requires an explicit type parameter in most situations, as it can't be inferred as easily.

  • Types that derive diesel_derive_enum::DbEnum (only SubgraphHealth, I believe) now needs the extra attribute #[DieselTypePath = "MappingType"]. Read diesel_derive_enum's README for info.

  • conn.execute(raw_query_string) now becomes sql_query(raw_query_string).execute(conn).

  • Whenever doing a ORDER BY count(*), the literal SQL sql::<Integer>("count(*)") hack to get around Diesel's aggregate rules doesn't seems to work anymore. Replacing the literal SQL with count_star() and calling .group_by() before .select() seems to do the trick.

  • DatabaseErrorKind::__Unknown is now DatabaseErrorKind::Unknown.

  • More restrictive type bounds for all LoadQuery implementors. This macro seems to provide the necessary trait implementations so that $typ falls under wildcard trait implementation rules:

    macro_rules! impl_load_query {
        ( $typ:ident ) => {
            impl<'a, Conn> RunQueryDsl<Conn> for $typ<'a> {}
    
            impl<'a> QueryId for $typ<'a> {
                type QueryId = ();
    
                const HAS_STATIC_QUERY_ID: bool = false;
            }
    
            impl<'a> Query for $typ<'a> {
                type SqlType = diesel::sql_types::Untyped;
            }
        };
    }

    It compiles, but I haven't checked whether the resulting trait implementation behaves the same.

@mangas
Copy link
Contributor

mangas commented Jan 6, 2023

I made a new attempt at this on branch filipe/diesel2-upgrade. I managed to fix a lot of the issues but the lifetimes around AstPass seem like they might require a larger refactor

@lutter
Copy link
Collaborator

lutter commented Mar 14, 2023

About the mutable connections: in several places where the connection winds up as part of a struct we can remove it from the struct and pass it explicitly (for example in copy.rs)

In other places, e.g., primary::Connection, the struct is really just a wrapper to make it clearer that the code is manipulating the primary, but there's no added protection from having that struct, and we could probably get rid of it in favor of free-standing functions that take a &mut PgConnection and just use the module namespace to clarify that this is affecting the primary, similar to how deployment.rs exposes methods to change deployment metadata but without defining a struct for that.

@fordN fordN linked a pull request Mar 12, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants