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

Better calculate the route for the resource name #30

Closed
wants to merge 2 commits into from

Conversation

aselder
Copy link

@aselder aselder commented Jul 9, 2020

Right now, the calculcation of the routes strips static
parts of the path when calculating the resource name

For instance if you have the following route:

scope "/carts" do
  forward "/cart", Routers.CartRouter
end

and in the CartRouter

scope "/", ApiV1Web do
  get "/:app_id/:identifier", CartController, :get
end

the request path would be something like /carts/cart/1234/abc. This
would get send with a resource name of GET /:app_id/:identifier, which
make is very hard to differentiate between routes in a different
router or scope.

This change preserves the static parts of the route so that a clearer
route name is obtained.

Right now, the calculcation of the routes strips static
parts of the path when calculating the resource name

For instance if you have the following route:

```
scope "/carts" do
  forward "/cart", Routers.CartRouter
end
```

and in the CartRouter
```
scope "/", ApiV1Web do
  get "/:app_id/:identifier", CartController, :get
end
```

the request path would be something like `/carts/cart/1234/abc`. This
would get send with a resource name of `GET /:app_id/:identifier`, which
make is very hard to differentiate between routes in a different
router or scope.

This change preserves the static parts of the route so that a clearer
route name is obtained.
@sourcelevel-bot
Copy link

Hello, @aselder! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

lib/spandex_phoenix.ex Outdated Show resolved Hide resolved
@zachdaniel
Copy link
Member

This is actually how this was initially implemented, but has problems. See the following example:

path_params = %{"id" => "1", "second_id" => "1", "third_id" => "1"}
path = "api/1/1/1"
Enum.reduce(path_params, path, fn {param, value}, acc -> String.replace(acc, value, ":#{param}") end)

You'd end up with something like:
"api/:id/:id/:id" as the result.

@zachdaniel
Copy link
Member

I agree that losing the forwarded prefix is a problem. Perhaps you don't have repeating path parameters/this isn't a problem for your application, but it wouldn't be an appropriate generic solution.

@zachdaniel zachdaniel closed this Jul 12, 2020
@zachdaniel
Copy link
Member

zachdaniel commented Jul 12, 2020

Thank you for your contribution!

@aselder
Copy link
Author

aselder commented Jul 12, 2020 via email

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

Successfully merging this pull request may close these issues.

2 participants