-
Notifications
You must be signed in to change notification settings - Fork 39
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
refactor(files) move serving files into a plugin #130
Changes from all commits
c7eaef5
9ae81e3
74b8370
2e13a05
cf61e46
39bf1f7
2eab8f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,11 +10,11 @@ package.path = './src/?.lua;./src/?/init.lua;' .. package.path | |||||
local Pegasus = require 'pegasus' | ||||||
local Compress = require 'pegasus.plugins.compress' | ||||||
local Downloads = require 'pegasus.plugins.downloads' | ||||||
local Files = require 'pegasus.plugins.files' | ||||||
-- local TLS = require 'pegasus.plugins.tls' | ||||||
|
||||||
local server = Pegasus:new({ | ||||||
port = '9090', | ||||||
location = '/example/root/', | ||||||
plugins = { | ||||||
-- TLS:new { -- the tls specific configuration | ||||||
-- wrap = { | ||||||
|
@@ -30,19 +30,32 @@ local server = Pegasus:new({ | |||||
-- }, | ||||||
|
||||||
Downloads:new { | ||||||
prefix = "downloads", | ||||||
location = '/example/root/', | ||||||
prefix = 'downloads', | ||||||
stripPrefix = true, | ||||||
}, | ||||||
|
||||||
Files:new { | ||||||
location = '/example/root/', | ||||||
}, | ||||||
|
||||||
Compress:new(), | ||||||
} | ||||||
}) | ||||||
|
||||||
server:start(function(req) | ||||||
local data = req:post() | ||||||
server:start(function(req, resp) | ||||||
local stop = false | ||||||
|
||||||
local path = req:path() | ||||||
if req:method() ~= "POST" or path ~= "/index.html" then | ||||||
return stop | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
end | ||||||
|
||||||
local data = req:post() | ||||||
if data then | ||||||
print(data['name']) | ||||||
print(data['age']) | ||||||
print("Name: ", data.name) | ||||||
print("Age: ", data.age) | ||||||
end | ||||||
stop = not not resp:writeFile("./example/root" .. path) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return stop | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
end) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
describe("Files plugin", function() | ||
|
||
local Files = require "pegasus.plugins.files" | ||
|
||
|
||
|
||
describe("instantiation", function() | ||
|
||
local options = {} | ||
local plugin = Files:new(options) | ||
|
||
it("should return a table", function() | ||
assert.is.table(plugin) | ||
end) | ||
|
||
|
||
it("should have a default location; '.'", function() | ||
assert.is.equal(".", plugin.location) | ||
end) | ||
|
||
|
||
it("should have a default; '/index.html'", function() | ||
assert.is.equal("/index.html", plugin.default) | ||
end) | ||
|
||
end) | ||
|
||
|
||
|
||
describe("invocation", function() | ||
|
||
local redirect_called, writeFile_called | ||
local request = {} | ||
local response = { | ||
redirect = function(self, ...) redirect_called = {...} end, | ||
writeFile = function(self, ...) writeFile_called = {...} return self end, | ||
-- finish = function(self, ...) end, | ||
-- setHeader = function(self, ...) end, | ||
-- setStatusCode = function(self, ...) end, | ||
} | ||
|
||
before_each(function() | ||
redirect_called = nil | ||
writeFile_called = nil | ||
end) | ||
|
||
|
||
it("handles GET", function() | ||
stub(request, "path", function() return "/some/file.html" end) | ||
stub(request, "method", function() return "GET" end) | ||
local stop = Files:new():newRequestResponse(request, response) | ||
assert.is.True(stop) | ||
assert.is.Nil(redirect_called) | ||
assert.are.same({ | ||
"./some/file.html", | ||
"text/html" | ||
}, writeFile_called) | ||
end) | ||
|
||
it("handles HEAD", function() | ||
stub(request, "path", function() return "/some/file.html" end) | ||
stub(request, "method", function() return "HEAD" end) | ||
local stop = Files:new():newRequestResponse(request, response) | ||
assert.is.True(stop) | ||
assert.is.Nil(redirect_called) | ||
assert.are.same({ | ||
"./some/file.html", | ||
"text/html" | ||
}, writeFile_called) | ||
end) | ||
|
||
it("doesn't handle POST", function() | ||
stub(request, "path", function() return "/some/file.html" end) | ||
stub(request, "method", function() return "POST" end) | ||
local stop = Files:new():newRequestResponse(request, response) | ||
assert.is.False(stop) | ||
assert.is.Nil(redirect_called) | ||
assert.is.Nil(writeFile_called) | ||
end) | ||
|
||
it("doesn't handle PUT", function() | ||
stub(request, "path", function() return "/some/file.html" end) | ||
stub(request, "method", function() return "PUT" end) | ||
local stop = Files:new():newRequestResponse(request, response) | ||
assert.is.False(stop) | ||
assert.is.Nil(redirect_called) | ||
assert.is.Nil(writeFile_called) | ||
end) | ||
|
||
it("redirects GET /", function() | ||
stub(request, "path", function() return "/" end) | ||
stub(request, "method", function() return "GET" end) | ||
local stop = Files:new():newRequestResponse(request, response) | ||
assert.is.True(stop) | ||
assert.are.same({ | ||
"/index.html" | ||
}, redirect_called) | ||
assert.is.Nil(writeFile_called) | ||
end) | ||
|
||
it("serves from specified location", function() | ||
stub(request, "path", function() return "/some/file.html" end) | ||
stub(request, "method", function() return "GET" end) | ||
local stop = Files:new({ location = "./location" }):newRequestResponse(request, response) | ||
assert.is.True(stop) | ||
assert.is.Nil(redirect_called) | ||
assert.are.same({ | ||
"./location/some/file.html", | ||
"text/html" | ||
}, writeFile_called) | ||
end) | ||
|
||
it("forces location to be relative", function() | ||
stub(request, "path", function() return "/some/file.html" end) | ||
stub(request, "method", function() return "GET" end) | ||
local stop = Files:new({ location = "/location" }):newRequestResponse(request, response) | ||
assert.is.True(stop) | ||
assert.is.Nil(redirect_called) | ||
assert.are.same({ | ||
"./location/some/file.html", | ||
"text/html" | ||
}, writeFile_called) | ||
end) | ||
|
||
end) | ||
|
||
end) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,22 @@ | ||
local Request = require 'pegasus.request' | ||
local Response = require 'pegasus.response' | ||
local mimetypes = require 'mimetypes' | ||
local lfs = require 'lfs' | ||
local Files = require 'pegasus.plugins.files' | ||
|
||
local Handler = {} | ||
Handler.__index = Handler | ||
|
||
function Handler:new(callback, location, plugins) | ||
local handler = {} | ||
handler.callback = callback | ||
handler.location = location or '' | ||
handler.plugins = plugins or {} | ||
|
||
if location then | ||
handler.plugins[#handler.plugins+1] = Files:new { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unsure if I agree that a plugin should be called into the handler logic. In my view, the handler logic should work just with stuff that is part of the Pegasus core. I'm curious to see how you envision it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Tieske ? |
||
location = location, | ||
default = "/index.html", | ||
} | ||
end | ||
|
||
local result = setmetatable(handler, self) | ||
result:pluginsAlterRequestResponseMetatable() | ||
|
||
|
@@ -109,58 +114,32 @@ function Handler:processRequest(port, client, server) | |
return false | ||
end | ||
|
||
local request = Request:new(port, client, server) | ||
if not request:method() then | ||
local request = Request:new(port, client, server, self) | ||
local response = request.response | ||
|
||
local method = request:method() | ||
if not method then | ||
client:close() | ||
return | ||
end | ||
|
||
local response = Response:new(client, self) | ||
response.request = request | ||
local stop = self:pluginsNewRequestResponse(request, response) | ||
|
||
if stop then | ||
return | ||
end | ||
|
||
if request:path() and self.location ~= '' then | ||
local path = request:path() | ||
if path == '/' or path == '' then | ||
path = 'index.html' | ||
end | ||
local filename = '.' .. self.location .. path | ||
|
||
if not lfs.attributes(filename) then | ||
response:statusCode(404) | ||
end | ||
|
||
stop = self:pluginsProcessFile(request, response, filename) | ||
|
||
if stop then | ||
return | ||
end | ||
|
||
local file = io.open(filename, 'rb') | ||
|
||
if file then | ||
response:writeFile(file, mimetypes.guess(filename or '') or 'text/html') | ||
stop = true | ||
else | ||
response:statusCode(404) | ||
end | ||
end | ||
|
||
if self.callback and not stop then | ||
if self.callback then | ||
response:statusCode(200) | ||
response.headers = {} | ||
response:addHeader('Content-Type', 'text/html') | ||
|
||
self.callback(request, response) | ||
stop = self.callback(request, response) | ||
if stop then | ||
return | ||
end | ||
end | ||
|
||
if response.status == 404 then | ||
response:writeDefaultErrorMessage(404) | ||
end | ||
response:writeDefaultErrorMessage(404) | ||
end | ||
|
||
|
||
|
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 this variable, you can simply return the values.
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.
That's what I originally had. I introduced the
stop
variable for readability. It makes it clear what the returned result is supposed to be. If results are directly returned one has to mentally parse every return statement, whereas this reads easy as the intent is clear.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.
let me know if you want me to change this anyway. No problem.
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 disagree that it's making it more readable, but I'm fine to keep it there as it's just a code example :)