-
Notifications
You must be signed in to change notification settings - Fork 117
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
fix(path-order): don't trash previous parameter names #504
Conversation
d25b32c
to
0f3402d
Compare
@@ -568,7 +568,7 @@ macro_rules! impl_path_tuple ({ $($ty:ident),+ } => { | |||
description: v.description, | |||
collection_format: None, // this defaults to csv | |||
items: v.items.as_deref().map(map_schema_to_items), | |||
name: k, | |||
name: String::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the code very well at all. Does it matter at all that k
is used for determining whether the parameter is "required", but then discarding it when determining the parameter "name"? Is it possible we end up marking the wrong parameters as "required"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not here, this is the impl for path tuples: Path<($($ty,)+)
So here the names are just indexes, "0", "1"....
But even with individual paths, so two Path<>, Path<>, the names are not matched with the url names, example:
/{continent}/{country}
fn f(country: Path, continent: Path)...
Here as you see, country and continent are swapped, but no error is thrown, in fact the arg names in the function seem to be completely ignored..
Fixing this will require a lot more changes probably.
Also means for proper validation we'd have to disallow using tuples like so:
/{continent}/{country}
fn f(p: Path<String, String>)...
@@ -140,6 +140,220 @@ impl Responder for Pet { | |||
} | |||
} | |||
|
|||
#[test] | |||
fn test_path_order() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this test! Does it fail without the proposed changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it does.
Though after writing it I found out we also have another one: test_params, which was not failing when it should have! but it's also a more complicated test.
There's a routine which patches parameter names and it trashes previous attempts when it runs twice. I'm not sure this is meant to work but it seems that the only time we need this function is when we don't have param names, example: _p: web::Path<(bool, u32, String)> We seem to completely ignore param names and just name things from the path, example: get(ace: web::Path<bool>, user_id: web::Path<u32>, friend: web::Path<String>) This can actually be written as get(user_id: web::Path<bool>, ace: web::Path<u32>, friend: web::Path<String>) Which actually swaps parameters in the generate json!! Signed-off-by: Tiago Castro <[email protected]>
0f3402d
to
7f8e54c
Compare
There's a routine which patches parameter names and it trashes previous attempts when it runs twice.
I'm not sure this is meant to work but it seems that the only time we need this function is when we don't have param names, example:
_p: web::Path<(bool, u32, String)>
We seem to completely ignore param names and just name things from the path, example:
get(ace: web::Path, user_id: web::Path, friend: web::Path) This can actually be written as
get(user_id: web::Path, ace: web::Path, friend: web::Path)
Which actually swaps parameters in the generate json!!
Resolves: #501