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

Resources #18

Closed
SergioBenitez opened this issue Oct 9, 2016 · 13 comments
Closed

Resources #18

SergioBenitez opened this issue Oct 9, 2016 · 13 comments
Labels
enhancement A minor feature request feedback wanted User feedback is needed

Comments

@SergioBenitez
Copy link
Member

SergioBenitez commented Oct 9, 2016

An idea I've been toying with is the addition of "resources" to Rocket. This would be similar to ActiveResource in Rails. The motivation is that many apps are CRUD-like, and this would remove some of boilerplate of doing this as well as provide more structure when necessary.

As an example, consider a Todo resource that be created, read, updated, and deleted. Here's what writing this might look like:

#[resource("todo")]
struct Todo {
    description: String,
    completed: bool
}

impl RocketResource for Todo {
    type ReadResponse = DBResult<Either<Template, JSON<String>>>;

    fn read(id: ID, format: ContentType) -> Self::ReadResponse {
        let todo = try!(Database::find_todo(id));
        if format.is_json() {
            Either::B(JSON(todo.to_string()))
        } else {
            Either::A(Template::render("todo", &todo))
        }
    }
}

fn main() {
    rocket::ignite().register("/", resources![Todo]).launch()
}

Note the associated type ReadResponse. This can go away entirely once impl Trait is allowed as a return type of trait methods. Then the return type can simply be impl Response.

This example above would create four routes: POST todo/, GET todo/<id>, PATCH todo/<id>, and DELETE todo/<id> mapping to the four RocketResource trait methods: create, read, update, delete. PUT should likely map to update as well, though PATCH is more appropriate. All of the RocketResource methods should have default implementation that return a "not-implemented" error of some sort. In the example above, a GET todo/<id> request would be routed to the read method, running the logic in the example. Note also the ContentType: resources will respond to any content-type, and the first content-type in the Accept header will be passed in.

Here's a sketch of what the trait might look like:

trait RocketResource {
    type CreateResponse = DefaultResponse;
    type ReadResponse = DefaultResponse;
    type UpdateResponse = DefaultResponse;
    type DeleteResponse = DefaultResponse;

    fn create(&self, format: ContentType) -> Self::CreateResponse {
        DefaultResponse::new()
    }

    fn read(id: ID, format: ContentType) -> Self::ReadResponse {
        DefaultResponse::new()
    }

    fn update(id: ID, format: ContentType) -> Self::UpdateResponse {
        DefaultResponse::new()
    }

    fn delete(id: ID, format: ContentType) -> Self::DeleteResponse {
        DefaultResponse::new()
    }
}

Of note is the create method which takes in an &self. This is because Rocket will automatically parse the incoming request from forms and JSON. This means that the resource must implement FromForm and Deserialize. An alternative is to simply pass in Data instead and let the user decide what to do. Another alternative is to pass in the accepted content-type to the resource attribute: #[resource("todo", format = "application/json, text/html")]. In this case, Rocket will return an error on any other kind of request format, and the user is guaranteed that ContentType will always be one of those in the format argument. And, the user will only have to implement FromForm if they specify text/html, and Deserialize if they specify application/json, which sounds nice.

Anyway, this is a developing thought. impl Trait in method return types would make this really nice.

@SergioBenitez SergioBenitez added enhancement A minor feature request feedback wanted User feedback is needed labels Oct 9, 2016
@LLBlumire
Copy link
Contributor

I agree with PUT being handled like PATCH, as this is increadibly common and needed for some compatability with legacy software. The fact you had first class handlign for patch at all instead of just the original http methods surprised me in the first place.

@mehcode
Copy link
Contributor

mehcode commented Jan 21, 2017

I'm 👎 as this can be easily implemented outside of Rocket (from what I can tell). Something like rocket_resource would be a neat crate that can be recommended and shown in examples, but I'd rather the focus be on keeping Rocket extensible and have strong extensions than a larger base.

@shssoichiro
Copy link
Contributor

shssoichiro commented Jan 21, 2017

I'm +1 for the exact reason that @mehcode is -1. I'd actually like to see a Rust framework that takes a batteries included approach like Rails. Rust is very nice for this, because you don't pay for what you don't use (at runtime, anyway--your compile time might go up a little), and you can even use cargo's features to make it more modular. Extensibility is not at odds with having a larger base. But in the end it depends on which direction @SergioBenitez wants to take Rocket.

@SergioBenitez
Copy link
Member Author

As we think about this issue, keep in mind that Rocket already separates core concepts from extensions via the rocket and rocket_contrib crates.

@mehcode
Copy link
Contributor

mehcode commented Jan 21, 2017

Don't get me wrong. I love this idea. But I could also love #142.

