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

Extensibility of AppContext #522

Open
yinho999 opened this issue Apr 5, 2024 · 16 comments · May be fixed by #777
Open

Extensibility of AppContext #522

yinho999 opened this issue Apr 5, 2024 · 16 comments · May be fixed by #777
Labels
enhancement New feature or request

Comments

@yinho999
Copy link
Contributor

yinho999 commented Apr 5, 2024

Feature Request

The current AppContext does not allow users to extend the struct which forces the user to create a lot of workaround when working with extensions/initializers.

Describe the solution you'd like
We can create a thin wrapper which exposes the inner structure to the user and allow the user to add/modify the wrapper as they want.

use loco_rs::prelude::*;
struct LocalAppContext{
  app_context: AppContext
}

Discord Discussion
Discord link

@yinho999 yinho999 added the enhancement New feature or request label Apr 5, 2024
@jondot
Copy link
Contributor

jondot commented Apr 15, 2024

I'm not sure about the implication of extending AppContext in this way, I'm wondering how it will pass around the different components without breaking?

@yinho999
Copy link
Contributor Author

I totally agree this framework refactor is massive and may introduce breaking among components. Can we explore alternative approaches that could allow us to achieve the same extensibility on AppContext?

@schungx
Copy link

schungx commented Apr 16, 2024

Some way to make db in AppContext refer to a DatabaseTransaction would be great, since I can simply pass those transactions to existing API endpoints without having to create double-cover versions for each API with a &impl ConnectionTrait parameter instead of State<AppContext>...

But I'm not sure how this can be done easily as the system has no way to know which transaction an API endpoint wants...

In that respect, it would be very helpful to be able to create a temp AppContext from a DatabaseTransaction... that would work also as I can then reuse the majority of the JSON API's...

Something like:

let trans = ctx.begin().await?;
let new_ctx = ctx.clone_with_transaction(&trans);    // Clone the context but replace the connection with a transaction

// ... use State(new_ctx) when calling other JSON API's
let item = add_one(State(new_ctx), params).await?;

trans.commit().await?;

@ElijahJohnson5
Copy link

ElijahJohnson5 commented Jul 7, 2024

How about something like adding an ExtraAppContext type to the hooks trait that is used in a generic app context

pub trait Hooks: Send + Sync {
    type ExtraAppContext: Send + Sync + Clone + 'static;
   ...rest of hooks trait
}

then making AppContext generic like

pub struct AppContext<T>
where
    T: Send + Sync,
    T: Clone,
{
    /// The environment in which the application is running.
    pub environment: Environment,
    #[cfg(feature = "with-db")]
    /// A database connection used by the application.    
    pub db: DatabaseConnection,
    /// An optional connection pool for Queue, for worker tasks
    pub queue: Option<Pool<RedisConnectionManager>>,
    /// Configuration settings for the application
    pub config: Config,
    /// An optional email sender component that can be used to send email.
    pub mailer: Option<EmailSender>,
    // An optional storage instance for the application
    pub storage: Arc<Storage>,
    // Cache instance for the application
    pub cache: Arc<cache::Cache>,
    // Extra user defined data
    pub extra: Option<T>,
}

I tried this with a local copy of the repo, and it ended up working well, the only thing is having to add type parameters everywhere app context is used. Here is the diff with my fork master...ElijahJohnson5:loco:master

@yinho999
Copy link
Contributor Author

yinho999 commented Jul 8, 2024

How about something like adding an ExtraAppContext type to the hooks trait that is used in a generic app context

pub trait Hooks: Send + Sync {
    type ExtraAppContext: Send + Sync + Clone + 'static;
   ...rest of hooks trait
}

then making AppContext generic like

pub struct AppContext<T>
where
    T: Send + Sync,
    T: Clone,
{
    /// The environment in which the application is running.
    pub environment: Environment,
    #[cfg(feature = "with-db")]
    /// A database connection used by the application.    
    pub db: DatabaseConnection,
    /// An optional connection pool for Queue, for worker tasks
    pub queue: Option<Pool<RedisConnectionManager>>,
    /// Configuration settings for the application
    pub config: Config,
    /// An optional email sender component that can be used to send email.
    pub mailer: Option<EmailSender>,
    // An optional storage instance for the application
    pub storage: Arc<Storage>,
    // Cache instance for the application
    pub cache: Arc<cache::Cache>,
    // Extra user defined data
    pub extra: Option<T>,
}

I tried this with a local copy of the repo, and it ended up working well, the only thing is having to add type parameters everywhere app context is used. Here is the diff with my fork master...ElijahJohnson5:loco:master

This works for most of the use cases. However, if we are implementing third-party traits/functions directly on AppContext, it will be impossible due to orphan rule.

One example is implementing Cookie Key onto AppState

// demo/src/app.rs
use loco::prelude::*;
use axum_extra::extract::cookie::Key;

// This is not going to work since both trait and struct are not in the same crate
impl FromRef<AppContext> for Key {
    fn from_ref(state: &AppContext) -> Self {
        state.0.key.clone() // You can also use `Key::generate()` to avoid the implementation
    }
}

Error

Only traits defined in the current crate can be implemented for arbitrary types [E0117]

@ElijahJohnson5
Copy link

Is that not a problem already with the current AppContext?

@yinho999
Copy link
Contributor Author

yinho999 commented Jul 8, 2024

Is that not a problem already with the current AppContext?

