fileserver: Support `precompressed` files without base files #5745

singhalkarun · 2023-08-14T12:17:49Z

Changes done so far:

Added a new bool value to the FileServer Struct called PreferPrecompressed
Start accepting prefer_precompressed subdirective under the fileserver directive
If PreCompressed is enabed on fileserver, ignore checking the existence of the original file and serve encoded file as per the accepted encoding if available, else return 404.

To Do:

If the file is not available as per accepted encoding, try to decompress the encoded file to original file and serve, if not able to, return 404. (Not handling this in current PR, will be opening a new PR for this.)

@francislavoie

…mpressed 2. Start accepting prefer_precompressed subdirective under the fileserver directive

CLAassistant · 2023-08-14T12:17:54Z

All committers have signed the CLA.

…tence of the original file and serve encoded file as per the accepted encoding if available, else return 404. To Do Next: If the file is not available as per accepted encoding, try to decompress the encoded file to original file and serve, if not able to, return 404.

singhalkarun · 2023-08-14T14:32:14Z

Hi @francislavoie looking forward for your feedback on the above commits before I proceed with further work. Can you please have a look?

mholt · 2023-08-14T15:45:51Z

Thanks for the enhancement!

This is looking pretty good.

I am not quite sure about this, though:

If the file is not available as per accepted encoding, try to decompress the encoded file to original file and serve, if not able to, return 404.

Is this something we should actually do? And if so, is 404 the right status code?

Note: We're on feature freeze until 2.9 development, so even if this gets merged soon it likely won't get released until after 2.8.

modules/caddyhttp/fileserver/staticfiles.go

…compressedInfo 2. Fixed an issue when the PreCompressed parameter is enabled and the original file is also present, need to fetch the FileInfo such that it can be used in ServeContent.

singhalkarun · 2023-08-15T22:12:08Z

Is this something we should actually do? And if so, is 404 the right status code?

