Using merge! instead of merge in urlmap to avoid duping the env hash

[Tim Connor]

Merge not only creates an additional unneeded hash, this creation of
said extra hash prevents any earlier called middleware from accessing
anything set by the lower:

http://github.com/rack/rack/issues#issue/18

My fork is stuck in "hardcore forking action" so I just made a patch
and put it here (along with a patch to check for the that map works in
Builder in the first place):

http://github.com/timocratic/rack/downloads

[Konstantin Haase]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

+1

Konstantin
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.14 (Darwin)

iQEcBAEBAgAGBQJL3A01AAoJEM+qkfuqK1IXMzgH/AsCvuXiaBcfz7drMP91EOVY
PRnV4PuezBLpYvWgnropAil7759FudYOrGDNegwyk0WW7UQ9wlQE0g4s4VuFEvRy
wcQmNfvUbUyZv2HQN/lSrGNp7FD85Jd2C7PSHQEPg9yj1r20McCNr3+PKzW9xKYn
+cKxGnf74Z8n58pZJfMbmREi2377BOC+oAWLn+Us8LVL0VWbofFINE2eXsrd973i
3j2GmaqqhvJAqj7Sk+w9Td4gbcCtc4Pw4DGenOWSGKUl1qw0twq9Szp/Oq1KQXB9
5v73oXJ1qTHJIeePSTob3nCPEgCosa+L6/dii12BpW8+1IDsh4gx/hhG9aZ0AqQ=
=CyzQ
-----END PGP SIGNATURE-----

[Eric Wong]

+1 here, too. I'm not sure if it's technically doable, but it'd be nice
to standardize and enforce in Lint. This would make it easier for
various middlewares to talk to each other and coordinate.

--
Eric Wong

[Ryan Tomayko]

+1

This should definitely be added to the SPEC, regardless of the
behavior we settle on.

Ryan

[Gaius]

This was discussed about six months ago. See
http://groups.google.com/group/rack-devel/browse_thread/thread/5d93266373a372ea/4d68e270e43832ea?lnk=gst&q=merge#4d68e270e43832ea

Changing this behavior is likely to break some middleware that were
expecting downstream middleware to communicate only by the [status,
headers, body] response, not the incoming {} request.

Still, I think reusing the object is the correct behavior. It's _very_
difficult to get same-object behavior for those few cases when you
need it, and it's simple to undo it. (Just save env.dup in your
middleware if you don't trust downstream ones.)

+1

On May 1, 7:18 pm, Ryan Tomayko <r...@tomayko.com> wrote:

[Tim Connor]

On Sat, May 1, 2010 at 4:13 PM, Eric Wong <normal...@yhbt.net> wrote:
> +1 here, too.  I'm not sure if it's technically doable, but it'd be nice
> to standardize and enforce in Lint.  This would make it easier for
> various middlewares to talk to each other and coordinate.
>
> --
> Eric Wong
>

Yeah, I thought about that, but it might be prohibitively tricky. I
guess you could override merge for just the singleton of env to fail.
That wouldn't catch any other ways someone might dupe it and pass the
wrong reference, but it's be something? Combined with a line in the
spec saying something to the effect of:

"You SHOULD pass a reference to the same env hash down through the
call chain, so that middleware higher in the call chain can access
things set by lower middleware and end-points. If you dup the env for
local usage in your middleware, pass the original; if you are updating
it, use update or merge! instead of merge."

I think it should probably be a should, not a must?

[Gaius]

Agreed that we shouldn't try to force it too hard. One thing we might
consider is redefining `env.dup` to print out a warning message
("Warning: Rack middlewares SHOULD NOT call `env.dup`") and then carry
on with normal dup-ing behavior.

On May 3, 12:03 pm, Tim Connor <timocra...@gmail.com> wrote:

[Tim Connor]

There are plenty of legitimate uses for dup'ing (filtering the param
list for outputting in error emails, for instance), as long as you
pass the original down. I think a mention in the spec and *maybe*
putting a warning on merge is about all you can really cover.

[Gaius]

That seems pretty fair. I think it's important to warn middleware
developers that they shouldn't be using env.merge, but if people think
that keeping that warning on forever is to annoying, we could do the
following:

1. release Rack 1.2.0 with a warning on env.merge saying, "middlewares
SHOULD NOT use env.merge; this message will disappear in a future
version of Rack"
2. release Rack 1.2.99 without the warning

(version numbers up for debate -- I merely meant them as examples)

On May 4, 11:07 am, Tim Connor <timocra...@gmail.com> wrote:
> There are plenty of legitimate uses for dup'ing (filtering the param
> list for outputting in error emails, for instance), as long as you
> pass the original down.  I think a mention in the spec and *maybe*
> putting a warning on merge is about all you can really cover.
>

[Michael Fellinger]

On Sat, May 1, 2010 at 4:33 PM, Tim Connor <timoc...@gmail.com> wrote:
> Merge not only creates an additional unneeded hash, this creation of
> said extra hash prevents any earlier called middleware from accessing
> anything set by the lower:
>
> http://github.com/rack/rack/issues#issue/18