Yeah, it is an existing problem with the current AppContext. And this can be solved by moving AppContext into an AppContextTrait, and creating a LocalAppContext with AppContextTrait. Or create a LocalAppContext wrapper directly around AppContext. Then we can workaround the orphan rule since the LocalAppContext struct has been created in the project.

@ElijahJohnson5
Copy link

Is the plan to move to something like an AppContextTrait in the future?

@mstallmo mstallmo linked a pull request Sep 22, 2024 that will close this issue
@jondot
Copy link
Contributor

jondot commented Sep 25, 2024

I want to thank everyone for the discussion and especially @mstallmo for providing something we can look at and play with. As was discussed the trait+generics solution is indeed breaking in many places.

I'm wondering, as we're about to let users "digest" all these code changes they have to make, what are the use cases and motivations for extending or creating a custom AppContext.

For my educational purposes, maybe I'm missing something.
Currently, the way to extend and bring your own custom context is two-fold, and relates to lifecycles:

  1. Request lifecycle: we "ride" on Axum's conceptual lifecycle solutions: AppContext takes the main state, and then we recommend each user add their Extension and that's how they get a "custom lifecycle vibe".
  2. Worker lifecycle: this is the major place where no Extension exists, and so people might look up to AppContext. But, however, workers have a "take from queue, execute, die" lifecycle, so even if they needed a custom shared state (say, a MongoDB connection), this could be solved via singular initialization and then sharing via Arc/Mutex, which is probably what Axum does under the hood (or demands that stuff you put in Extension be shareable safely).

So, if we are able to provide some "sugar" over these use cases maybe the issue is solved without creating the generic appcontext and breaking code.

At this point I worry -- is there a use case I missed that forces extension of AppContext?

@mstallmo
Copy link

mstallmo commented Sep 25, 2024

I'll share what motivated me to implement this change and why I think it will be useful for others. Hopefully this can shed some light on why this change would be worth the disruption.

My Specific Case

I was looking into using Loco with an InertiaJS based frontend for an app that I'm working on and found the axum-inertia crate that implements the InertiaJS protocol for Axum. To use this crate with an Axum app the InertiaContext struct must be present in the State.

From their documentation this can either be done by using the InertiaContext struct as the application state or by implementing axum::extract::FromRef<T> for InertiaContext and including InertiaContext as a member of the struct used for State. For this crate specifically Extension cannot be used because of how FromRequestParts is implemented for the Inertia struct

General Case

I suspect that there are other crates in the wider Axum ecosystem that have a similar requirement to the one that I ran into with axum-inertia. In general, libraries that are created to work with Axum wouldn't expect that a user would not be able to modify the struct that is present in State directly since end users would be expected to be creating their own State struct for their Axum app.

There is an argument to be made that a solution would be to fork or create a new loco specific crate that does the same thing (something that I am also working on for inertia) that instead uses Extension instead of State to work with Loco.

With that said I think it would be good for Loco to be as compatible with the wider Axum ecosystem as possible as it both makes transitioning from an Axum app to a Loco app easier for users and gives them access to crates that were created to work with Axum without having to fork or create a new crate on their own.

@jondot
Copy link
Contributor

jondot commented Sep 25, 2024

Thanks. This is very interesting. I will spend some time learning what inertia is, but meanwhile I read the create source code and it seems that it very easily can use Extension.

in fact I’m almost convinced it should have used extension because it is exactly fitting for what extension is.

Talking over state means some thing that’s central. Loco took over state because it is central to the app as it is the core framework so it needs this freedom of operation.

I would actually submit a change to the inertia crate to move to use Extension or at least wonder why they didn’t.

@mstallmo
Copy link

Thanks. This is very interesting. I will spend some time learning what inertia is, but meanwhile I read the create source code and it seems that it very easily can use Extension.

You're 100% right on this. I have a fork of axum-inertia where I updated the crate to use extension instead of state and it was a very minimal change.

I ultimately decided to go down the road of making a loco specific version of the crate because I wanted nicer integration with the rendering facilities already present in loco and to make a smoother holistic experience for loco apps.

For these kinds of scenarios I think my general question is this: How many existing crates in the ecosystem use Extension over State in their implementation of FromRequestParts for internal structs?

@jondot
Copy link
Contributor

jondot commented Sep 25, 2024

I would say a lot. Just in Loco itself we use Extension for all the extensions we have that are optional. As long as the type is unique you can have as many as you want.

PS Even if you did not use Loco and had your own state, the intertia crate would force a refactor on you.

@mstallmo
Copy link

PS Even if you did not use Loco and had your own state, the intertia crate would force a refactor on you.

It would but it would be a minimal refactor, no? Just implementing FromRef<T> for InertiaContext for the existing state struct.

@jondot
Copy link
Contributor

jondot commented Sep 26, 2024

Correct, but Extension makes this completely transparent

@mstallmo
Copy link

Another point to consider in favor of allowing users to utilize custom State as well as Extension is compile time errors.

With extensions you do not get a compile time error if an extension is missing that is needed by a route handler. Instead this shows up at runtime with a panic or server error response. Since the FromRequestParts trait may not be implemented in a user's application it may not be immediately clear what behavior the missing extension will cause in the application (i.e. users may have to go digging through 3rd party libraries to find the unwrap that is called if an extension is not found).

Allowing for custom state gives user the ability to lean on Rust's strong compile time guarantees with respect to state used in their route handlers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants