-
Notifications
You must be signed in to change notification settings - Fork 412
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
add static fileserver #2450
add static fileserver #2450
Conversation
3ad1195
to
1625894
Compare
1625894
to
a5846e6
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2450 +/- ##
==========================================
- Coverage 64.75% 64.58% -0.17%
==========================================
Files 135 135
Lines 7135 7153 +18
Branches 1200 1207 +7
==========================================
Hits 4620 4620
- Misses 2515 2533 +18
☔ View full report in Codecov by Sentry. |
} | ||
} | ||
|
||
def middleware[R, E](path: Path, staticServe: StaticServe[R, E, Any, Response]): Middleware[R] = |
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.
This should be toMiddleware
.
Also, I think there is value in only having a public Middleware.staticServer
interface, and hiding all these types. Thoughts?
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.
Yes, I think it's a good idea to stay consistent with Middleware
in general.
If I see that correctly we don´t need anything composable for the user when configuring a static server. It could be enough to only support the two main use-cases:
-
publish a given directory at a specified path:
Middleware.staticServer.fromDirectory(atPath:Path, docRoot: File):Middleware
-
publish resources at a specified path:
Middleware.staticServer.fromResources(atPath:Path):Middleware
Is that what you mean? If yes, I think we should do it that way.
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.
One drawback would be that we could not add caching of static assets transparently, no (assuming we want to do that)? With StaticServe
we could add a new operator cached:StaticServe
that can cache results. Maybe it's best to go with 1. and 2. from above and wait for feedback...
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 would go for:
object Middleware {
...
def serveDirectory(path: Path, docRoot: File, cached: Boolean = false): Middleware
}
Then it has minimal impact on the surface area, and all of these helper classes / functions can be made private inside the companion object of Middleware
.
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 discarded adding an option for cache
as that should be more carefully thought through - if I find time to do that we can still add that option without any problems. Besides that I changed the API according to your suggestion!
One more suggestion as a followup PR: add this as either an HttpApp or as a Route (e.g. |
As it is, `Middleware.serveResources(path)` going to serve any file at all under src/main/resources, including e.g. somebody's reference.conf file. I added a resourcePrefix parameter for this, so that the resources served can be limited to, e.g., src/main/resources/public. I also added delegates for serveResources and serveDirectory in Routes, as per this suggestion: zio#2450 (comment)
As it is, `Middleware.serveResources(path)` going to serve any file at all under src/main/resources, including e.g. somebody's reference.conf file. I added a resourcePrefix parameter for this, so that the resources served can be limited to, e.g., src/main/resources/public. I also added delegates for serveResources and serveDirectory in Routes, as per this suggestion: #2450 (comment)
I have a simple static fileserver as middleware, which provides an API like that:
I also added checks to anticipate (and prevent) traversal attacks (same as in akka http):