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

openapi: method with same URL and different path parameters name result in 405 #948

Open
robko23 opened this issue Jan 20, 2025 · 2 comments · May be fixed by #952
Open

openapi: method with same URL and different path parameters name result in 405 #948

robko23 opened this issue Jan 20, 2025 · 2 comments · May be fixed by #952
Labels
bug Something isn't working

Comments

@robko23
Copy link
Contributor

robko23 commented Jan 20, 2025

Expected Behavior

Both endpoints should return 200 OK

Actual Behavior

When having two endpoints on the same URL with different path parameters names result in one of them returning 200 OK and the other one 405 Method Not Allowed. It seems that the order of the functions does not matter, the GET method always gets the 405.
This issue is related to #429 but since that issue is closed and I cannot reopen it I'am opening a new one.

Steps to Reproduce the Problem

I have created a minimal reproduction of this issue repository: https://github.com/robko23/poem-openapi-routing-issue/tree/master

Specifications

@robko23 robko23 added the bug Something isn't working label Jan 20, 2025
@robko23
Copy link
Contributor Author

robko23 commented Jan 20, 2025

So as I was investigating further, I found out that the issue #429 was fixed by 78fb112. (change route_table from HashMap<String, HashMap<Method, ...>> to HashMap<Method, HashMap<String,...>>)
Then there was another issue ( #489 ) which was resolved by 9876172. (change route_table back to HashMap<String, HashMap<Method, ...>>
However these issues seem to be mutually exclusive and the latter commit reverted the fix by the former commit.
Kinda tricky situation. Any ideas how to solve this?

@robko23
Copy link
Contributor Author

robko23 commented Jan 21, 2025

So I have an idea that this could be resolved by renaming the path to normalize param names.
eg. /hello/:param -> /hello/:param0, /hello/:another_param -> /hello/:param0.

This would create same entry in the routing table and it could resolve the issue. It could be "injecting" #[oai(name = "param{i}")] in the function signature.

I can look into this on the weekend and send PR to resolve this, but a small guidance would be appreciated.

I tried inserting format!("param{i}") here, but that did not work.

oai_path.push_str("/{");
oai_path.push_str(var);
oai_path.push('}');
new_path.push_str("/:");
new_path.push_str(var);

Then I tried changing this

let pname = format_ident!("p{}", i);
let param_name = operation_param
.name
.clone()
.unwrap_or_else(|| arg_ident.unraw().to_string());

like this

let param_name = format!("param{i}");

but that didn't work either.
However I think that I'm on the right track.
I just need to rename the entry in the routing table and then change this

let mut param_opts = #crate_name::ExtractParamOptions {
name: #param_name,
default_value: #default_value,
example_value: #example_value,
explode: #explode,

to extract from the param{i} while keeping the openapi param name in meta and the variable name in method signature

@robko23 robko23 changed the title openapi: method with same ULR and different path parameters name result in 405 openapi: method with same URL and different path parameters name result in 405 Jan 21, 2025
robko23 added a commit to robko23/poem that referenced this issue Jan 26, 2025
This commits add automatic renaming of path parameters in route table
and param extraction.
When having two same routes with different method and different param
names, it resulted in one of them returning 405 instead of firing the
handler.

This fix is somewhat janky, because it checks if the extracted param
type is `Path` and then it renames is to `param{i}`. There could be
more edge cases down the line, but for now it passess all the tests.

Fixes: poem-web#948
Related: poem-web#429
Signed-off-by: Robin Ilyk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant