bpo-46436: Fix command-line option -d/--directory in module http.server

[geryogam]

Fix command-line option -d/--directory in http.server main
function that was ignored when combined with --cgi.

https://bugs.python.org/issue46436

Automerge-Triggered-By: GH:merwok

Automerge-Triggered-By: GH:merwok

[JelleZijlstra]

The substantive change looks good and I confirmed that it worked, but I agree that you should back out the unnecessary changes such as those in import order. They're not necessary to the fix.

[merwok]

Thanks! Do revert the import change then this can be merged

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[geryogam]

@merwok Done.

I have made the requested changes; please review again.

[bedevere-bot]

Thanks for making the requested changes!

@merwok: please review the changes made to this pull request.

[merwok]

Please see https://bugs.python.org/issue46285

[geryogam]

Thanks for mentioning this issue @merwok.

As you pointed out, function test in module http.server expects a real request handler class argument (SimpleHTTPRequestHandler or CGIHTTPRequestHandler), not a partial object partial(SimpleHTTPRequestHandler, directory=args.directory) or partial(CGIHTTPRequestHandler, directory=args.directory), so that the assignment of class attribute protocol_version in test is not ignored.

The partial object in the if __name__ == '__main__': branch of module http.server was introduced in the first place to pass the directory argument at request handler class instantiation in method BaseServer.finish_request of module socketserver:

def finish_request(self, request, client_address): """Finish one request by instantiating RequestHandlerClass.""" self.RequestHandlerClass(request, client_address, self) 

But method BaseServer.finish_request is a factory method of class BaseServer (the abstract creator) so it is designed to be overridden in subclasses to customize the instantiation of request handler subclasses of class BaseRequestHandler (the abstract product). So the proper way to instantiate subclasses SimpleHTTPRequestHandler and CGIHTTPRequestHandler with the directory argument is to override BaseServer.finish_request, not using a partial object.

I have just updated my PR to implement this. It fixes both #46285 and #46436. It also adds a command-line option -p/--protocol to module http.server and passes it to function test in the if __name__ == '__main__' branch (the protocol argument of test is now no longer ignored since we also pass a real request handler class instead of a partial object).

[merwok]

Thanks for fixing both issues! I’ll review the code to see if it also addresses my bad feeling about setting attributes directly on class objects.

Can we reference two tickets in one pull request title? Also have two blurb notes?

[merwok]

Nice trick with using a closure to keep finish_request’s signature compliant with the definition in socketserver.
On the other hand, I wonder if it wouldn’t be more straightforward to add a directory parameter to test.

test still sets attributes on the passed classes directly, which I find fishy, but it’s not the job of this PR to change that, and maybe it doesn’t need to be fixed at all (it’s a main function, not one that would be imported and called from regular code).

[geryogam]

Nice trick with using a closure to keep finish_request’s signature compliant with the definition in socketserver. On the other hand, I wonder if it wouldn’t be more straightforward to add a directory parameter to test.

Yes that is another solution: moving the creation of partial objects with argument directory after the assignment of class attribute protocol_version in function test. The drawback of this approach is that, as you mentioned, it requires changing the signature of test (though it would be backward compatible and test is not part of the API of module http.server), and it requires such a change each time the signature of the __init__ method of a handler subclass changes (i.e. it couples test with __init__).

Factory methods (like finish_request) are a design pattern that avoids such information distribution.

[merwok]

I wasn’t suggesting to use partial objects, if that’s necessary then keeping your technique is better.
(Otherwise I don’t mind changing the signature of test, just like it was changed for protocol)

[geryogam]

I wasn’t suggesting to use partial objects, if that’s necessary then keeping your technique is better.
(Otherwise I don’t mind changing the signature of test, just like it was changed for protocol)

Yes I think that is necessary. To instantiate a request handler with the directory argument from the method finish_request of a server, we have only two solutions:

1. The partial object solution: pass a callable partial(RequestHandlerClass, directory=directory) to the server as the RequestHandlerClass argument (which becomes the self.RequestHandlerClass instance attribute) and let the server inherit the method finish_request of the class socketserver.BaseServer:

 def finish_request(self, request, client_address): """Finish one request by instantiating RequestHandlerClass.""" self.RequestHandlerClass(request, client_address, self) 

2. The factory method solution: override the method finish_request of the class socketserver.BaseServer with the instantiation of the class RequestHandlerClass:

 def finish_request(self, request, client_address): RequestHandlerClass(request, client_address, self, directory=directory) 

The designer of the class socketserver.BaseServer gave us the two possibilities. In the first solution, finish_request is a useless factory method. In the second solution, self.RequestHandlerClass is a useless instance attribute.

@orsenthil, do you a preference?

[geryogam]

Thanks for getting the PR ready to merge!
Can you confirm that you tested it locally?

Thanks for reviewing it @merwok! I cleaned up my history and tested the HTTP server locally with Curl for both PRs and I confirm that they both work. You can merge them.

[merwok]

Can you add your name to Misc/ACKS?

[geryogam]

Can you add your name to Misc/ACKS?

@merwok It is already there actually.

[miss-islington]

Sorry, I can't merge this PR. Reason: You're not authorized to push to this branch. Visit https://docs.github.com/articles/about-protected-branches/ for more information..

[geryogam]

@merwok Is there anything that I can do to let miss-islington merge?

[miss-islington]

Thanks @maggyero for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

[bedevere-bot]

GH-31102 is a backport of this pull request to the 3.10 branch.

[bedevere-bot]

GH-31103 is a backport of this pull request to the 3.9 branch.

[merwok]

The main branch was locked temporarily, tests were always failing because of a missing indent in a new test.

[geryogam]

@merwok Thanks a lot for your careful review! It really improved the quality of this PR.

I think we can now merge the other PR.