spandex-project / spandex_phoenix

Phoenix Instrumentation tracer
MIT License
82 stars 29 forks source link

Better calculate the route for the resource name #30

Closed aselder closed 4 years ago

aselder commented 4 years ago

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[bot] commented 4 years ago

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.

zachdaniel commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Thank you for your contribution!

aselder commented 4 years ago

After taking to Jose, I think I’ll have a better solution shortly.

Sent from my iPhone

On Jul 11, 2020, at 9:29 PM, Zach Daniel notifications@github.com wrote:

 Thank you for your contribution!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.