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

🐛 [Bug]: Naming of routes works wrong after mount #2688

Closed
3 tasks done
Antonov-guap opened this issue Oct 23, 2023 · 18 comments · Fixed by #2689
Closed
3 tasks done

🐛 [Bug]: Naming of routes works wrong after mount #2688

Antonov-guap opened this issue Oct 23, 2023 · 18 comments · Fixed by #2689

Comments

@Antonov-guap
Copy link

Antonov-guap commented Oct 23, 2023

Bug Description

Naming of routes after mounting sub-apps into root app works wrong.
All routes turn into flat map of routes, but expected that they all separately work without any changing in original sub-apps.
Also there is no way to set names of sub-apps (but it could be great, as groups have this feature).

How to Reproduce

Steps to reproduce the behavior:

  1. Create few sub-apps
  2. Setup some routes with names
  3. Add sub-apps to root app
  4. Try to use names in handlers or in main.go to find any Routes

Expected Behavior

Expected, that I can use naming in some sub-apps and it works. And after that if I mount sub-app to root-app, naming also works, but with correct prefixes (of paths and of names, like it works with groups).

I added some test, that could be placed into the end of app_test.go. Check if the proposal behaviour is correct.

Fiber Version

v2.50.0

Code Snippet (optional)

func Test_Mount_Route_Names(t *testing.T) {
	// create sub-app with 2 handlers:
	subApp1 := New()
	subApp1.Name("app1.") // add some name to the app
	subApp1.Get("/users", func(c *Ctx) error {
		url, err := c.GetRouteURL("add-user", Map{})
		utils.AssertEqual(t, nil, err)
		utils.AssertEqual(t, "/app1/users", url, "handler: app1.add-user") // the prefix is /app1 because of the mount
		// if subApp1 is not mounted, expected url just /users
		return nil
	}).Name("get-users")
	subApp1.Post("/users", func(c *Ctx) error {
		route := c.App().GetRoute("get-users")
		utils.AssertEqual(t, MethodGet, route.Method, "handler: app1.get-users method")
		utils.AssertEqual(t, "/app1/users", route.Path, "handler: app1.get-users path")
		return nil
	}).Name("add-user")

	// create sub-app with 2 handlers inside a group:
	subApp2 := New()
	subApp2.Name("app2.") // add some name to the app
	app2Grp := subApp2.Group("/users").Name("users.")
	app2Grp.Get("", nil).Name("get")
	app2Grp.Post("", nil).Name("add")

	// put both sub-apps into root app
	rootApp := New()
	rootApp.Mount("/app1", subApp1)
	rootApp.Mount("/app2", subApp2)
	rootApp.startupProcess()

	// take route directly from sub-app
	route := subApp1.GetRoute("get-users")
	utils.AssertEqual(t, MethodGet, route.Method)
	utils.AssertEqual(t, "/users", route.Path)
	route = subApp1.GetRoute("add-user")
	utils.AssertEqual(t, MethodPost, route.Method)
	utils.AssertEqual(t, "/users", route.Path)

	// take route directly from sub-app with group
	route = subApp2.GetRoute("users.get")
	utils.AssertEqual(t, MethodGet, route.Method)
	utils.AssertEqual(t, "/users", route.Path)
	route = subApp2.GetRoute("users.add")
	utils.AssertEqual(t, MethodPost, route.Method)
	utils.AssertEqual(t, "/users", route.Path)

	// take route from root app (using names of sub-apps)
	route = rootApp.GetRoute("app1.add-user")
	utils.AssertEqual(t, MethodPost, route.Method)
	utils.AssertEqual(t, "/app1/users", route.Path)
	route = rootApp.GetRoute("app2.users.add")
	utils.AssertEqual(t, MethodPost, route.Method)
	utils.AssertEqual(t, "/app2/users", route.Path)

	// GetRouteURL inside handler
	req := httptest.NewRequest(MethodGet, "/app1/users", nil)
	_, _ = rootApp.Test(req)

	// ctx.App().GetRoute() inside handler
	req = httptest.NewRequest(MethodPost, "/app1/users", nil)
	_, _ = rootApp.Test(req)
}

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my problem prior to opening this one.
  • I understand that improperly formatted bug reports may be closed without explanation.
