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

Recognize_path should account for HTTP method #2415

Open
ideaoforder opened this issue Feb 23, 2024 · 4 comments
Open

Recognize_path should account for HTTP method #2415

ideaoforder opened this issue Feb 23, 2024 · 4 comments

Comments

@ideaoforder
Copy link

In a RESTful API, you'll use the same path multiple times, but with a different REST verb.

These will return the same endpoint:

  • GET /orders
  • POST /orders

So will these:

  • GET /orders/123
  • PUT /orders/123
  • DELETE /orders/123

Ideally, we'd be able to pass that in as an option:

API.recognize_path('/orders/123', method: 'PUT')
@dblock
Copy link
Member

dblock commented Feb 24, 2024

Indeed. I am actually kinda surprised it doesn't. Start by writing a spec that (fails to) differentiate(s) multiple methods on the same API. What do we return today? The first matching route? In which case I think we should consider making this recognize_path(path, verb = 'GET') vs. adding an options hash. What other options would make sense?

jcagarcia added a commit to jcagarcia/grape that referenced this issue Oct 4, 2024
@jcagarcia
Copy link
Contributor

Hey 👋

I was taking a look to this issue. As @ideaoforder described, when using the recognize_path method, we are not able to specify the method. So in case you have more than one method for the same path, you will get always the one defined in the last position.

Check with this spec that when obtaining the routes for the /books path, we only get one of them.

jcagarcia@919f5f3

We need to define which behaviour we want for the recognize_path method (in case we want to change it):

Option a

  1. Add an optional parameter verb/method, to the recognize_path method.
  2. If not received, it will continue behaving as it is, just returning the last endpoint defined.
  3. If received, return the endpoint that matches the path and the provided verb.

Internally, when verb/method parameter is provided, it will continue delegating into the greedy_match?(input) method for finding a match with the provided input. On the other hand, if verb/method parameter is provided, it will delegate into the match?(input, method) method, that takes into account the method and will select the correct one depending on the provided method.

In this option, the routes will always return ONLY 0 or 1 routes inside the routes array.

Option b

  1. Add an optional parameter verb/method, to the recognize_path method.
  2. If not received, return all the possible routes and return them inside the routes array.
  3. If received, return the endpoint that matches the path and the provided verb.

In this option, the routes will always return 0, 1 or N routes inside the routes array.

This is the option that is added to jcagarcia@919f5f3 as a failed spec.

For this Option b is pending to check how to include all the routes inside the array could affect the detection of the routes in Grape.

@jcagarcia
Copy link
Contributor

What do you think @dblock ?

@dblock
Copy link
Member

dblock commented Oct 8, 2024

Option b) is a breaking change whereas option a) is not but doesn't behave the way we want.

I think a cleaner option would be c) add a new method called, for example, routes(path, verb = nil) that behaves like b), and deprecate recognize_path. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants