rwf2 / Rocket

A web framework for Rust.
https://rocket.rs
Other
24.23k stars 1.56k forks source link

Request params fetch by name #2854

Closed Andynopol closed 2 weeks ago

Andynopol commented 3 weeks ago

What's missing?

Hi guys! I am a new Rocket enthusiast and I just migrated some services in my project in this wonderful framework, but I've noticed a somewhat bad pattern regarding the uri params.

  1. There is no way to just NOT include a parameter in a request handler unless I am marking it as ignored(<_>) which basically means that at first glance, I have no idea what the endpoint expects. Sure, I can document the function or I can just make it so: <_user>, but Rocket is such a well structured framework, it feels lacklustre.
  2. Point No. 1 also hinders some use cases with request guards. For example, If I have in a project multiple entities with links between them, I might want to make sure, before I reach the request handler, that both of them exist, and to pass those entities to the handler. Right now, the request.param() expects a usize as a parameter. Unfortunately, if you have multiple endpoints, you need to construct them in the same manner, putting the params in the same position so that you can reuse the guard.

Ideal Solution

I have two suggestions for this:

In order to have a clear view of the endpoint and have a naming convention for uri params, I suggest a flag that marks an endpoint as ignored for the handler while still keeping a name:

#[get("/path-to-endpoint/<param_name1>/<?param_name2>")]
or
#[get("/path-to-endpoint/<param_name1>/<!param_name2>")]
fn foo<'r>(param_name1: &'r str) -> &'r str {
  param_name1
}
// Something like that

In this example, param_name1 is required in the request handler, while param_name2 is not.

Also, the req.param should be able to accept a &str as param so that you can grab the desired parameter according to the name or the parameter, not it's position inside the path.

Why can't this be implemented outside of Rocket?

This issue targets the parsing of the uri.

Are there workarounds usable today?

No workarounds that I know of.

Alternative Solutions

Maybe take a page from Actix Web Extractors:

https://actix.rs/docs/extractors/

System Checks

SergioBenitez commented 3 weeks ago

Both of these are very deliberate design decisions.

There is no way to just NOT include a parameter in a request handler unless I am marking it as ignored(<_>) which basically means that at first glance, I have no idea what the endpoint expects. Sure, I can document the function or I can just make it so: <_user>, but Rocket is such a well structured framework, it feels lacklustre.

There's some conflict here. You simultaneously want to know "what the endpoint expects" (by naming a parameter) and but don't want to specify a type for said parameter. Rocket cares much more about the latter as a type directly determines which values are valid. On the other hand, a name is just a hint. The only way to resolve this conflict is to either be completely ambiguous (_)` or completely specific (name + type), which is what Rocket does today.

Still, I'm happy to be convinced that there's some curious space I'm missing. Do you have any compelling examples that illustrate the existence of such a space?

Point No. 1 also hinders some use cases with request guards. For example, If I have in a project multiple entities with links between them, I might want to make sure, before I reach the request handler, that both of them exist, and to pass those entities to the handler. Right now, the request.param() expects a usize as a parameter. Unfortunately, if you have multiple endpoints, you need to construct them in the same manner, putting the params in the same position so that you can reuse the guard.

I'm not sure I follow the example you're promoting. Can you perhaps concretize it with some code?

In general, we don't want you to be using request.param() at all. This is even in the docs: "This method exists only to be used by manual routing. To retrieve parameters from a request, use Rocket’s code generation facilities."

Andynopol commented 3 weeks ago

Ok. Maybe I am missing something that Rocket provides and I haven't seen an example of, nor the docs(from what I've read are very clear about this).

The concept is not new. It is present in NestJs, It is called Interceptor. Rocket is basically doing the same thing with guards.

Let's say, you want to link a post to a comment.


#[get("/link-post/post/<postid>/comment/<commentid>")]
fn handler(post: PostEntity, comment: CommentEntity) -> Json<Value>

In stead of using the guards mechanic, you are passing the entity/models of the posts and comment in the handler, having a clean separation of concern.

You are basically validating the request(kind of what a guard/interceptor normally does).

The naming convention for the ignored params is for the guards mostly. So that a dev, can understand that the postid is used in the PostEntity guard and the commentid is used in the CommentEntity guard.

To be noted: I know that Nestjs and Rust(Rocket) are two different languages and frameworks. But just based on what I've seen Rocket can spin, I don't see why it can't do this.

I am ready to be completely wrong :sweat_smile:

Andynopol commented 3 weeks ago

Here is the code example you requested. Extracted from a personal project:

For this endpoint that grabs all gateways based on a user access:

#[post("/<user>/gateway/<_>", format="json", data="<registry_query>")]
async fn get_node_value_series(
  user: &str,
  registry_col: &State<Arc<Collection<RegistryModel>>>,
  map_col: &State<Arc<Collection<PayloadMappingModel>>>, 
  registry_query: Json<RegistryQueryDto>, 
  _auth: Auth,
  gateway: GatewayModel) -> Custom<Json<Value>>{
  let user_oid = ObjectId::parse_str(user);
  if let Err(_) = user_oid {
    return Custom(
      Status::BadRequest,
      Json(json!({"status": "error", "message": "Invalid user provided"}))
    )
  }

  let series = get_series(None, registry_col, map_col, registry_query, gateway).await;

  return Custom(Status::Ok, Json(json!({"series": series})))
}

I am using the GatewayModel request guard.


#[rocket::async_trait]
impl<'r> FromRequest<'r> for GatewayModel {
  type Error = GatewayError;
  async fn from_request(req: &'r Request<'_>) -> Outcome<Self, Self::Error> {

    let user_id = match req.param::<&str>(0) {
      Some(Ok(value)) => match ObjectId::parse_str(value) {
        Ok(value) => value,
        Err(_) => return Outcome::Error((Status::BadRequest, GatewayError::InvalidId))
      },
      Some(Err(_)) => return Outcome::Error((Status::BadRequest, GatewayError::InvalidId)),
      None => return Outcome::Error((Status::BadRequest, GatewayError::InvalidId))
    };

    let gateway_id = match req.param::<&str>(2){
      Some(Ok(value)) => match ObjectId::parse_str(value) {
        Ok(value) => value,
        Err(_) => return Outcome::Error((Status::BadRequest, GatewayError::InvalidId))
      },
      Some(Err(_)) => return Outcome::Error((Status::BadRequest, GatewayError::InvalidId)),
      None => return Outcome::Error((Status::BadRequest, GatewayError::InvalidId))
    };

    let gateway_col = match req.rocket().state::<Arc<Collection<GatewayModel>>>(){
      Some(collection) => collection,
      None => return Outcome::Error((Status::InternalServerError, GatewayError::ServerErorr))
    };

    let gateway = match gateway_col.find_one(doc!{"_id": gateway_id, "access._id": user_id}).await {
      Ok(Some(value)) => value,
      Ok(None) => return Outcome::Error((Status::NotFound, GatewayError::Notfound)),
      Err(_) => return Outcome::Error((Status::InternalServerError, GatewayError::ConnectionError))
    };

    Outcome::Success(gateway)
  }

}

As you can see, I am using the req.params function to extract the gateway from my database.

But If I change the order of the endpoint or the overall structure, the guard is not longer valid. I need to change it too. That is not good design...

For example, let's say I have multiple endpoints that require such a check and I want to use THIS EXACT guard there. Now I need to make the endpoints in the exact same order like this one.

the10thWiz commented 3 weeks ago

I would suggest tackling this is a different way - by doing the actual selection inside the function. From that angle, the function would look something like this:

#[post("/<user>/gateway/<gateway>", format="json", data="<registry_query>")]
async fn get_node_value_series(
  user: ObjectId,
  gateway: ObjectId,
  registry_col: &State<Arc<Collection<RegistryModel>>>,
  map_col: &State<Arc<Collection<PayloadMappingModel>>>,
  gateway_col: &State<Arc<Collection<GatewayModel>>>,
  registry_query: Json<RegistryQueryDto>, 
  _auth: Auth,
)
  -> Result<Json<Value>, Custom<Json<Value>>>
{
  let gateway = gateway_col.find_one(doc!{"_id": gateway_id, "access._id": user_id})
    .await
    .map_err(|e| Custom(Status::InternalServerError, Json(json!{})))?
    .ok_or_else(|| Custom(Status::NotFound, Json(json!{})))?;
  let series = get_series(None, registry_col, map_col, registry_query, gateway).await;

  Ok(Json(json!({"series": series})))
}

To enable this, you would need a FromParam implementation for ObjectId, but that would be pretty simple:

impl FromParam<'_> for ObjectId {
  type Error = GatewayError;
  fn from_param(value: &str) -> Result<Self, Self::Error> {
    Self::parse_str(value).map_err(|_| GatewayError::InvalidId)
  }
}

This approach naturally allows the parameters to be in whatever location you want within the path. It also makes it obvious what you expect the user and gateway parameters to be - they should be ObjectIds (which identifies a specific format), that identify a specific user and gateway respectively.

Andynopol commented 2 weeks ago

Hi! Yes, this is a great option. It's more or less what I was talking about but in a different order.

I won't discard this idea tho. I understand that Nestjs and Rocket are 2 different frameworks in 2 different technologies, but the idea to reach the endpoint callback only after you validate your data is quite good. Just keep a mental note in the future for the development of the framework. Good luck! And thanks!