[Twisted-Python] Configuration options for twisted code
While using twisted.web (but not only) I had to sub-class twisted code to implement the following: HTTPChannel * reduce limit of HTTP headers length * reduce number of HTTP headers * connection timeout HTTPRequest * change HTTP server signature * change HTTP cookie name and secure/HTTPOnly flag Static.File * resource for child not found * resource for forbidden * template for directory listing. During review process, one feedback was to add these options as part of __init__ method for the required classes and another one was to design some configuration system, which allows changing these options without sub-classing. What do you think? Should I go ahead and add options as __init__ arguments or think of a configuration system? Thanks! -- Adi Roiban
On 03/03/2014 01:09 PM, Adi Roiban wrote:
What do you think? Should I go ahead and add options as __init__ arguments or think of a configuration system?
A configuration system would take a while to design and meanwhile these unrelated features would languish, so I'd suggest you start with options to __init__.
On 04/03/2014 00:52, Itamar Turner-Trauring wrote:
-1 I hate __init__ methods with a hojillion parameters, and that's where this leads. +1 instead for class variables which can be overridden in sub-classes to achieve DRY and/or set after instantiation if necessary e.g. class Foo: maxThingLen = 10000 class MyFoo(Foo): maxThingLen = 100 This is also somewhat consistent with what things like t.w.s.Site already do.
On 07:41 am, p.mayers@imperial.ac.uk wrote:
Functions that take arguments are great. If your function takes *too many* arguments then maybe the function does too many things or maybe there is some structure spread across those arguments that should be explicitly reflected in their type (iow maybe those last five arguments are all actually part of the same thing and should be represented by a single argument instead).
This is compatible with accepting these values as arguments. Defaults that provide good behavior by default for the common case are great. Jean-Paul
A configuration system would take a while to design
This reads like: A configuration system will never be implemented. I agree that for the time being we can implement current tickets using __init__ arguments, but why not try to implement something simple? Example: TWISTED_WEB_DEFAULT_CONFIGURATIO = { 'request.headers.count': 500, 'request.headers.size': 16384, 'request.session.name' : 'TWISTED_SESSION', } # We can have a helper method to get default configuration configInstance = TWISTED_WEB_DEFAULT_CONFIGURATION.copy() configInstance[''request.headers.count'] = 100 t.w.s.Site(configuration=configInstance) Or a bit more structured: class RequestConfiguration(object): def __init__(self): self.max_headers_count = 500 self.request_max_header_size = 16384 self.session_cookie_name = 'TWISTED_SESSION' self.session_cookie_secure = True self.session_cookie_http_only = True class TwistedWebConfiguration(object): def __init__(self): self.request = RequestConfiguration() configInstance = TwistedWebConfiguration() configInstance.request.max_headers_count = 100 t.w.s.Site(configuration=configInstance) Or a mix: configInstance = TwistedWebConfiguration() configInstance.set('request.headers.count', 100) t.w.s.Site(configuration=configInstance) and support some sort of inheritance. configInstance.set('timeout', 100) assert configInstance.get('request.timeout') == 100 configInstance.set('request.timeout', 200) assert configInstance.get('request.timeout') == 200 Or some design used in other project... or some other crazy idea. ----- In Twisted web I found both configuration overridden in sub-classes (t.w.s.Site) and defined via __init__ (t.w.s.File) twisted.web.static.File has 5 arguments, but the following configuration are missing : indexNames, childNotFoundResource, forbiddenResource, directoryListing resource As Jean-Paul commented, when there are too many arguments and they are all related, they could be represented by a single argument. For me, this single argument could be a configuration object. Too many arguments are a code smell, but when you can say that a method has too many arguments? Thanks! -- Adi Roiban
On 12:41 pm, adi@roiban.ro wrote:
The big downside of a "configuration object" is that "configuration" isn't any kind of abstraction. "Configuration" approximately means "stuff". Where does your "configuration" end? By just saying this is where "configuration" goes you've defined its scope as infinite. Before you know it, there's 80,000 items in your "configuration" object and you're really much worse off than if you'd just added a couple more arguments to `__init__`. I'd like to see this discussion going in the other direction so to give it a little push that way, here's a specific suggestion. :) Most or all of this may be recycled from discussion that's already taken place - I'm not taking credit, just repeating it. :) Give `Request.__init__` some new optional arguments to control the size of the header. This could be several arguments like `maxHeaderLines` and `maxHeaderValueLength` and so on... Or perhaps it should just be one argument that can completely describe the header size limitation (stretching this idea further, perhaps the one argument is a callable that takes a `Headers` instance and determines if it has gotten to big). Then, perhaps, also give `HTTPFactory.__init__` (and therefore `Site.__init__` a `requestFactory` argument so that the request class can be replaced more easily (although setting the `requestFactory` attribute after creating an instance isn't *too* bad). These two things combined make the usage pretty flexible while still being pretty simple and without involving any more complicated "configuration" system: def requestFactory(*args, **kwargs): kwargs["headerSizeLimit"] = HeaderSizeLimit( maxLines=23, maxTotalBytes=57, ...) return Site.requestFactory(*args, **kwargs) site = Site(..., requestFactory=requestFactory) If anyone is really interested in building (or adopting) a more general "configuration" system for Twisted then I suggest you start by looking up some of Twisted's past attempts including the the "Coil" library Itamar started (he'll have to provide a link, I've lost track of where it lives now). Jean-Paul
On 4 March 2014 15:22, <exarkun@twistedmatrix.com> wrote:
Agree.
Why setting `requestFactory` attribute after instanciation isn't too bad? Why not apply the same rule for setting attributes like maxHeaders and maxHeader size in Request itself?
The HeaderSizeLimit class looks like a Configuration class and I am happy with it. I will go and update the branches to include suggested changes. Thank you all for the feedback!
I am not really interested in building a big "configuration" system for Twisted so don't worry for the link. Thanks! -- Adi Roiban
On 03/03/2014 01:09 PM, Adi Roiban wrote:
What do you think? Should I go ahead and add options as __init__ arguments or think of a configuration system?
A configuration system would take a while to design and meanwhile these unrelated features would languish, so I'd suggest you start with options to __init__.
On 04/03/2014 00:52, Itamar Turner-Trauring wrote:
-1 I hate __init__ methods with a hojillion parameters, and that's where this leads. +1 instead for class variables which can be overridden in sub-classes to achieve DRY and/or set after instantiation if necessary e.g. class Foo: maxThingLen = 10000 class MyFoo(Foo): maxThingLen = 100 This is also somewhat consistent with what things like t.w.s.Site already do.
On 07:41 am, p.mayers@imperial.ac.uk wrote:
Functions that take arguments are great. If your function takes *too many* arguments then maybe the function does too many things or maybe there is some structure spread across those arguments that should be explicitly reflected in their type (iow maybe those last five arguments are all actually part of the same thing and should be represented by a single argument instead).
This is compatible with accepting these values as arguments. Defaults that provide good behavior by default for the common case are great. Jean-Paul
A configuration system would take a while to design
This reads like: A configuration system will never be implemented. I agree that for the time being we can implement current tickets using __init__ arguments, but why not try to implement something simple? Example: TWISTED_WEB_DEFAULT_CONFIGURATIO = { 'request.headers.count': 500, 'request.headers.size': 16384, 'request.session.name' : 'TWISTED_SESSION', } # We can have a helper method to get default configuration configInstance = TWISTED_WEB_DEFAULT_CONFIGURATION.copy() configInstance[''request.headers.count'] = 100 t.w.s.Site(configuration=configInstance) Or a bit more structured: class RequestConfiguration(object): def __init__(self): self.max_headers_count = 500 self.request_max_header_size = 16384 self.session_cookie_name = 'TWISTED_SESSION' self.session_cookie_secure = True self.session_cookie_http_only = True class TwistedWebConfiguration(object): def __init__(self): self.request = RequestConfiguration() configInstance = TwistedWebConfiguration() configInstance.request.max_headers_count = 100 t.w.s.Site(configuration=configInstance) Or a mix: configInstance = TwistedWebConfiguration() configInstance.set('request.headers.count', 100) t.w.s.Site(configuration=configInstance) and support some sort of inheritance. configInstance.set('timeout', 100) assert configInstance.get('request.timeout') == 100 configInstance.set('request.timeout', 200) assert configInstance.get('request.timeout') == 200 Or some design used in other project... or some other crazy idea. ----- In Twisted web I found both configuration overridden in sub-classes (t.w.s.Site) and defined via __init__ (t.w.s.File) twisted.web.static.File has 5 arguments, but the following configuration are missing : indexNames, childNotFoundResource, forbiddenResource, directoryListing resource As Jean-Paul commented, when there are too many arguments and they are all related, they could be represented by a single argument. For me, this single argument could be a configuration object. Too many arguments are a code smell, but when you can say that a method has too many arguments? Thanks! -- Adi Roiban
On 12:41 pm, adi@roiban.ro wrote:
The big downside of a "configuration object" is that "configuration" isn't any kind of abstraction. "Configuration" approximately means "stuff". Where does your "configuration" end? By just saying this is where "configuration" goes you've defined its scope as infinite. Before you know it, there's 80,000 items in your "configuration" object and you're really much worse off than if you'd just added a couple more arguments to `__init__`. I'd like to see this discussion going in the other direction so to give it a little push that way, here's a specific suggestion. :) Most or all of this may be recycled from discussion that's already taken place - I'm not taking credit, just repeating it. :) Give `Request.__init__` some new optional arguments to control the size of the header. This could be several arguments like `maxHeaderLines` and `maxHeaderValueLength` and so on... Or perhaps it should just be one argument that can completely describe the header size limitation (stretching this idea further, perhaps the one argument is a callable that takes a `Headers` instance and determines if it has gotten to big). Then, perhaps, also give `HTTPFactory.__init__` (and therefore `Site.__init__` a `requestFactory` argument so that the request class can be replaced more easily (although setting the `requestFactory` attribute after creating an instance isn't *too* bad). These two things combined make the usage pretty flexible while still being pretty simple and without involving any more complicated "configuration" system: def requestFactory(*args, **kwargs): kwargs["headerSizeLimit"] = HeaderSizeLimit( maxLines=23, maxTotalBytes=57, ...) return Site.requestFactory(*args, **kwargs) site = Site(..., requestFactory=requestFactory) If anyone is really interested in building (or adopting) a more general "configuration" system for Twisted then I suggest you start by looking up some of Twisted's past attempts including the the "Coil" library Itamar started (he'll have to provide a link, I've lost track of where it lives now). Jean-Paul
On 4 March 2014 15:22, <exarkun@twistedmatrix.com> wrote:
Agree.
Why setting `requestFactory` attribute after instanciation isn't too bad? Why not apply the same rule for setting attributes like maxHeaders and maxHeader size in Request itself?
The HeaderSizeLimit class looks like a Configuration class and I am happy with it. I will go and update the branches to include suggested changes. Thank you all for the feedback!
I am not really interested in building a big "configuration" system for Twisted so don't worry for the link. Thanks! -- Adi Roiban
participants (4)
-
Adi Roiban -
exarkun＠twistedmatrix.com -
Itamar Turner-Trauring -
Phil Mayers