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

Add Actix #156

Merged
merged 28 commits into from
Nov 5, 2018
Merged

Add Actix #156

merged 28 commits into from
Nov 5, 2018

Conversation

asonix
Copy link
Collaborator

@asonix asonix commented Oct 28, 2018

It's ready It's ready It's ready It's ready take a look

@asonix
Copy link
Collaborator Author

asonix commented Oct 29, 2018

I'm going to invite commentary on these changes, now, even though I'm not quite finished getting it all sorted out.

@asonix
Copy link
Collaborator Author

asonix commented Oct 29, 2018

Note to self: Don't implement ResponseError for custom error types. Define a trait like AardwolfResponseError and impl<T: AardwolfResponseError> ResponseError for T

This allows us to share error-response-generating code between Rocket and Actix, and also means all our response errors should maintain consistent formatting.

@asonix
Copy link
Collaborator Author

asonix commented Oct 30, 2018

Getting real close to feature parity, now

@asonix asonix changed the title WIP: Add Actix Add Actix Oct 31, 2018
@asonix
Copy link
Collaborator Author

asonix commented Nov 1, 2018

Note to self: Implement a way to figure out which template to use based on the route that was hit (i.e. Implement FromRequest for some TemplateName type). This will make it much cleaner to handle errors in request guards.

Maybe this can exist in the following form:

pub enum RenderOrRedirect {
    Render(TemplateName),
    Redirect(RouteName),
}

// Maybe do something a little more magical here, like looking for the '/auth/sign_in' template if the path is '/auth/sign_in'
impl From<&str> for RenderOrRedirect {
    fn from(path: &str) -> Self {
        "/auth/sign_in" => RenderOrRedirect::Render("sign_in".to_owned()),
        ...,
        _ => "404",
    }
}

impl FromRequest<AppConfig> for RenderOrRedirect {
    type Config = ();
    type Result = Self;

    fn from_request(req: &HttpRequest, _: &Self::Config) -> Self::Result {
        RenderOrRedirect::from(req.path())
    }
}

Copy link
Collaborator

@BanjoFox BanjoFox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Staaaaaaaaaaaaaaaaaaaaaaaaare
( ⚆ _ ⚆ )

(Jokingly replying to "Don't pay attention.)
(( Will wait for PR to review properly :3c ))

@asonix
Copy link
Collaborator Author

asonix commented Nov 2, 2018

Maybe instead of RenderOrRedirect I do Render and Redirect separately, and derive FromRequest for both of them. Then they can be used interchangeably where they make sense.

@asonix
Copy link
Collaborator Author

asonix commented Nov 2, 2018

I also don't like the current AardwolfError trait. I might also have it require Serialize so we can pass it into render functions, and remove the name and description required methods.

I do think that the kind is important, because it gives our errors a little more context for what kind of HTTP response should be given, but ultimately any redirect will be a 303, so it doesn't really matter unless we're rendering directly.

Maybe AardwolfError shouldn't exist and we should just have TemplateError and RedirectError (although currently RedirectError doesn't do anything) and then TemplateError would know which template, and what kind of response to use

@asonix
Copy link
Collaborator Author

asonix commented Nov 2, 2018

I'm also thinking about implementing a macro called perform which will do something like this

let res = perform!(state, [
    FirstAction::new(input_data),
    SecondAction::new(other_required_data),
    ThirdAction::new(more_required_data),
]);

where each action implements DbAction (or a similar trait). This will make routes that need to perform database operations much cleaner in the futures world, since we won't need to .and_then(|data| {}) everything, and we won't need to deal with manually mapping errors, since the macro could handle mapping each action's error into the return error type.

@asonix
Copy link
Collaborator Author

asonix commented Nov 2, 2018

ultimately, the perform!(state, []) macro would only be necessary until async/await becomes standard, because then we could write it like