As we think about this issue, keep in mind that Rocket already separates core concepts from extensions via the rocket and rocket_contrib crates.

💯


In my opinion, this should be drafted into a separate crate rocket_resource. It can be marketed on the main website under "Extensions" or something. When it passes some pre-determined level of "accepted" it can be placed into rocket_contrib. This format empowers the community.

@Meralis40
Copy link

I've been thinking a lot about this lately, and here's the sketch of the trait I propose :

pub trait RocketResource {
    type Id: for<'a> request::FromParam<'a>;
    type Requirements: for<'a, 'r> request::FromRequest<'a, 'r>;
    type CreateResponse: for<'r> response::Responder<'r>;
    type ReadResponse: for<'r> response::Responder<'r>;
    type UpdateResponse: for<'r> response::Responder<'r>;
    type DeleteResponse: for<'r> response::Responder<'r>;
    type Input: data::FromData;

    fn create(input: Self::Input, format: http::ContentType, requirements: Self::Requirements)
        -> Self::CreateResponse;

    fn read(id: Self::Id, format: http::ContentType, requirements: Self::Requirements)
        -> Self::ReadResponse;

    fn update(input: Self::Input, id: Self::Id, format: http::ContentType, requirements: Self::Requirements)
        -> Self::UpdateResponse;

    fn delete(id: Self::Id, format: http::ContentType, requirements: Self::Requirements)
        -> Self::DeleteResponse;
}

From @SergioBenitez proposition :

  • I've added a parameter for requirements, like retrieving state,
  • made id type generic,
  • and also made data from request generic

@hackeryarn
Copy link

@Meralis40 would this trait be derivable? I think the big win of having a resource is being able to add one annotation and have everything flushed out.

If it's not possible to make this derivable, maybe a rails style cli generator could be an option here. The generator could take care of the routes and a common diesel model. I also think the generator would be great for a working example, including tests.

I've been thinking of the best approach for this as well. Maybe the answer is a combination of a trait and generator, I just wanted to get this idea out there.

@Meralis40
Copy link

Meralis40 commented Apr 10, 2017

I think the best approch is the combination of a trait and generator.
I don't think this should be derivable : the underlying implementation is up to the user, not rocket_codegen.
Maybe we could add this in rocket_contrib first ?

Also, :+1 for common diesel model.

I will try to make a working example.

Edit : i've modified the todo example to use the trait, see at https://github.com/Meralis40/rocket-resource
After doing this, I think we can combine a generator (for default implementation), and a attribute
(for generate the boilerplate).

@TedDriggs
Copy link

If I had a resource which required an API key to access, how would I express that guard while using the trait?

@Meralis40
Copy link

The more I think about it, the more I think this trait is not a good fit to Rocket in the current form : for example you can't have your API key on body, it must be an custom header. In addition, if you want several guards for access (imagine an API key with a database and user session loaded), you need another struct for that, only to comply with the trait.

Also, if we want to have different data depending on user been admin or not (for example), the user need to implement the logic on the trait (when currently, the auto-generated handler helps).

Another potential design can be something like this :

#[resource("todo")]
pub mod todo {
    struct Todo {
        description: String,
        completed: bool,
    }

    #[resource_get("/<id>")]
    fn read_one(id: ID, format: ContentType) -> DBResult<Either<Template, JSON<String>>> {
        let todo = try!(Database::find_todo(id));
        if format.is_json() {
            Either::B(JSON(todo.to_string()))
        } else {
            Either::A(Template::render("todo", &todo))
        }
    }
}

fn main() {
    rocket::ignite().register("/", resources![todo]).launch()
}

With:

  • the resource attribute collecting all resource_* handlers, and creating routes,
  • the resource_* attributes creating the handlers like current routing attributes, without the route information.

This design is less code for users, but requires to handle this on rocket_codegen.
An open question is the current lint unmounted_route.

@mmrath
Copy link

mmrath commented Aug 1, 2017

I think this is not a great idea. In general you would need a little more than CRUD ops for every CRUD style app. The idea described in this issue cannot handle all of that, if you want to handle all of that it might become another diesel project.

@Meralis40
Copy link

@mmrath Do you have an example?
Also, I think the idea must help for most common, not all (like you say, it's just big work).

@SergioBenitez
Copy link
Member Author

This issue has been open for quite some time without any particular progress, and Rocket has evolved significantly since then. I don't believe the concept of resources as suggested in the original issue text makes sense for today's Rocket, so I'm closing this issue.

I have some new ideas on what would conceptually be "resources" that I'd like to formalize soon, however.

@SergioBenitez SergioBenitez closed this as not planned Won't fix, can't repro, duplicate, stale Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A minor feature request feedback wanted User feedback is needed
Projects
None yet
Development

No branches or pull requests

8 participants