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.

Sign up for GitHub

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

Jump to bottom

🩹 Fix: static server in sub app with mount (#3104) #3132

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from
Open

🩹 Fix: static server in sub app with mount (#3104) #3132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions mount.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ type mountFields struct {
subAppsProcessed sync.Once
// Prefix of app if it was mounted
mountPath string
// Parent app of the current app
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ReneWerner87 do you think we should use full-mount path for mounting? I think this might be a bug related to MountPath

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MountPath can only see that of subApp. Currently, the app cannot know how the upper layer is mounted. So I introduce a reference to Parent here. Do you have any good suggestions?

My current thinking is that when PathRewrite in registerStatic can replace the mounted part of the path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efectn @yinheli yes it should have something to do with the path
don't know if we necessarily need the complete parent reference for this

in the bug you said that the path is public/public, then we should start there

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efectn @yinheli yes it should have something to do with the path don't know if we necessarily need the complete parent reference for this

in the bug you said that the path is public/public, then we should start there

Instead, perhaps we can do it during the path setting https://github.com/gofiber/fiber/blob/main/mount.go#L52

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have investigated this. However, here only the path of the subapp can be obtained. And registerStatic initializes the fileHandler when registering. So I chose the current implementation and added FullMountPath. I think that is easy to implement and has a lower cost.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have investigated this. However, here only the path of the subapp can be obtained. And registerStatic initializes the fileHandler when registering. So I chose the current implementation and added FullMountPath. I think that is easy to implement and has a lower cost.

Ok i will check it tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the problem occur with v3 @yinheli ? I've just given a try with v3, and seems to work well:

func TestStaticMount(t *testing.T) {
	t.Parallel()
	app := fiber.New()
	sub := fiber.New()
	sub2 := fiber.New()

	sub2.Get("/doe*", New("../../.github"))

	sub.Use("/sub2", sub2)
	app.Use("/sub", sub)

	resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/sub/sub2/doe/release.yml", nil))
	require.NoError(t, err, "app.Test(req)")

	reqq, err := io.ReadAll(resp.Body)
	require.NoError(t, err)

	require.Equal(t, 200, resp.StatusCode, "Status code")
	require.Equal(t, uint32(1), app.HandlersCount())
	require.Equal(t, " ... ", string(reqq))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't investigated yet. I'm a bit busy during this period.

parentApp *App
}

// Create empty mountFields instance
Expand Down Expand Up @@ -50,6 +52,7 @@ func (app *App) Mount(prefix string, subApp *App) Router {

subApp.mountFields.mountPath = path
app.mountFields.appList[path] = subApp
subApp.mountFields.parentApp = app
}

// register mounted group
Expand Down Expand Up @@ -99,6 +102,14 @@ func (app *App) MountPath() string {
return app.mountFields.mountPath
}

// FullMountPath returns the full mount path of the app, including the parent app's mount path.
func (app *App) FullMountPath() string {
if app.mountFields.parentApp == nil {
return app.mountFields.mountPath
}
return getGroupPath(app.mountFields.parentApp.FullMountPath(), app.mountFields.mountPath)
}

// hasMountedApps Checks if there are any mounted apps in the current application.
func (app *App) hasMountedApps() bool {
return len(app.mountFields.appList) > 1
Expand Down
5 changes: 5 additions & 0 deletions router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package fiber

import (
"bytes"
"fmt"
"html"
"sort"
Expand Down Expand Up @@ -357,6 +358,10 @@ func (app *App) registerStatic(prefix, root string, config ...Static) {
IndexNames: []string{"index.html"},
PathRewrite: func(fctx *fasthttp.RequestCtx) []byte {
path := fctx.Path()
mountPath := app.FullMountPath()
if n := len(mountPath); n > 0 && bytes.Equal(path[:n], utils.UnsafeBytes(mountPath)) {
path = path[n:]
}
if len(path) >= prefixLen {
if isStar && app.getString(path[0:prefixLen]) == prefix {
path = append(path[0:0], '/')
Expand Down
49 changes: 48 additions & 1 deletion router_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// 📃 Github Repository: https://github.com/gofiber/fiber
// 📌 API Documentation: https://docs.gofiber.io

//nolint:bodyclose // Much easier to just ignore memory leaks in tests
//nolint:bodyclose,goconst // Much easier to just ignore memory leaks in tests and constant variables in tests
package fiber

import (
Expand Down Expand Up @@ -471,6 +471,53 @@ func Test_Route_Static_HasPrefix(t *testing.T) {
body, err = io.ReadAll(resp.Body)
utils.AssertEqual(t, nil, err, "app.Test(req)")
utils.AssertEqual(t, true, strings.Contains(app.getString(body), "color"))

app = New()
app.Static("/css", dir)

resp, err = app.Test(httptest.NewRequest(MethodGet, "/css/style.css", nil))
utils.AssertEqual(t, nil, err, "app.Test(req)")
utils.AssertEqual(t, 200, resp.StatusCode, "Status code")

body, err = io.ReadAll(resp.Body)
utils.AssertEqual(t, nil, err, "app.Test(req)")
utils.AssertEqual(t, true, strings.Contains(app.getString(body), "color"))
}

func Test_Route_Static_SubApp(t *testing.T) {
t.Parallel()

dir := "./.github/testdata/fs/css"
gaby marked this conversation as resolved.
Show resolved Hide resolved
app := New()

// subapp
subApp := New()
subApp.Static("/css", dir)
app.Mount("/sub", subApp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a testcase for nested mounting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested mounting is not working. I'll try to fix it.


// nested subapp
nestApp := New()
nestApp.Static("/css", dir)
subApp.Mount("/nest", nestApp)

// test subapp
resp, err := app.Test(httptest.NewRequest(MethodGet, "/sub/css/style.css", nil))
utils.AssertEqual(t, nil, err, "app.Test(req)")
utils.AssertEqual(t, 200, resp.StatusCode, "Status code")

body, err := io.ReadAll(resp.Body)
utils.AssertEqual(t, nil, err, "app.Test(req)")
utils.AssertEqual(t, true, strings.Contains(app.getString(body), "color"))

// test nested subapp

resp, err = app.Test(httptest.NewRequest(MethodGet, "/sub/nest/css/style.css", nil))
utils.AssertEqual(t, nil, err, "app.Test(req)")
utils.AssertEqual(t, 200, resp.StatusCode, "Status code")

body, err = io.ReadAll(resp.Body)
utils.AssertEqual(t, nil, err, "app.Test(req)")
utils.AssertEqual(t, true, strings.Contains(app.getString(body), "color"))
}

func Test_Router_NotFound(t *testing.T) {
Expand Down
Loading