I'm against the change because it means that you cannot nest URLMap
inside URLMap.
But it seems like I'm in the minority, I'll implement the method in my
own code and amend Rack to use merge!.
Not sure if that's spec-worthy, we can mention it in any case without
enforcing it outside the rack codebase.
The patch will be pushed tomorrow if nobody has any other opinions.

--
Michael Fellinger
CTO, The Rubyists, LLC

[Konstantin Haase]

On Jun 8, 2010, at 20:14 , Michael Fellinger wrote:

> On Sat, May 1, 2010 at 4:33 PM, Tim Connor <timoc...@gmail.com> wrote:
>> Merge not only creates an additional unneeded hash, this creation of
>> said extra hash prevents any earlier called middleware from accessing
>> anything set by the lower:
>>
>> http://github.com/rack/rack/issues#issue/18
>
> I'm against the change because it means that you cannot nest URLMap
> inside URLMap.

I don't get this. Why?

Konstantin

[Tim Connor]

On Tue, Jun 8, 2010 at 11:14 AM, Michael Fellinger <m.fel...@gmail.com> wrote:

On Sat, May 1, 2010 at 4:33 PM, Tim Connor <timoc...@gmail.com> wrote:
> Merge not only creates an additional unneeded hash, this creation of
> said extra hash prevents any earlier called middleware from accessing
> anything set by the lower:
>
> http://github.com/rack/rack/issues#issue/18

I'm against the change because it means that you cannot nest URLMap
inside URLMap.

If there is a good reason why it shouldn't be, then I am against my own patch. I just don't quite get why this is the case.

Maybe if you think it shouldn't be in, then make a test/spec with the behavior you are shooting for, so people like me don't come along wanting to change an undocumented use case? 

[Konstantin Haase]

I just checked, and the test for nesting passes.
Here is the test: http://github.com/rack/rack/blob/master/test/spec_rack_urlmap.rb#L128-148

Konstantin

On Jun 8, 2010, at 20:14 , Michael Fellinger wrote:

[Tim Connor]

On Tue, Jun 8, 2010 at 11:33 AM, Konstantin Haase <k.h...@finn.de> wrote:

I just checked, and the test for nesting passes.
Here is the test: http://github.com/rack/rack/blob/master/test/spec_rack_urlmap.rb#L128-148

I think I see what he is saying.  URLMap intentionally massages the PATH_INFO and SCRIPT_NAME so you could end up stepping on each others SCRIPT_NAME, if you use destructive merge? Maybe, when I get a chance I'll play with some fleshing out the nesting tests to make sure that is covered in them (unless he gets to it first)

Tim 

[Konstantin Haase]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

SCRIPT_NAME is appended when nesting and only modified if an appropriate app is found.

Konstantin
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.14 (Darwin)

iQEcBAEBAgAGBQJMDpCnAAoJEM+qkfuqK1IXVHoIANk4n7EE0rB/3JkePO1eGNer
DTQRtrBWtj8KU8FYA4Cpu8r/EiSES/8LqkbGGIMxCh8SQK+xSo+ntq1/zws4IqzJ
wy9kRufJNovhgu7tHIUaRCR5zI0KdPii1030S/upBHlEWmUauR9ZDyQ1mA8WNmZg
Hx935a+iHQYmIFZ6UXtincrfu83QEO7f4xIY2YvzonNZaD6iRMmufZHvMX/8o6ag
lyYnwYZlmqNVtqZNADD5CnMOD8tbv8J3LOB54PDA+3trCqWrzqrnBGACV/jBhId8
9s9jGxE5REkb/mjLTjLPbj+shOdVxxYj5OSrgO0aF7tCawuBSbxzmW015qEgJeU=
=AU5v
-----END PGP SIGNATURE-----

[Konstantin Haase]

On Jun 8, 2010, at 20:44 , Tim Connor wrote:

I think a somewhat related issue, especially when using a cascade, is that the values don't get reseted.
This should fix the issue: http://github.com/rkh/rack/commit/47506242f26bb9642766bb35048cc8d4da6ddaeb

Konstantin

[Christian Neukirchen]

I think this could introduce a lot of hard to find bugs, but I cannot prove it.

In doubt, let's try it on HEAD.

On Jun 8, 2010 8:15 PM, "Michael Fellinger" <m.fel...@gmail.com> wrote:

On Sat, May 1, 2010 at 4:33 PM, Tim Connor <timoc...@gmail.com> wrote:

> Merge not only creates an...

[Michael Fellinger]

Good, I think this would work in the majority of cases.
Of course, it will still cause headaches for someone, but it solves my
issues and doesn't use merge.

[Michael Fellinger]

> Good, I think this would work in the majority of cases.
> Of course, it will still cause headaches for someone, but it solves my
> issues and doesn't use merge.

I committed the patches I found in rkh/rack, thanks to Tim and Konstantin.
The first parent is at:
http://github.com/rack/rack/commit/09779a2a4965886b5814231074

[Tim Connor]

On Tue, Jun 8, 2010 at 9:06 PM, Michael Fellinger <m.fel...@gmail.com> wrote:

I committed the patches I found in rkh/rack, thanks to Tim and Konstantin.
The first parent is at:
http://github.com/rack/rack/commit/09779a2a4965886b5814231074

Thanks Michael.  I closed the ticket with a link point at the changeset