Hey guys. I just wanted to post about a problem I've been working through and a potential solution to get some feedback. Effectively, there's a bunch of related code that handles various processing of the Rack environment. However, much of this processing is triggered by requests by the user, and can be expensive. The current recommended approach is to create a middleware that processes the rack environment as needed, and stuff the results back into the environment hash. This works fine for cases where you know the processing will be needed, but not as well for cases where the processing is to be used optionally by a user downstream in the middleware chain. Before rack, the approach both Rails and Merb used was to do that processing in a Request object that could process the environment only when a specific accessor was called. Some examples include "method", a 10-line method in Merb, various parameter parsing (which can be quite expensive), remote_ip (another function that is longer than it seems like is reasonable), and it goes on and on. What is desirable is for the appropriate processing to be done when required, and only once. Whether or not the user is in the Rails/Merb application or Rack middleware should not affect this (the user should have access to any required processing). Unfortunately, since the Rack environment is required to be a "real" Hash, it's not possible to modify the environment to support lazy-loaded and memoized processing. One option we *considered* was a middleware that did something like env["rack.request_obj"] = Rack::Request.new(env). This would provide the benefits of a logical object for the request without breaking Rack conventions. However, after emailing Chris to explain this problem, he suggested an even better (to my eyes) solution: > I think I have a better idea for this... we could make > Rack::Request.new cache the request object in the env itself(!) and > just return it when it was created and calculated already. This would > be backward compatible, efficient, and not expose any details on the > implementation. I love this idea and would love to get some feedback on it. Thanks a ton,
On Wed, Dec 24, 2008 at 5:35 PM, Yehuda Katz <wyc...@gmail.com> wrote: > Hey guys. I just wanted to post about a problem I've been working through > and a potential solution to get some feedback. Effectively, there's a bunch > of related code that handles various processing of the Rack environment. > However, much of this processing is triggered by requests by the user, and > can be expensive. The current recommended approach is to create a middleware > that processes the rack environment as needed, and stuff the results back > into the environment hash. This works fine for cases where you know the > processing will be needed, but not as well for cases where the processing is > to be used optionally by a user downstream in the middleware chain. Before > rack, the approach both Rails and Merb used was to do that processing in a > Request object that could process the environment only when a specific > accessor was called. Some examples include "method", a 10-line method in > Merb, various parameter parsing (which can be quite expensive), remote_ip > (another function that is longer than it seems like is reasonable), and it > goes on and on. What is desirable is for the appropriate processing to be > done when required, and only once. Whether or not the user is in the > Rails/Merb application or Rack middleware should not affect this (the user > should have access to any required processing). Unfortunately, since the > Rack environment is required to be a "real" Hash, it's not possible to > modify the environment to support lazy-loaded and memoized processing. One > option we *considered* was a middleware that did something like > env["rack.request_obj"] = Rack::Request.new(env). This would provide the > benefits of a logical object for the request without breaking Rack > conventions. However, after emailing Chris to explain this problem, he > suggested an even better (to my eyes) solution: > I think I have a better > idea for this... we could make > Rack::Request.new cache the request object > in the env itself(!) and > just return it when it was created and calculated > already. This would > be backward compatible, efficient, and not expose any > details on the > implementation. I love this idea and would love to get some > feedback on it. Thanks a ton,
Yep! This allows any middleware to use Rack::Request throughout the chain without having to worry about the overhead of continually setting up a new object and having to reprocess.
On Wed, Dec 24, 2008 at 2:56 PM, Matt Todd <chiol...@gmail.com> wrote: > Effectively...
> def Rack::Request.from(env) > unless env['rack.request'] > env['rack.request'] = Rack::Request.new(env) > end > env['rack.request'] > end
> Hmm?
> Matt
> On Wed, Dec 24, 2008 at 5:35 PM, Yehuda Katz <wyc...@gmail.com> wrote:
>> Hey guys. I just wanted to post about a problem I've been working through >> and a potential solution to get some feedback. Effectively, there's a bunch >> of related code that handles various processing of the Rack environment. >> However, much of this processing is triggered by requests by the user, and >> can be expensive. The current recommended approach is to create a middleware >> that processes the rack environment as needed, and stuff the results back >> into the environment hash. This works fine for cases where you know the >> processing will be needed, but not as well for cases where the processing is >> to be used optionally by a user downstream in the middleware chain. Before >> rack, the approach both Rails and Merb used was to do that processing in a >> Request object that could process the environment only when a specific >> accessor was called. Some examples include "method", a 10-line method in >> Merb, various parameter parsing (which can be quite expensive), remote_ip >> (another function that is longer than it seems like is reasonable), and it >> goes on and on. What is desirable is for the appropriate processing to be >> done when required, and only once. Whether or not the user is in the >> Rails/Merb application or Rack middleware should not affect this (the user >> should have access to any required processing). Unfortunately, since the >> Rack environment is required to be a "real" Hash, it's not possible to >> modify the environment to support lazy-loaded and memoized processing. One >> option we *considered* was a middleware that did something like >> env["rack.request_obj"] = Rack::Request.new(env). This would provide the >> benefits of a logical object for the request without breaking Rack >> conventions. However, after emailing Chris to explain this problem, he >> suggested an even better (to my eyes) solution: > I think I have a better >> idea for this... we could make > Rack::Request.new cache the request object >> in the env itself(!) and > just return it when it was created and calculated >> already. This would > be backward compatible, efficient, and not expose any >> details on the > implementation. I love this idea and would love to get some >> feedback on it. Thanks a ton,
On Wed, Dec 24, 2008 at 2:59 PM, Yehuda Katz <wyc...@gmail.com> wrote: > Yep! > This allows any middleware to use Rack::Request throughout the chain > without having to worry about the overhead of continually setting up a new > object and having to reprocess.
> w00t!
> -- Yehuda
> On Wed, Dec 24, 2008 at 2:56 PM, Matt Todd <chiol...@gmail.com> wrote:
>> Effectively...
>> def Rack::Request.from(env) >> unless env['rack.request'] >> env['rack.request'] = Rack::Request.new(env) >> end >> env['rack.request'] >> end
>> Hmm?
>> Matt
>> On Wed, Dec 24, 2008 at 5:35 PM, Yehuda Katz <wyc...@gmail.com> wrote:
>>> Hey guys. I just wanted to post about a problem I've been working through >>> and a potential solution to get some feedback. Effectively, there's a bunch >>> of related code that handles various processing of the Rack environment. >>> However, much of this processing is triggered by requests by the user, and >>> can be expensive. The current recommended approach is to create a middleware >>> that processes the rack environment as needed, and stuff the results back >>> into the environment hash. This works fine for cases where you know the >>> processing will be needed, but not as well for cases where the processing is >>> to be used optionally by a user downstream in the middleware chain. Before >>> rack, the approach both Rails and Merb used was to do that processing in a >>> Request object that could process the environment only when a specific >>> accessor was called. Some examples include "method", a 10-line method in >>> Merb, various parameter parsing (which can be quite expensive), remote_ip >>> (another function that is longer than it seems like is reasonable), and it >>> goes on and on. What is desirable is for the appropriate processing to be >>> done when required, and only once. Whether or not the user is in the >>> Rails/Merb application or Rack middleware should not affect this (the user >>> should have access to any required processing). Unfortunately, since the >>> Rack environment is required to be a "real" Hash, it's not possible to >>> modify the environment to support lazy-loaded and memoized processing. One >>> option we *considered* was a middleware that did something like >>> env["rack.request_obj"] = Rack::Request.new(env). This would provide the >>> benefits of a logical object for the request without breaking Rack >>> conventions. However, after emailing Chris to explain this problem, he >>> suggested an even better (to my eyes) solution: > I think I have a better >>> idea for this... we could make > Rack::Request.new cache the request object >>> in the env itself(!) and > just return it when it was created and calculated >>> already. This would > be backward compatible, efficient, and not expose any >>> details on the > implementation. I love this idea and would love to get some >>> feedback on it. Thanks a ton,
"Yehuda Katz" <wyc...@gmail.com> writes: > I think Chris specifically wanted to override new, as opposed to making a new > builder, for BC-related reasons.
> -- Yehuda
Yep. Also we can move the Request-internal caches (form_vars etc) to instance variables then.
> def Rack::Request.from(env) > unless env['rack.request'] > env['rack.request'] = Rack::Request.new(env) > end > env['rack.request'] > end
> Hmm?
> Matt
> On Wed, Dec 24, 2008 at 5:35 PM, Yehuda Katz <wyc...@gmail.com> wrote:
>> Hey guys. I just wanted to post about a problem I've been working through >> and a potential solution to get some feedback. Effectively, there's a >> bunch >> of related code that handles various processing of the Rack environment. >> However, much of this processing is triggered by requests by the user, and >> can be expensive. The current recommended approach is to create a >> middleware >> that processes the rack environment as needed, and stuff the results back >> into the environment hash. This works fine for cases where you know the >> processing will be needed, but not as well for cases where the processing >> is >> to be used optionally by a user downstream in the middleware chain. Before >> rack, the approach both Rails and Merb used was to do that processing in a >> Request object that could process the environment only when a specific >> accessor was called. Some examples include "method", a 10-line method in >> Merb, various parameter parsing (which can be quite expensive), remote_ip >> (another function that is longer than it seems like is reasonable), and it >> goes on and on. What is desirable is for the appropriate processing to be >> done when required, and only once. Whether or not the user is in the >> Rails/Merb application or Rack middleware should not affect this (the user >> should have access to any required processing). Unfortunately, since the >> Rack environment is required to be a "real" Hash, it's not possible to >> modify the environment to support lazy-loaded and memoized processing. One >> option we *considered* was a middleware that did something like >> env["rack.request_obj"] = Rack::Request.new(env). This would provide the >> benefits of a logical object for the request without breaking Rack >> conventions. However, after emailing Chris to explain this problem, he >> suggested an even better (to my eyes) solution: > I think I have a better >> idea for this... we could make > Rack::Request.new cache the request >> object >> in the env itself(!) and > just return it when it was created and >> calculated >> already. This would > be backward compatible, efficient, and not expose >> any >> details on the > implementation. I love this idea and would love to get >> some >> feedback on it. Thanks a ton,
Yeah, just ignored that for the sake of simplicity. class Rack::Request def self.new(env) unless env['r'] env['r'] = super(env) end env['r'] end def initialize(env); @env = env; end end
I'll work on a patch tonight unless someone beats me to it.
> > On Wed, Dec 24, 2008 at 5:35 PM, Yehuda Katz <wyc...@gmail.com> wrote:
> >> Hey guys. I just wanted to post about a problem I've been working > through > >> and a potential solution to get some feedback. Effectively, there's a > >> bunch > >> of related code that handles various processing of the Rack environment. > >> However, much of this processing is triggered by requests by the user, > and > >> can be expensive. The current recommended approach is to create a > >> middleware > >> that processes the rack environment as needed, and stuff the results > back > >> into the environment hash. This works fine for cases where you know the > >> processing will be needed, but not as well for cases where the > processing > >> is > >> to be used optionally by a user downstream in the middleware chain. > Before > >> rack, the approach both Rails and Merb used was to do that processing in > a > >> Request object that could process the environment only when a specific > >> accessor was called. Some examples include "method", a 10-line method in > >> Merb, various parameter parsing (which can be quite expensive), > remote_ip > >> (another function that is longer than it seems like is reasonable), and > it > >> goes on and on. What is desirable is for the appropriate processing to > be > >> done when required, and only once. Whether or not the user is in the > >> Rails/Merb application or Rack middleware should not affect this (the > user > >> should have access to any required processing). Unfortunately, since the > >> Rack environment is required to be a "real" Hash, it's not possible to > >> modify the environment to support lazy-loaded and memoized processing. > One > >> option we *considered* was a middleware that did something like > >> env["rack.request_obj"] = Rack::Request.new(env). This would provide the > >> benefits of a logical object for the request without breaking Rack > >> conventions. However, after emailing Chris to explain this problem, he > >> suggested an even better (to my eyes) solution: > I think I have a > better > >> idea for this... we could make > Rack::Request.new cache the request > >> object > >> in the env itself(!) and > just return it when it was created and > >> calculated > >> already. This would > be backward compatible, efficient, and not expose > >> any > >> details on the > implementation. I love this idea and would love to get > >> some > >> feedback on it. Thanks a ton,
Implementing now, tests look fine except for the MockRequest test... test_spec {Rack::MockResponse} 002 [should provide access to the HTTP headers](Rack::MockResponse) [./test/spec_rack_mock.rb:130]: <381> with id <763> expected to be equal? to <444> with id <889>.
The problem is that it's explicitly testing for content length of 381 and it's now 444 with the new rack.request reference.
There are two solutions... one, change the test to test against 444 content-length, obviously.
But I don't think this addresses the real problem: he app is calling env.to_yaml and setting it as the body for the mock response.
The other solution could be to define a way to remove rack.request from the collection of headers before calling to_yaml on that hash, but I don't know if this is a legitimate issue that requires this exceptional behavior.
On Wed, Dec 24, 2008 at 6:57 PM, Matt Todd <chiol...@gmail.com> wrote: > Yeah, just ignored that for the sake of simplicity. > class Rack::Request > def self.new(env) > unless env['r'] > env['r'] = super(env) > end > env['r'] > end > def initialize(env); @env = env; end > end
> I'll work on a patch tonight unless someone beats me to it.
> Matt
> On Wed, Dec 24, 2008 at 6:27 PM, Joshua Peek <j...@joshpeek.com> wrote:
>> Tiny detail, override new for better backwords compat and a nicer API I >> think.
>> On 12/24/08, Matt Todd <chiol...@gmail.com> wrote: >> > Effectively...
>> > On Wed, Dec 24, 2008 at 5:35 PM, Yehuda Katz <wyc...@gmail.com> wrote:
>> >> Hey guys. I just wanted to post about a problem I've been working >> through >> >> and a potential solution to get some feedback. Effectively, there's a >> >> bunch >> >> of related code that handles various processing of the Rack >> environment. >> >> However, much of this processing is triggered by requests by the user, >> and >> >> can be expensive. The current recommended approach is to create a >> >> middleware >> >> that processes the rack environment as needed, and stuff the results >> back >> >> into the environment hash. This works fine for cases where you know the >> >> processing will be needed, but not as well for cases where the >> processing >> >> is >> >> to be used optionally by a user downstream in the middleware chain. >> Before >> >> rack, the approach both Rails and Merb used was to do that processing >> in a >> >> Request object that could process the environment only when a specific >> >> accessor was called. Some examples include "method", a 10-line method >> in >> >> Merb, various parameter parsing (which can be quite expensive), >> remote_ip >> >> (another function that is longer than it seems like is reasonable), and >> it >> >> goes on and on. What is desirable is for the appropriate processing to >> be >> >> done when required, and only once. Whether or not the user is in the >> >> Rails/Merb application or Rack middleware should not affect this (the >> user >> >> should have access to any required processing). Unfortunately, since >> the >> >> Rack environment is required to be a "real" Hash, it's not possible to >> >> modify the environment to support lazy-loaded and memoized processing. >> One >> >> option we *considered* was a middleware that did something like >> >> env["rack.request_obj"] = Rack::Request.new(env). This would provide >> the >> >> benefits of a logical object for the request without breaking Rack >> >> conventions. However, after emailing Chris to explain this problem, he >> >> suggested an even better (to my eyes) solution: > I think I have a >> better >> >> idea for this... we could make > Rack::Request.new cache the request >> >> object >> >> in the env itself(!) and > just return it when it was created and >> >> calculated >> >> already. This would > be backward compatible, efficient, and not expose >> >> any >> >> details on the > implementation. I love this idea and would love to get >> >> some >> >> feedback on it. Thanks a ton,
Correction, the to_yaml is called on the environment variable directly on not on the Rack::Request object so it's not to be helped that the content length is slightly longer. I've updated the test with the new length.
Pull request sent via GitHub, refers to the following commits:
This includes: * Rack::Request memoization in environment passed in implementation * Adjustment to MockRequest content-length expected (now 444) * Spec to determine that the Rack::Request is memoized for the same environment
Some concerns/questions: * I didn't take any account into marshalling/loading of the request... should this be a concern at all? * I did not add tests (though I did test it) for unmemoizing the rack.request value... should I add this in?
On Wed, Dec 24, 2008 at 7:22 PM, Matt Todd <chiol...@gmail.com> wrote: > Implementing now, tests look fine except for the MockRequest test... > test_spec {Rack::MockResponse} 002 [should provide access to the HTTP > headers](Rack::MockResponse) [./test/spec_rack_mock.rb:130]: > <381> > with id <763> expected to be equal? to > <444> > with id <889>.
> The problem is that it's explicitly testing for content length of 381 and > it's now 444 with the new rack.request reference.
> There are two solutions... one, change the test to test against 444 > content-length, obviously.
> But I don't think this addresses the real problem: he app is calling > env.to_yaml and setting it as the body for the mock response.
> The other solution could be to define a way to remove rack.request from the > collection of headers before calling to_yaml on that hash, but I don't know > if this is a legitimate issue that requires this exceptional behavior.
> Looking for opinions; share any you have please.
> Matt
> On Wed, Dec 24, 2008 at 6:57 PM, Matt Todd <chiol...@gmail.com> wrote:
>> Yeah, just ignored that for the sake of simplicity. >> class Rack::Request >> def self.new(env) >> unless env['r'] >> env['r'] = super(env) >> end >> env['r'] >> end >> def initialize(env); @env = env; end >> end
>> I'll work on a patch tonight unless someone beats me to it.
>> Matt
>> On Wed, Dec 24, 2008 at 6:27 PM, Joshua Peek <j...@joshpeek.com> wrote:
>>> Tiny detail, override new for better backwords compat and a nicer API I >>> think.
>>> On 12/24/08, Matt Todd <chiol...@gmail.com> wrote: >>> > Effectively...
>>> > On Wed, Dec 24, 2008 at 5:35 PM, Yehuda Katz <wyc...@gmail.com> wrote:
>>> >> Hey guys. I just wanted to post about a problem I've been working >>> through >>> >> and a potential solution to get some feedback. Effectively, there's a >>> >> bunch >>> >> of related code that handles various processing of the Rack >>> environment. >>> >> However, much of this processing is triggered by requests by the user, >>> and >>> >> can be expensive. The current recommended approach is to create a >>> >> middleware >>> >> that processes the rack environment as needed, and stuff the results >>> back >>> >> into the environment hash. This works fine for cases where you know >>> the >>> >> processing will be needed, but not as well for cases where the >>> processing >>> >> is >>> >> to be used optionally by a user downstream in the middleware chain. >>> Before >>> >> rack, the approach both Rails and Merb used was to do that processing >>> in a >>> >> Request object that could process the environment only when a specific >>> >> accessor was called. Some examples include "method", a 10-line method >>> in >>> >> Merb, various parameter parsing (which can be quite expensive), >>> remote_ip >>> >> (another function that is longer than it seems like is reasonable), >>> and it >>> >> goes on and on. What is desirable is for the appropriate processing to >>> be >>> >> done when required, and only once. Whether or not the user is in the >>> >> Rails/Merb application or Rack middleware should not affect this (the >>> user >>> >> should have access to any required processing). Unfortunately, since >>> the >>> >> Rack environment is required to be a "real" Hash, it's not possible to >>> >> modify the environment to support lazy-loaded and memoized processing. >>> One >>> >> option we *considered* was a middleware that did something like >>> >> env["rack.request_obj"] = Rack::Request.new(env). This would provide >>> the >>> >> benefits of a logical object for the request without breaking Rack >>> >> conventions. However, after emailing Chris to explain this problem, he >>> >> suggested an even better (to my eyes) solution: > I think I have a >>> better >>> >> idea for this... we could make > Rack::Request.new cache the request >>> >> object >>> >> in the env itself(!) and > just return it when it was created and >>> >> calculated >>> >> already. This would > be backward compatible, efficient, and not >>> expose >>> >> any >>> >> details on the > implementation. I love this idea and would love to >>> get >>> >> some >>> >> feedback on it. Thanks a ton,
On Wed, Dec 24, 2008 at 6:49 PM, Matt Todd <chiol...@gmail.com> wrote: > Correction, the to_yaml is called on the environment variable directly on > not on the Rack::Request object so it's not to be helped that the content > length is slightly longer. > I've updated the test with the new length. > Pull request sent via GitHub, refers to the following commits: > http://github.com/mtodd/rack/ > eefbed89c4ece749e889132012d0f67cd87926a8 > 3873269bea8d29e3ae4f1f46eb11822f9eae795f > 552f7b0718ee8cd79c185cd72413690f0da72402 > This includes: > * Rack::Request memoization in environment passed in implementation > * Adjustment to MockRequest content-length expected (now 444) > * Spec to determine that the Rack::Request is memoized for the same > environment > Some concerns/questions: > * I didn't take any account into marshalling/loading of the request... > should this be a concern at all? > * I did not add tests (though I did test it) for unmemoizing the > rack.request value... should I add this in? > Matt
> On Wed, Dec 24, 2008 at 7:22 PM, Matt Todd <chiol...@gmail.com> wrote:
>> Implementing now, tests look fine except for the MockRequest test... >> test_spec {Rack::MockResponse} 002 [should provide access to the HTTP >> headers](Rack::MockResponse) [./test/spec_rack_mock.rb:130]: >> <381> >> with id <763> expected to be equal? to >> <444> >> with id <889>. >> The problem is that it's explicitly testing for content length of 381 and >> it's now 444 with the new rack.request reference. >> There are two solutions... one, change the test to test against 444 >> content-length, obviously. >> But I don't think this addresses the real problem: he app is calling >> env.to_yaml and setting it as the body for the mock response. >> The other solution could be to define a way to remove rack.request from >> the collection of headers before calling to_yaml on that hash, but I don't >> know if this is a legitimate issue that requires this exceptional behavior. >> Looking for opinions; share any you have please. >> Matt
>> On Wed, Dec 24, 2008 at 6:57 PM, Matt Todd <chiol...@gmail.com> wrote:
>>> Yeah, just ignored that for the sake of simplicity. >>> class Rack::Request >>> def self.new(env) >>> unless env['r'] >>> env['r'] = super(env) >>> end >>> env['r'] >>> end >>> def initialize(env); @env = env; end >>> end
>>> I'll work on a patch tonight unless someone beats me to it. >>> Matt
>>> On Wed, Dec 24, 2008 at 6:27 PM, Joshua Peek <j...@joshpeek.com> wrote:
>>>> Tiny detail, override new for better backwords compat and a nicer API I >>>> think.
>>>> On 12/24/08, Matt Todd <chiol...@gmail.com> wrote: >>>> > Effectively...
>>>> > On Wed, Dec 24, 2008 at 5:35 PM, Yehuda Katz <wyc...@gmail.com> wrote:
>>>> >> Hey guys. I just wanted to post about a problem I've been working >>>> >> through >>>> >> and a potential solution to get some feedback. Effectively, there's a >>>> >> bunch >>>> >> of related code that handles various processing of the Rack >>>> >> environment. >>>> >> However, much of this processing is triggered by requests by the >>>> >> user, and >>>> >> can be expensive. The current recommended approach is to create a >>>> >> middleware >>>> >> that processes the rack environment as needed, and stuff the results >>>> >> back >>>> >> into the environment hash. This works fine for cases where you know >>>> >> the >>>> >> processing will be needed, but not as well for cases where the >>>> >> processing >>>> >> is >>>> >> to be used optionally by a user downstream in the middleware chain. >>>> >> Before >>>> >> rack, the approach both Rails and Merb used was to do that processing >>>> >> in a >>>> >> Request object that could process the environment only when a >>>> >> specific >>>> >> accessor was called. Some examples include "method", a 10-line method >>>> >> in >>>> >> Merb, various parameter parsing (which can be quite expensive), >>>> >> remote_ip >>>> >> (another function that is longer than it seems like is reasonable), >>>> >> and it >>>> >> goes on and on. What is desirable is for the appropriate processing >>>> >> to be >>>> >> done when required, and only once. Whether or not the user is in the >>>> >> Rails/Merb application or Rack middleware should not affect this (the >>>> >> user >>>> >> should have access to any required processing). Unfortunately, since >>>> >> the >>>> >> Rack environment is required to be a "real" Hash, it's not possible >>>> >> to >>>> >> modify the environment to support lazy-loaded and memoized >>>> >> processing. One >>>> >> option we *considered* was a middleware that did something like >>>> >> env["rack.request_obj"] = Rack::Request.new(env). This would provide >>>> >> the >>>> >> benefits of a logical object for the request without breaking Rack >>>> >> conventions. However, after emailing Chris to explain this problem, >>>> >> he >>>> >> suggested an even better (to my eyes) solution: > I think I have a >>>> >> better >>>> >> idea for this... we could make > Rack::Request.new cache the request >>>> >> object >>>> >> in the env itself(!) and > just return it when it was created and >>>> >> calculated >>>> >> already. This would > be backward compatible, efficient, and not >>>> >> expose >>>> >> any >>>> >> details on the > implementation. I love this idea and would love to >>>> >> get >>>> >> some >>>> >> feedback on it. Thanks a ton,
On Wed, Dec 24, 2008 at 16:49, Matt Todd <chiol...@gmail.com> wrote: > Correction, the to_yaml is called on the environment variable directly on > not on the Rack::Request object so it's not to be helped that the content > length is slightly longer. > I've updated the test with the new length. > Pull request sent via GitHub, refers to the following commits: > http://github.com/mtodd/rack/ > eefbed89c4ece749e889132012d0f67cd87926a8 > 3873269bea8d29e3ae4f1f46eb11822f9eae795f > 552f7b0718ee8cd79c185cd72413690f0da72402 > This includes: > * Rack::Request memoization in environment passed in implementation > * Adjustment to MockRequest content-length expected (now 444) > * Spec to determine that the Rack::Request is memoized for the same > environment > Some concerns/questions: > * I didn't take any account into marshalling/loading of the request... > should this be a concern at all? > * I did not add tests (though I did test it) for unmemoizing the > rack.request value... should I add this in? > Matt
> On Wed, Dec 24, 2008 at 7:22 PM, Matt Todd <chiol...@gmail.com> wrote:
>> Implementing now, tests look fine except for the MockRequest test... >> test_spec {Rack::MockResponse} 002 [should provide access to the HTTP >> headers](Rack::MockResponse) [./test/spec_rack_mock.rb:130]: >> <381> >> with id <763> expected to be equal? to >> <444> >> with id <889>. >> The problem is that it's explicitly testing for content length of 381 and >> it's now 444 with the new rack.request reference. >> There are two solutions... one, change the test to test against 444 >> content-length, obviously. >> But I don't think this addresses the real problem: he app is calling >> env.to_yaml and setting it as the body for the mock response. >> The other solution could be to define a way to remove rack.request from >> the collection of headers before calling to_yaml on that hash, but I don't >> know if this is a legitimate issue that requires this exceptional behavior. >> Looking for opinions; share any you have please. >> Matt
>> On Wed, Dec 24, 2008 at 6:57 PM, Matt Todd <chiol...@gmail.com> wrote:
>>> Yeah, just ignored that for the sake of simplicity. >>> class Rack::Request >>> def self.new(env) >>> unless env['r'] >>> env['r'] = super(env) >>> end >>> env['r'] >>> end >>> def initialize(env); @env = env; end >>> end
>>> I'll work on a patch tonight unless someone beats me to it. >>> Matt
>>> On Wed, Dec 24, 2008 at 6:27 PM, Joshua Peek <j...@joshpeek.com> wrote:
>>>> Tiny detail, override new for better backwords compat and a nicer API I >>>> think.
>>>> On 12/24/08, Matt Todd <chiol...@gmail.com> wrote: >>>> > Effectively...
>>>> > On Wed, Dec 24, 2008 at 5:35 PM, Yehuda Katz <wyc...@gmail.com> wrote:
>>>> >> Hey guys. I just wanted to post about a problem I've been working >>>> >> through >>>> >> and a potential solution to get some feedback. Effectively, there's a >>>> >> bunch >>>> >> of related code that handles various processing of the Rack >>>> >> environment. >>>> >> However, much of this processing is triggered by requests by the >>>> >> user, and >>>> >> can be expensive. The current recommended approach is to create a >>>> >> middleware >>>> >> that processes the rack environment as needed, and stuff the results >>>> >> back >>>> >> into the environment hash. This works fine for cases where you know >>>> >> the >>>> >> processing will be needed, but not as well for cases where the >>>> >> processing >>>> >> is >>>> >> to be used optionally by a user downstream in the middleware chain. >>>> >> Before >>>> >> rack, the approach both Rails and Merb used was to do that processing >>>> >> in a >>>> >> Request object that could process the environment only when a >>>> >> specific >>>> >> accessor was called. Some examples include "method", a 10-line method >>>> >> in >>>> >> Merb, various parameter parsing (which can be quite expensive), >>>> >> remote_ip >>>> >> (another function that is longer than it seems like is reasonable), >>>> >> and it >>>> >> goes on and on. What is desirable is for the appropriate processing >>>> >> to be >>>> >> done when required, and only once. Whether or not the user is in the >>>> >> Rails/Merb application or Rack middleware should not affect this (the >>>> >> user >>>> >> should have access to any required processing). Unfortunately, since >>>> >> the >>>> >> Rack environment is required to be a "real" Hash, it's not possible >>>> >> to >>>> >> modify the environment to support lazy-loaded and memoized >>>> >> processing. One >>>> >> option we *considered* was a middleware that did something like >>>> >> env["rack.request_obj"] = Rack::Request.new(env). This would provide >>>> >> the >>>> >> benefits of a logical object for the request without breaking Rack >>>> >> conventions. However, after emailing Chris to explain this problem, >>>> >> he >>>> >> suggested an even better (to my eyes) solution: > I think I have a >>>> >> better >>>> >> idea for this... we could make > Rack::Request.new cache the request >>>> >> object >>>> >> in the env itself(!) and > just return it when it was created and >>>> >> calculated >>>> >> already. This would > be backward compatible, efficient, and not >>>> >> expose >>>> >> any >>>> >> details on the > implementation. I love this idea and would love to >>>> >> get >>>> >> some >>>> >> feedback on it. Thanks a ton,
"Matt Todd" <chiol...@gmail.com> writes: > Correction, the to_yaml is called on the environment variable directly on not > on the Rack::Request object so it's not to be helped that the content length is > slightly longer.
> I've updated the test with the new length.
Obviously that test "tests too much". Your fix is okay.
Yeah, makes much more sense. I believe (though I don't remember the exact reason) I started that way because I wanted to expand the test to be more than just existence but also of class, et al. That, however, wasn't necessary, or, at least, not yet anyways.
Matt
On Thu, Dec 25, 2008 at 8:52 AM, Christoffer Sawicki <
