-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
🩹 Fix: static server in sub app with mount (#3104) #3132
base: v2
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
fe948ca
to
506a800
Compare
subApp := New() | ||
subApp.Static("/css", dir) | ||
|
||
app.Mount("/sub", subApp) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@yinheli Does this issue affect |
733357b
to
9808465
Compare
router.go
Outdated
@@ -357,6 +358,12 @@ 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 { | |||
if bytes.Equal(path[:n], utils.UnsafeBytes(mountPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need two-level if here. You can write:
if n := len(mountPath); n > 0 && bytes.Equal(path[:n], utils.UnsafeBytes(mountPath)) {
...
}
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate
There was a problem hiding this comment.
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))
}
There was a problem hiding this comment.
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.
1ddc40f
to
258e56e
Compare
258e56e
to
d415399
Compare
Description
This commit fixes an issue with static file serving in sub-app when using the Mount method. The problem was that static routes in sub-apps were not being properly handled when the sub-app was mounted to a parent app.
Key changes:
PathRewrite
function inrouter.go
to handle mount paths correctly for static file serving.Test_Route_Static_SubApp
test inrouter_test.go
to include a nested sub-app scenario.Fixes #3104
Changes introduced
Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/
directory for Fiber's documentation.