Not very sure on this. But considering the main discussion thread (https://caddy.community/t/precompressed-files-without-base-files/17330),

We should be able to serve the file even if the precompressed version doesn't exist (This is already solved by the PR)
If the client doesn't support compressed version, we should decompress and serve (This is yet to be solved which can be discussed over)

For now, let me convert the draft PR to a normal PR and I'll open a separate PR that handles the 2nd scenario and we can continue the discussion over there.

Associated with #5116

@francislavoie @mholt

mholt

Thanks for this feature. I think it's looking pretty good, but I haven't done a thorough inspection or tried it yet.

It might be best to add a test case (an integration test) that verifies it works.

Then we'll get this merged in for ~2.9!

mholt · 2023-08-18T15:58:16Z

modules/caddyhttp/fileserver/staticfiles.go

@@ -132,6 +132,8 @@ type FileServer struct {
 	// clobbering the explicit rewrite with implicit behavior.
 	CanonicalURIs *bool `json:"canonical_uris,omitempty"`

+	PreferPrecompressed bool `json:"prefer_precompressed,omitempty"`


We should add godoc comments to this field, otherwise it'll appear without documentation on the Caddy website (in the JSON config section).

Got it. Let me add the godoc comments to this field.

singhalkarun · 2023-08-18T17:01:24Z

Thanks for this feature. I think it's looking pretty good, but I haven't done a thorough inspection or tried it yet.

It might be best to add a test case (an integration test) that verifies it works.

Then we'll get this merged in for ~2.9!

Sure, let me add test cases.

mholt · 2023-09-07T23:10:03Z

@singhalkarun I think we're almost done with this. Any interest in finishing it up?

francislavoie · 2023-09-07T23:27:16Z

modules/caddyhttp/fileserver/staticfiles.go

@@ -476,6 +492,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
 	// that errors generated by ServeContent are written immediately
 	// to the response, so we cannot handle them (but errors there
 	// are rare)
+


Don't need this newline anymore

Suggested change

francislavoie · 2023-09-07T23:28:07Z

modules/caddyhttp/fileserver/staticfiles.go

-		fsrv.logger.Debug("opening file", zap.String("filename", filename))
-


I think we should preserve the debug log, it's helpful for tracing. In fact, maybe we should add more debug logs in here relating to which file is being picked depending on the precompressed options.

singhalkarun · 2023-09-08T14:49:30Z

@singhalkarun I think we're almost done with this. Any interest in finishing it up?

@mholt Sorry, got occupied with other things. Can I work on this after 15 Sep if it works?

mholt · 2023-09-08T14:55:07Z

Yeah, absolutely. I just wanted to check in for status. 👌

mholt · 2024-04-18T03:50:41Z

@singhalkarun Just wanted to check in one more time, wondering if you're still interested in this.

singhalkarun · 2024-04-18T14:17:00Z

Thanks for the enhancement!

This is looking pretty good.

I am not quite sure about this, though:

If the file is not available as per accepted encoding, try to decompress the encoded file to original file and serve, if not able to, return 404.

Is this something we should actually do? And if so, is 404 the right status code?

Note: We're on feature freeze until 2.9 development, so even if this gets merged soon it likely won't get released until after 2.8.

Hi @mholt I would like to pass this one for now due to time constraints. Thanks.

mholt · 2024-04-18T15:12:26Z

Alrighty, thanks for the update.

1. Added a new bool value to the FileServer Struct called PreferPreco…

d1ef85b

…mpressed 2. Start accepting prefer_precompressed subdirective under the fileserver directive

francislavoie reviewed Aug 14, 2023

View reviewed changes

modules/caddyhttp/fileserver/staticfiles.go Outdated Show resolved Hide resolved

1. Refactored to use a info variable instead of 2 variables info and …

4e97c97

…compressedInfo 2. Fixed an issue when the PreCompressed parameter is enabled and the original file is also present, need to fetch the FileInfo such that it can be used in ServeContent.

singhalkarun marked this pull request as ready for review August 15, 2023 22:12

singhalkarun requested a review from francislavoie August 17, 2023 18:58

mholt requested changes Aug 18, 2023

View reviewed changes

francislavoie reviewed Sep 7, 2023

View reviewed changes

francislavoie changed the title ~~Support "precompressed" files without "base" files (#5116)~~ fileserver: Support precompressed files without base files Sep 8, 2023

francislavoie added the feature ⚙️ New feature or request label Sep 8, 2023

francislavoie added this to the 2.x milestone Sep 8, 2023

mholt closed this Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fileserver: Support `precompressed` files without base files #5745

fileserver: Support `precompressed` files without base files #5745

singhalkarun commented Aug 14, 2023 •

edited by francislavoie

Loading

CLAassistant commented Aug 14, 2023 •

edited

Loading

singhalkarun commented Aug 14, 2023

mholt commented Aug 14, 2023

singhalkarun commented Aug 15, 2023 •

edited

Loading

mholt left a comment

mholt Aug 18, 2023

singhalkarun Aug 18, 2023

singhalkarun commented Aug 18, 2023

mholt commented Sep 7, 2023

francislavoie Sep 7, 2023

francislavoie Sep 7, 2023

singhalkarun commented Sep 8, 2023

mholt commented Sep 8, 2023

mholt commented Apr 18, 2024

singhalkarun commented Apr 18, 2024

mholt commented Apr 18, 2024

		fsrv.logger.Debug("opening file", zap.String("filename", filename))

fileserver: Support precompressed files without base files #5745

fileserver: Support precompressed files without base files #5745

Conversation

singhalkarun commented Aug 14, 2023 • edited by francislavoie Loading

CLAassistant commented Aug 14, 2023 • edited Loading

singhalkarun commented Aug 14, 2023

mholt commented Aug 14, 2023

singhalkarun commented Aug 15, 2023 • edited Loading

mholt left a comment

Choose a reason for hiding this comment

mholt Aug 18, 2023

Choose a reason for hiding this comment

singhalkarun Aug 18, 2023

Choose a reason for hiding this comment

singhalkarun commented Aug 18, 2023

mholt commented Sep 7, 2023

francislavoie Sep 7, 2023

Choose a reason for hiding this comment

francislavoie Sep 7, 2023

Choose a reason for hiding this comment

singhalkarun commented Sep 8, 2023

mholt commented Sep 8, 2023

mholt commented Apr 18, 2024

singhalkarun commented Apr 18, 2024

mholt commented Apr 18, 2024

fileserver: Support `precompressed` files without base files #5745

fileserver: Support `precompressed` files without base files #5745

singhalkarun commented Aug 14, 2023 •

edited by francislavoie

Loading

CLAassistant commented Aug 14, 2023 •

edited

Loading

singhalkarun commented Aug 15, 2023 •

edited

Loading