Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

app.ServeFiles should always be accessed last, regardless of when/where it's defined #1206

Open
Tracked by #2332
byungjikroh opened this issue Aug 2, 2018 · 3 comments
Labels
help wanted Feel free to contribute! proposal A suggestion for a change, feature, enhancement, etc
Milestone

Comments

@byungjikroh
Copy link

Steps to Reproduce the Problem

I added nested routes without groups' names like categories and products.

actions/app.go

...
cr := &CategoriesResource{}
...
pr := &ProductsResource{}
a.GET("/{category_name}", pr.List)
a.GET("/{category_name}/{product_name}", pr.Show)
...
app.ServeFiles("/", assetsBox)

actions/render.go

var assetsBox = packr.NewBox("../public")

Then, I wanted to serve 'robots.txt' files in public directory but server even couldn't serve assets files in 'public/assets' directory because it tried to fetch a category record named 'robots.txt' or 'assets' which are placed at 'category name' of routes.

So I reverted 'app.go' and 'render.go' like under Buffalo v0.12.4 (which supports public folder's files like robots.txt) and render with plain on products' List action.

actions/app.go

app.ServeFiles("/assets", assetsBox)
...
cr := &CategoriesResource{}
...
pr := &ProductsResource{}
a.GET("/{category_name}", pr.List)
a.GET("/{category_name}/{product_name}", pr.Show)
...

actions/render.go

var assetsBox = packr.NewBox("../public/assets")

actions/products.go

func (v ProductsResource) List(c buffalo.Context) error {
  if c.Param("category_name") == "robots.txt" {
    return c.Render(200, r.Plain("robots.txt"))
  }
  ...
}

Expected Behavior

Expect the server to serve files on public directory like robots.txt or ads.txt properly even without groups on routes.

So I want a feature to pre-define public directory files to serve files before matching routes params.

Elixir web framework Phoenix did it like below.

# Serve at "/" the static files from "priv/static" directory.
#
# You should set gzip to true if you are running phoenix.digest
# when deploying your static files in production.
plug Plug.Static,
  at: "/", from: :example, gzip: false,
  only: ~w(css fonts images js icons favicon.ico robots.txt ads.txt sitemaps)

Actual Behavior

The server can't distinguish static files on public directory and routes params.

Info

### Buffalo Version v0.12.4

App Information

Pwd=C:\Users\byungjik\go\src\example
Root=C:\Users\byungjik\go\src\example
GoPath=C:\Users\byungjik\go
Name=example
Bin=bin\example.exe
PackagePkg=example
ActionsPkg=example/actions
ModelsPkg=example/models
GriftsPkg=example/grifts
VCS=git
WithPop=true
WithSQLite=false
WithDep=false
WithWebpack=true
WithYarn=true
WithDocker=true
WithGrifts=true

Go Version

go version go1.9.1 windows/amd64

Go Env

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\byungjik\go
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config

Node Version

v8.11.3

NPM Version

5.6.0

Yarn Version

1.9.2

PostgreSQL Version

PostgreSQL Not Found (But installed 9.6)

MySQL Version

MySQL Not Found

SQLite Version

SQLite Not Found

Dep Version

could not find a Gopkg.toml file

Dep Status

could not find a Gopkg.toml file

@markbates markbates added help wanted Feel free to contribute! proposal A suggestion for a change, feature, enhancement, etc labels Aug 3, 2018
@markbates markbates changed the title Feature to pre-define public directory files before matching routes app.ServeFiles should always be accessed last, regardless of when/where it's defined Aug 3, 2018
@markbates markbates added enhancement New feature or request and removed proposal A suggestion for a change, feature, enhancement, etc labels Aug 3, 2018
@kteb
Copy link
Member

kteb commented Aug 5, 2018

@markbates I'm not sure you got the point that was a problem for @byungjikroh.

The first time I read it, I thought the same way that you did, but the second time I understood it. He wanted, if you put the serveFiles at the end, he will not be able to access any routes because they'll be caught by the route above.

...
cr := &CategoriesResource{}
...
pr := &ProductsResource{}
a.GET("/{category_name}", pr.List)
a.GET("/{category_name}/{product_name}", pr.Show)
...

I don't see a clear path to this way, maybe an approach could be to create something similar to app.ServeFiles("/", assetsBox) but more app.ServeInsideFiles("/public",assetsBox) and the public folder will not be a root anymore but only what's inside the folder will be routed.

Otherwise Mark I love the idea to have the ServeFiles always at the end. It'll remove some bugs for a lot of people.

@byungjikroh
Copy link
Author

byungjikroh commented Aug 5, 2018

Sorry for my poor explanation for the problem.

@kteb is close to my problem. Yes, if I put 'ServeFiles' as root path at the end, I can't access any public or assets files.

Because, for example, if public file path is '/robots.txt', the file route is matched to a param {category_name} and if assets file path is 'assets/application.css', route is matched to params {category_name} and {product_name} respectively.

So both routes are returned an error (sql: no rows in result) on the database because neither category or product has those names.

If I put 'ServeFiles' as root path at the start, I can't access any pages routes below it. Because all routes are matched to 'assetsBox'.

Therefore I suggest a feature to pre-define assets folder or public files name before matching pages routes. For example, if I set 'assets' (folder), 'robots.txt' (file) as non-matching routes on top, pages routes are not tried to be matched (I meant 'exclude') with paths started with them. (Phoenix web framework did like this.)

Or as @kteb said, add app.ServeInsideFiles("/public", assetsBox) additionally with app.ServeFiles("/", assetsBox).

Tell me if you need more explanation. :)

@sio4 sio4 added the s: triage Some tests need to be run to confirm the issue label Sep 26, 2022
@sio4 sio4 added this to the v1.1.0 milestone Sep 27, 2022
@sio4 sio4 mentioned this issue Sep 27, 2022
11 tasks
@sio4
Copy link
Member

sio4 commented Sep 29, 2022

My understanding of this issue:

First, This issue only happens in a very specific condition:

  1. the condition of this issue: use resource path such as /joe/front.right instead of /dogs/joe/legs/front.right.
  2. in this case, /assets/manifest.json could be interpreted as a static file /public/assets/manifest.json when if app.ServeFiles() was registered first
  3. but if app.GET("/{dog_name}/{leg_name}", dr.Show())) is the first, it will be interpreted as a resource call.

Indeed, it could happen. However, I don't think such as /joe/front.right style of path scheme is a good practice. It is ambiguous, and also it cannot be used along with /dog_name/eye_name or /cat_name/leg_name in the same application. I am not saying the path scheme is totally bad, but well, considering that edge case could be a low priority.

I will put this issue in the Proposal milestone, and it could be revisited when it has any extended idea, PR, or me-toos.

@sio4 sio4 modified the milestones: v1.1.0, Proposal Sep 29, 2022
@sio4 sio4 added proposal A suggestion for a change, feature, enhancement, etc and removed enhancement New feature or request s: triage Some tests need to be run to confirm the issue labels Sep 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Feel free to contribute! proposal A suggestion for a change, feature, enhancement, etc
Projects
None yet
Development

No branches or pull requests

4 participants