gobuffalo / buffalo

Rapid Web Development w/ Go
http://gobuffalo.io
MIT License
8.07k stars 573 forks source link

Fix URL prefix #2367

Closed qdongxu closed 1 year ago

qdongxu commented 1 year ago

This PR addresses Issue #2356.

What is being done in this PR?

Type in here a description of the changes in this PR. The web console cannot be opened after setting a URL prefix in actions/app.go :


appOnce.Do(func() {
app = buffalo.New(buffalo.Options{
Env:         ENV,
SessionName: "_prefix_test_session",
Prefix:      "/prefix/",
})
The error is : 

home/index.plush.html: line 5: "rootPath": unknown identifier



### What are the main choices made to get to this solution?
> Type in here a description of the choices made to get to this solution.

`routenamer.go` considers "/" as the root path only, ignoring a URL prefix as a root path.

### List the manual test cases you've covered before sending this PR:
1. Add a prefix as above then open `http://127.0.0.1:3000/prefix/` in the browser.
sio4 commented 1 year ago

This PR is not acceptable:

  1. This changes the behavior and will cause unwanted results if existing users use the current result of names. (e.g. prefixSomethingPath instead somethingPath for the prefixed /something) This is a breaking change.
  2. The fix just fixes the root path only. If the goal of this PR is to keep the route name the same regardless of the prefix, the fix should address the other paths too. (one possible way is trimming the prefix from the given path before the evaluation)

Basically, I agree that the path name should be preserved when we use the prefix to make the app can be ported (mounted) in a different prefix easily without code level change. However, we cannot change this behavior in v1. Also, I have the plan (#2240) to rewrite related things including App, Home, Group, and route information then this behavior could be redefined at that moment.

qdongxu commented 1 year ago

This PR is not acceptable:

  1. This changes the behavior and will cause unwanted results if existing users use the current result of names. (e.g. prefixSomethingPath instead somethingPath for the prefixed /something) This is a breaking change.
  2. The fix just fixes the root path only. If the goal of this PR is to keep the route name the same regardless of the prefix, the fix should address the other paths too. (one possible way is trimming the prefix from the given path before the evaluation)

Basically, I agree that the path name should be preserved when we use the prefix to make the app can be ported (mounted) in a different prefix easily without code level change. However, we cannot change this behavior in v1. Also, I have the plan (#2240) to rewrite related things including App, Home, Group, and route information then this behavior could be redefined at that moment.

Hi @sio4, trimming the prefix from the route name, resolved both concerns. This is a fully backward-compatible solution.

sio4 commented 1 year ago

This PR is not acceptable:

  1. This changes the behavior and will cause unwanted results if existing users use the current result of names. (e.g. prefixSomethingPath instead somethingPath for the prefixed /something) This is a breaking change.
  2. The fix just fixes the root path only. If the goal of this PR is to keep the route name the same regardless of the prefix, the fix should address the other paths too. (one possible way is trimming the prefix from the given path before the evaluation)

Basically, I agree that the path name should be preserved when we use the prefix to make the app can be ported (mounted) in a different prefix easily without code level change. However, we cannot change this behavior in v1. Also, I have the plan (#2240) to rewrite related things including App, Home, Group, and route information then this behavior could be redefined at that moment.

Hi @sio4, trimming the prefix from the route name, resolved both concerns. This is a fully backward-compatible solution.

While the current code generates a route name as prefixPath and prefixHomePath but the patched version will generate rootPath and homePath. What do you mean by fully backward-compatible?

qdongxu commented 1 year ago

While the current code generates a route name as prefixPath and prefixHomePath but the patched version will generate rootPath and homePath. What do you mean by fully backward-compatible?

No. the code has been updated as generating a route name as rootPath and homePath without the prefix. fully backward-compatible just means both rootPath and homePath keep unchanged despite a prefix is configured or not.

Input with a prefix:


app = buffalo.New(buffalo.Options{
    Env:         ENV,
    SessionName: "_prefix_test_session",
    Prefix:      "/prefix",
})

app.GET("/", HomeHandler)

app.GET("/about/", About)
app.GET("/prefix/about/", About)

h1 := app.VirtualHost("h1.com")
h1.GET("/func1", VirtualHost1About)

h2 := app.VirtualHost("h2.com")
h2.GET("/func1", VirtualHost2About)

g1 := app.Group("/group/")
g1.GET("func2", About)

subg1 := g1.Group("subg")
subg1.GET("func3", About)

app.Resource("/base-res", &buffalo.BaseResource{})

Route names are identical as no prefix configured:

METHOD PATH NAME HANDLER
GET /prefix/ rootPath prefix_test/actions.HomeHandler
GET /prefix/about/ aboutPath prefix_test/actions.About
GET /prefix/prefix/about/ prefixAboutPath prefix_test/actions.About
GET /prefix/func1/ func1Path prefix_test/actions.VirtualHost1About
GET /prefix/func1/ func1Path prefix_test/actions.VirtualHost2About
GET /prefix/group/func2/ groupFunc2Path prefix_test/actions.About
GET /prefix/group/subg/func3/ groupSubgFunc3Path prefix_test/actions.About
GET /prefix/base-res/ baseResPath github.com/gobuffalo/buffalo.BaseResource.List
POST /prefix/base-res/ baseResPath github.com/gobuffalo/buffalo.BaseResource.Create
GET /prefix/base-res/{base_id}/ baseReBaseIDPath github.com/gobuffalo/buffalo.BaseResource.Show
PUT /prefix/base-res/{base_id}/ baseReBaseIDPath github.com/gobuffalo/buffalo.BaseResource.Update
DELETE /prefix/base-res/{base_id}/ baseReBaseIDPath github.com/gobuffalo/buffalo.BaseResource.Destroy
sio4 commented 1 year ago

While the current code generates a route name as prefixPath and prefixHomePath but the patched version will generate rootPath and homePath. What do you mean by fully backward-compatible?

No. the code has been updated as generating a route name as rootPath and homePath without the prefix. fully backward-compatible just means both rootPath and homePath keep unchanged despite a prefix is configured or not.

No, what I mean is that the result should be preserved as the same as today (which is prefixPath instead of rootPath) for the current major version. If we release a new version with this change without bumping the major version, the existing application that already uses the current route name as prefixPath will be broken when they update the module. This is breaking change even though the current behavior is not good.

Also, your PR is targeted to the main branch but a PR against the branch is not acceptable for now. We are planning a new major version on the branch, but not today.

I would like to keep this PR as the status of blocked and will revisit it once we started working on the new major version soon.

sio4 commented 1 year ago

Also, in the next major version, the App/Home/Group structure could be completely changed as I already linked the open issue #2240. So I am not fully sure if we can merge this change directly or just need to fix the issue in a different way.

qdongxu commented 1 year ago

Currently rootPath(), and javascriptTag() in the render package do not work with Prefix. the web console cannot open at all when a prefix is configured. The only to configure a reverse proxy is by routing by port.

It will be great if this issue is scheduled in #2240. thanks for your patient clarification, @sio4

qdongxu commented 1 year ago

This PR closed as it will be addressed in #2240