async our_route(...) -> Result<item, err> {
    let first = await!(perform_db_action(FirstAction::new(input_data)))?;
    let second = await!(perform_db_action(SecondAction::new(other_required_data).with(first)))?;
    Ok(await!(perform_db_action(ThirdAction::new(more_required_data).with(second)?)
}

which, to be fair, is still a bit ugly, but we could work out a cleaner look for that when the time comes

I need to get going, but I'll fix this tomorrow
I have a macro and I'm excited by it
@BanjoFox
Copy link
Collaborator

BanjoFox commented Nov 3, 2018

I do like the idea of having more... specific errors (TemplateError and RedirectError), over simply AardwolfError because it allows for more specific troubleshooting down the line :)

@asonix
Copy link
Collaborator Author

asonix commented Nov 3, 2018

The perform macro is complete

Here's the sync version for aardwolf-rocket

(aardwolf-actix has an async version)

macro_rules! perform {
 ( $state:expr, $start:expr, $error_type:ty, [] ) => {{
     // If there's no operations, return the initial value as the result
     Ok($start) as Result<_, $error_type>
 }};
 (
     $state:expr,
     $start:expr,
     $error_type:ty,
     [
         ($wrapper:ty => $first:expr),
         $(($wrappers:ty => $rest:expr),)*
     ]
 ) => {{
     use $crate::action::Action;

     // Pass the initial value to the first operation
     // and wrap it inside a DbActionWrapper of ValidateWrapper
     let wrapper: $wrapper = $first.with($start).into();

     // Perform the action, passing the provided state as an argument
     let res = wrapper.action($state.clone());

     // Call the perform_inner macro with the result of the first operation
     // and a list of the remaining operations
     perform_inner!($state, $error_type, res, [ $(($wrappers => $rest),)* ])
 }};
}

macro_rules! perform_inner {
    (
        $state:expr,
        $error_type:ty,
        $first:expr,
        []
    ) => {{
        // If there are no remaining operations, coerce the error
        // from the provided result into the $error_type
        $first.map_err(|e| {
            let e: $error_type = e.into();
            e
        })
    }};
    (
        $state:expr,
        $error_type:ty,
        $first:expr,
        [
            ($wrapper:ty => $item:expr),
            $(($wrappers:ty => $items:expr),)*
        ]
    ) => {{
        use $crate::action::Action;

        // Coerce the error from the provided result into the
        // $error_type
        $first
            .map_err(|e| {
                let e: $error_type = e.into();
                e
            })
            .and_then(|item| {
                // Pass the result of the previous operation to the next operation and
                // wrap it inside a DbActionWrapper or ValidateWrapper
                let wrapper: $wrapper = $item.with(item).into();

                // Perform the action
                let res = wrapper.action($state.clone());

                // call the perform_inner macro with the result of the action, and the
                // list of remaining operations
                perform_inner!($state, $error_type, res, [ $(($wrappers => $items),)* ])
            })
    }};
}

This macro takes 4 arguments:

  • The application state
  • An initial value to pass to the first operation
  • The error type that it should coerce errors into
  • A list of operations, and their corresponding wrapper types

Why pass the state?

  • In the async version, the macro needs to know about the state so it can pass the operations to the Database Actor. In the sync version, the state is simply a &diesel::pg::PgConnection.

Action trait

The Action trait is an abstraction over DbAction and Validate (And possibly other things) to allow for them to both be used as perform! operations.

pub trait Action<T, E>
where
    E: Fail,
{
    fn action(self, db: &PgConnection) -> Result<T, E>;
}

An action is simply a type that has an action function that returns a result.

Example Usage

    let res = perform!(
        state,
        id.into_inner(),
        PersonaDeleteError,
        [
            (DbActionWrapper<_, _, _> => FetchPersona),
            (DbActionWrapper<_, _, _> => CheckDeletePersonaPermission::new(user.0)),
            (DbActionWrapper<_, _, _> => DeletePersona),
        ]
    );

This code does the following:

  • Fetches a Persona from the database with id id
  • Checks if the User has permission to delete that persona
  • Deletes that persona

If any of these steps produced an error, it was coerced into the PersonaDeleteError type.

@asonix
Copy link
Collaborator Author

asonix commented Nov 3, 2018

diesel warnings are now silenced due to an issue with how nightly builds work: diesel-rs/diesel#1785

Copy link
Collaborator

@BanjoFox BanjoFox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve.
This is one heck of a large batch of work. Much appreciated friend! :D

@asonix asonix merged commit 8ccbbc5 into master Nov 5, 2018
@BanjoFox BanjoFox mentioned this pull request Nov 8, 2018
@asonix asonix deleted the asonix/add-actix branch November 21, 2018 16:25
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.

2 participants