Bugfix/gzip #35
Bugfix/gzip #35
Conversation
andrewleech
left a comment
andrewleech
left a comment
There was a problem hiding this comment.
Fantastic cleanup & consolidation in data_to_bytes_iterator() and send_body_encoded(). I've read through them and to me it does look like you've correctly captured the slightly different usages of this code nicely with a bunch of great simplifications along the way!
I don't have any way to really test this myself other than the unit tests run here in Actions - which fail but they fail in the same way as the master branch :-) so it's equivalent as far as that goes!
| else: | ||
| content_type = dc.get_prop(uri, "DAV:", "getcontenttype") | ||
| if content_type == 'httpd/unix-directory': | ||
| content_type = 'text/html;charset=utf-8' |
There was a problem hiding this comment.
What inspired this change?
I'm not sure what the webdav spec suggests, however a google of webdav mime "httpd/unix-directory" gets a lot of hits; it seems to be pretty common?
There was a problem hiding this comment.
This is not a WebDAV response, but a standard HTTP GET/HEAD response returning the list of files as a HTML. Serving the HTML file as httpd/unix-directory makes no sense. It breaks the webbrowser pointed at that URL, while WebDAV clients would nevertheless use the WebDAV method PROPFIND and not the GET/HEAD.
There was a problem hiding this comment.
Ah ok, that makes sense. I didn't actually realise the server would serve "regular" http requests as well as webdav ones.
| if os.path.isdir(path): | ||
| for filename in self.index_files: | ||
| new_path = os.path.join(path, filename) | ||
| if os.path.isfile(new_path): | ||
| path = new_path | ||
| break | ||
| else: | ||
| msg = self._get_listing(path) | ||
| return Resource(StringIO(msg), len(msg)) |
There was a problem hiding this comment.
I gather this index file handing is quite non-standard for webdav servers?
Does this mean that if it's enabled and you try to view a folder that's got an index style file in it, you get served that file rather than the directory listing?
by default is disabled
Looking at the code, is an end user expected to edit this python file to add filenames to index_files = () above?
Or have to create a wrapper script that imports this, injects the names, then start the server programatically?
For this to be included I think it'd need at least some docs, if not a command line argument to enable?
There was a problem hiding this comment.
-
WebDAV allows that behavior:
http://www.webdav.org/specs/rfc4918.html#rfc.section.9.4 -
I intended it to be extended via inheritance, like this:
class CustomFilesystemHandler(FilesystemHandler):
mimecheck = True
index_files = ("index.html",)However I see it would be better if I write some docs for that feature.
| log.debug("Don't use iterator") | ||
|
|
||
| for buf in data: | ||
| yield buf if isinstance(buf, six.binary_type) else str(buf).encode('utf-8') |
There was a problem hiding this comment.
This would be safer as:
| yield buf if isinstance(buf, six.binary_type) else str(buf).encode('utf-8') | |
| yield buf if isinstance(buf, six.binary_type) else bytes(buf, 'utf-8') |
Using str(buf) could cause an undesired output if buf happens to be something that doesn't implicitely convert to the str cleanly.
There was a problem hiding this comment.
Maybe we should just throw exception in such case? This should be str or bytes anyway.
There was a problem hiding this comment.
Well I don't think it's an expected error, throwing an exception is ugly useful if you're going to catch and deal with it somewhere.
Without tracing the code through I could see it potentially being a bytearray, or a future change might want to change it to one...
This was more of a best practices suggestion, if you want to convert the object to a bytes it's better to convert in one step, rather than convert to str first and then to bytes.
Completed comprehensive review of PR #35 which adds index file serving functionality to the WebDAV server. The review identifies: BLOCKERS: - Feature is non-functional (empty index_files tuple) - No documentation - No test coverage RECOMMENDATIONS: - Add configurable index_files parameter - Document WebDAV semantic changes - Add comprehensive test suite - Consider CLI option for server configuration The core implementation logic is sound but requires significant changes before merge. See PR_35_REVIEW.md for full analysis. Recommendation: DO NOT MERGE without addressing blocker issues.
MAJOR REVISION based on reviewing the full PR discussion and all 3 commits. KEY FINDINGS: - PR includes 3 commits, not just index.html feature - Commit 1: Excellent gzip refactoring (maintainer praised it) - Commit 2: Correct MIME type fix (maintainer agreed after explanation) - Commit 3: Index files feature (intentionally uses inheritance pattern) CHANGED ASSESSMENT: Previous review incorrectly assessed the index files feature as "non-functional" due to empty tuple. Author clarified in discussion this was intentional design using inheritance pattern (matching existing mimecheck attribute). Maintainer's concerns were addressed in discussion, but documentation promised by author was never added, causing 2-year stall. UPDATED RECOMMENDATION: CONDITIONAL APPROVAL - merge after adding documentation for inheritance pattern and rebasing to remove 'six' library usage. This is good code stuck in review purgatory on a documentation technicality that was resolved in principle but never executed.
2 participants
httpd/unix-directorymakes no sense with a HTML listing of a directory.