Closed bugfest closed 1 year ago
Merging #207 (4c3235f) into master (5f6c7cc) will increase coverage by
51.38%
. Report is 5 commits behind head on master. The diff coverage is91.66%
.:exclamation: Current head 4c3235f differs from pull request most recent head 4f6df09. Consider uploading reports for the commit 4f6df09 to get more accurate results
@@ Coverage Diff @@
## master #207 +/- ##
===========================================
+ Coverage 31.00% 82.38% +51.38%
===========================================
Files 3 6 +3
Lines 200 613 +413
===========================================
+ Hits 62 505 +443
+ Misses 132 73 -59
- Partials 6 35 +29
Files Changed | Coverage Δ | |
---|---|---|
router.go | 90.76% <88.88%> (ø) |
|
config.go | 100.00% <100.00%> (ø) |
|
helper.go | 87.83% <100.00%> (ø) |
|
prefetch.go | 100.00% <100.00%> (ø) |
|
webp-server.go | 78.78% <100.00%> (ø) |
... and 4 files with indirect coverage changes
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
waiting on #206 agreement to fix this PR accordingly
Hash algorithm: we're using it for identification, not cryptography, so choose the fastest algorithm https://cyan4973.github.io/xxHash/
@n0vad3v, @BennyThink this PR is ready for review
Great! I will take a look during weekends!
Emmm, I've tried this branch with configuration as below, with some images under pics
and pics2
(pics3
does not exist).
{
"HOST": "127.0.0.1",
"PORT": "3333",
"IMG_PATH": "./pics",
"EXHAUST_PATH": "./exhaust",
"IMG_MAP": {
"/2": "./pics2",
"/3": "./pics3",
"www.example.com": "http://origin.example.com:8000",
"http://www.example.com": "https://docs.webp.sh"
},
"ALLOWED_TYPES": ["jpg","png","jpeg"],
"ENABLE_AVIF": false,
"ENABLE_EXTRA_PARAMS": false
}
http://localhost:3333/DSC05953.jpg
, works finehttp://localhost:3333/2/Unknown.jpg
where Unknown.jpg
is an image under pics2
directory, returns image not found
http://localhost:3333/www.example.com/
, returns File extension not allowed! www.example.com
http://localhost:3333/www.example.com/1.jpg
, returns image not found
Not sure if I've done something wrong 🤔
BTW, should we delete the metadata JSON file if the image doesn't exist, inside the function below:
// Check the original image for existence,
if !helper.ImageExists(rawImageAbs) {
msg := "image not found"
_ = c.Send([]byte(msg))
log.Warn(msg)
_ = c.SendStatus(404)
// DELETE JSON HERE?
return nil
}
Thanks @n0vad3v, going to add test cases for those and fix them accordingly
Hi @n0vad3v,
Emmm, I've tried this branch with configuration as below, with some images under
pics
andpics2
(pics3
does not exist).{ "HOST": "127.0.0.1", "PORT": "3333", "IMG_PATH": "./pics", "EXHAUST_PATH": "./exhaust", "IMG_MAP": { "/2": "./pics2", "/3": "./pics3", "www.example.com": "http://origin.example.com:8000", "http://www.example.com": "https://docs.webp.sh" }, "ALLOWED_TYPES": ["jpg","png","jpeg"], "ENABLE_AVIF": false, "ENABLE_EXTRA_PARAMS": false }
1. Tried to visit: `http://localhost:3333/DSC05953.jpg`, works fine 2. Tried to visit: `http://localhost:3333/2/Unknown.jpg` where `Unknown.jpg` is an image under `pics2` directory, returns `image not found` 3. Tried to visit: `http://localhost:3333/www.example.com/`, returns `File extension not allowed! www.example.com` 4. Tried to visit: `http://localhost:3333/www.example.com/1.jpg`, returns `image not found`
I forgot to implement the path mapping case :_)
I implemented it and added more tess to cover these scenarios. The most important decision is assumming keys not starting with "/" or "http://" or "https://" won't be applied. I haven't showed any warning as I'm not parsing that config piece on boot (should we?); right now invalid entries are just skipped.
BTW, should we delete the metadata JSON file if the image doesn't exist, inside the function below:
// Check the original image for existence, if !helper.ImageExists(rawImageAbs) { msg := "image not found" _ = c.Send([]byte(msg)) log.Warn(msg) _ = c.SendStatus(404) // DELETE JSON HERE? return nil }
Yeah, probably it's a good idea. Is something we need to do in this PR?
Nice and thank you! ❤️ (Sorry I've missed your reply and update for a long time)
I haven't showed any warning as I'm not parsing that config piece on boot (should we?);
Probably adding a warining there will be better.
Is something we need to do in this PR?
No, we will update this in another PR.
I'm still using config.json
with content as below:
{
"HOST": "127.0.0.1",
"PORT": "3333",
"IMG_PATH": "",
"EXHAUST_PATH": "./exhaust",
"IMG_MAP": {
"/2": "./pics2",
"/3": "./pics3",
"www.example.com": "http://origin.example.com:8000",
"http://www.example.com": "https://docs.webp.sh"
},
"ALLOWED_TYPES": ["jpg","png","jpeg"],
"ENABLE_AVIF": false,
"ENABLE_EXTRA_PARAMS": false
}
And found that if IMG_PATH
is set to ./pics
, then http://localhost:3333/2/Unknown.jpg
will try to use image at ./pics/pic2/Unknown.jpg
, but if IMG_PATH
is set to empty, images will be served from ./pic2/Unknown.jpg
.
Maybe there need to be a big enhancement on our documentation site after this PR merged and before the next version released. (Maybe we need to dedicate the configuration page from https://docs.webp.sh/usage/usage-with-binary/) @BennyThink
Nice and thank you! ❤️ (Sorry I've missed your reply and update for a long time)
I haven't showed any warning as I'm not parsing that config piece on boot (should we?);
Probably adding a warining there will be better.
Ok, will do. I'll parse the config at boot to show these warnings once if applicable.
I'm still using
config.json
with content as below:{ "HOST": "127.0.0.1", "PORT": "3333", "IMG_PATH": "", "EXHAUST_PATH": "./exhaust", "IMG_MAP": { "/2": "./pics2", "/3": "./pics3", "www.example.com": "http://origin.example.com:8000", "http://www.example.com": "https://docs.webp.sh" }, "ALLOWED_TYPES": ["jpg","png","jpeg"], "ENABLE_AVIF": false, "ENABLE_EXTRA_PARAMS": false }
And found that if
IMG_PATH
is set to./pics
, thenhttp://localhost:3333/2/Unknown.jpg
will try to use image at./pics/pic2/Unknown.jpg
, but ifIMG_PATH
is set to empty, images will be served from./pic2/Unknown.jpg
.
I assummed all paths are relative to IMG_PATH, but I guess we can make then relative to the CWD of the process - is it what you expect @n0vad3v?
is it what you expect
Yes, because there are no documents to refer, my first reaction is that the addresses in each IMG_MAP are relative paths starting from the CWD.(However if IMG_PATH
is set to empty, it indeed works as I expected)
Perhaps it would be better for each one to start from the CWD (Current Working Directory)?
(It's unlikely that someone would need to serve multiple paths simultaneously, as all those paths would be under IMG_PATH
anyway.)
However, I believe both approaches are acceptable as long as they are clearly documented.
Hi @n0vad3v, I've added the patch for the CWD and the IMG_MAP parser on start. Please review.
Nice! I'm merging this PR.
Implements #206
IMG_MAP
as per #217h2non/filetype
upgraded)