@ReneWerner87
Copy link
Member

related to #2685 #2686
@Antonov-guap have you tried the fix yet?
#2686 (comment)

does it work after that?

@Antonov-guap
Copy link
Author

related to #2685 #2686 @Antonov-guap have you tried the fix yet? #2686 (comment)

does it work after that?

Yes, I tried. The fix of Method+Path works well with flat application (single). But with multi-aps (with mount) is much more difficult to fix. I checked on master the testcase. It fails.

@Antonov-guap
Copy link
Author

be careful, I fixed some problems in testcase, use fresh one

@ReneWerner87
Copy link
Member

ReneWerner87 commented Oct 23, 2023

@efectn can you have a look at this

@Antonov-guap
Copy link
Author

@ReneWerner87 please, read the test-case. Check, if I expect behaviour well. If some mistakes or questions, let me know.

@ReneWerner87
Copy link
Member

okay now i understand

naming apps is currently not yet possible

only the last route, because an app is not a route this does not work either

furthermore, currently, in order to merge the stacks of the apps, the startup process must be executed, otherwise the routes of the subapps cannot be found in the main app

rootApp.startupProcess()

or

rootApp.mountStartupProcess()

but this does not happen

what do you think? is it a bug or feature? should we add this? @gaby @efectn

@Antonov-guap
Copy link
Author

Antonov-guap commented Oct 23, 2023

Adding names to Apps looks like a new feature. Actually, I don't really need it. But all other is a problem of how mounting works. It look more like a bug. I tried also to add rootApp.startupProcess() after mount. It is not solving the problem. Still not getting routes by name from handler or from rootApp.
I added to test rootApp.startupProcess() and fixed utils.AssertEqual (i used actual/expected vice versa).

@ReneWerner87
Copy link
Member

image
correct, because the copy method is used when merging (the subapps routes) and the other properties/infos are missing there

@Antonov-guap
Copy link
Author

image correct, because the copy method is used when merging (the subapps routes) and the other properties/infos are missing there

yes, there is no Naming info in the copy :(

@ReneWerner87
Copy link
Member

will add it, sec

@Antonov-guap
Copy link
Author

The easy way: to add naming info when merging. But the problem is: when you add 2 sub-apps having the same names, they will conflict on names (if you have the same names in the tree). And when you make redirect inside handler with ctx.RedirectToLocation, it could take wrong route to make path. That is why it is not that easy to fix. Mounting is a great feature, but using it without handy Naming of routes is very difficult.

@ReneWerner87
Copy link
Member

they will conflict on names (if you have the same names in the tree)

this can also happen in the same app

@Antonov-guap
Copy link
Author

n the same app

When you work on the same app, expected that you control unique route names and any conflicts are on your focus.
But when you mount some app into your app, it is better when you don't check all route names, but just trust the framework to mount without bugs. I agree that now it is not a bug, but a feature :)

@ReneWerner87
Copy link
Member

after the merge of the pull request, i will close the bug, you can then create a feature request for our backlog

feature is already useful

@ReneWerner87 ReneWerner87 added this to the v2 Next Release milestone Oct 23, 2023
@Antonov-guap
Copy link
Author

@ReneWerner87 thank you very much! Very fast solving of problems. Fixing of both problems will make working with Fiber much more convenient.

@Antonov-guap
Copy link
Author

Now I just have to wait until tag-release ;) When it is usually expected? Few days or weeks?

1 similar comment
@Antonov-guap
Copy link
Author

Now I just have to wait until tag-release ;) When it is usually expected? Few days or weeks?

@ReneWerner87
Copy link
Member

some days

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

Successfully merging a pull request may close this issue.

2 participants