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 off postgres functions and on to the diesel ORM #774

Merged
merged 2 commits into from Nov 8, 2018

Conversation

Projects
None yet
5 participants
@elliott-davis
Member

elliott-davis commented Nov 2, 2018

Move the remaining API calls to postgres functions to leverage diesel directly
Leave the postgres functions in place for now so we don't break jobsrv

Signed-off-by: Elliott Davis elliott@excellent.io
Signed-off-by: Ian Henry ihenry@chef.io

@thesentinels

This comment has been minimized.

Contributor

thesentinels commented Nov 2, 2018

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

not quite done yet

Show resolved Hide resolved components/builder-db/src/models/account.rs Outdated
Show resolved Hide resolved components/builder-db/src/models/channel.rs
Show resolved Hide resolved components/builder-db/src/models/channel.rs Outdated
Show resolved Hide resolved components/builder-db/src/models/channel.rs Outdated

@elliott-davis elliott-davis requested review from cnunciato and mgamini as code owners Nov 6, 2018

.execute(conn)
// If this looks bad, it is. To ensure we get values here or die we have to execute queries to get the IDs first.
// I can hear the groaning already, "Why can't we just do a sub-select and let the database barf on insert"
// Great question - Because the typechecking happens in Rust and you wanted a type-safe language

This comment has been minimized.

@weiznich

weiznich Nov 6, 2018

As discussed in the diesel gitter, this catches the possibility that one of the subqueries return no value. If that's impossible because of certain database constrains it is possible to trick diesel into accepting that query:

To do this you need to extend diesel by some custom type magic:

struct AssertNotNull<T, ST>(T, ::std::marker::PhantomData<ST>);

fn unsafe_assert_not_null<T, ST>(t: T) -> AssertNotNull<T, ST>
where T: Expression<SqlType = Nullable<ST>>,
      ST: NotNull
{
    AssertNotNull(t, Default::default())
}

impl<T, ST> Expression for AssertNotNull<T, ST>
where T: Expression<SqlType = Nullable<ST>>,
      ST: NotNull,
{
    type SqlType = ST;
}

impl<DB, T, ST> QueryFragment<DB> for AssertNotNull<T, ST>
where T: QueryFragment<DB>,
      DB: Backend
{

    fn walk_ast(&self, pass: AstPass<DB>) -> Result<(), Error> {
        self.0.walk_ast(pass)
    }
}

This defines some type that changes the sql type of a nullable expression to not nullable. After that just call unsafe_assert_not_null on both queries and it will work.

Elliott Davis
Migrate off postgres functions and on to the diesel ORM
Move the remaining API calls to postgres functions to leverage diesel directly
Leave the postgres functions in place for now so we don't break jobsrv

Signed-off-by: Elliott Davis <elliott@excellent.io>
Signed-off-by: Ian Henry <ihenry@chef.io>

@elliott-davis elliott-davis changed the title from WIP: start converting queries to diesel to Migrate off postgres functions and on to the diesel ORM Nov 7, 2018

Removes comments, tweak to log messages
Signed-off-by: Ian Henry <ihenry@chef.io>

@chefsalim chefsalim merged commit 60c4b78 into master Nov 8, 2018

2 checks passed

DCO This commit has a DCO Signed-off-by line
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chefsalim chefsalim deleted the elliott/diesel_queries branch Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment