Discussion:
[Mason-devel] [Fwd: [Mason] patch for task 294: support exec/subexec("comp:method")]
Zach Welch
2006-05-21 15:24:02 UTC
Permalink
Howdy,

I suppose it's too late for 1.33, but I posted this patch and never saw
any reply to it. I am using it in (light) production, so I would at
least like to hear a reply as to why it was not considered. I suppose
"because you posted it to the user's list" is valid, too.

Cheers,

Zach


-------- Original Message --------
Subject: [Mason] patch for task 294: support exec/subexec("comp:method")
Date: Mon, 03 Apr 2006 19:55:43 -0700
From: Zach Welch <zach-***@splitstring.com>
Organization: The Split String Experience
To: mason-***@lists.sourceforge.net

Hi all,

I've been using Mason for five years or so, primarily using it with
mod_perl to develop dynamic websites. Many thanks to all that continue
to contribute to its success; I look forward to the continued support
and growth that the community can bring. That said, the patch mentioned
in the subject line itself deserves a far better introduction.


For a couple of days last week, I time working on the problem of making
the URLs for a web site to be compliant with REST principles (as
outlined in O'Reilly's "Web Services with Perl"). As a result of this
effort, I have developed a series of independent "areas" that contain
dhandlers for handling any immediately subsequent variable portions of
the request path.

After a dhandler consumes its parameters (present in the first part(s)
of its dhandler_arg path), it uses a $m->subexec call to trigger another
"area request", first building up menus and other output through its
chain of autohandlers. This process can be repeated by additional
dhandler/subexec requests, as necessary. Using this structure, dynamic
paths that follow REST principles can be processed sanely using Mason.

At first glance, these changes seemed to be a major win for the site,
but the tricky bit came when I began to update the mechanisms for
retrieving the page title, required authorization levels, and other
dynamic attribute data from these deeply nested components. After some
hacking, I realized that the ideal solution was to add support for
"comp:method" syntax to the core request subexec code path.

Specifically, the subrequest needed to establish proper inheritance for
the requested component (possibly after locating a dhandler), locate the
correct component method (which may have been inherited), and then call
it directly - without any autohandlers. [As often turns out to be the
case, defining my actual problems and the required solutions took orders
of magnitude longer than implementing the proper improvements.]


To start, I made a couple of changes to the initialization method in
HTML::Mason::Request, establishing the proper request_comp for a method
(if specified). Later in the same module, the exec method disables the
wrapping chain for direct requests on subcomponents. In this regard, a
exec/subexec of a method behaves very similarly to a normal "comp" call
(which conforms to my personal intuitive expectations). Together, this
extended the system to allow the direct call of methods located in a
subrequest dhandler (with proper setup of its dhandler_arg path).

After finishing my work, I found task 294 on the MasonHQ wiki, and I
think these specific changes mostly satisfy its (now somewhat dated)
requirements. Since that task has been left open, I assume that this
patch will be welcomed into the main tree, so I am posting it here.
(Actually, I remember seeing that issue a couple of years ago and
somehow worked around the limitation; this time, I put my foot down.)

The changes are as simple and clean as I could make them; for example, I
didn't properly reindent the wrapping chain code block, in order to keep
the patch short and clear. The existing test suite does not show any
obvious regressions, but I have not looked at extending it properly to
cover this new capability. Because of the new permutations of support
that it brings to the exec call, this may not be as easy as the patch.


In theory, this should have broken a previously existing test built to
cover requests which directly specify a subcomponent object; the second
half of my changes break the inheritance chain for such calls. If it
shouldn't have changed in this particular way, the different conditions
could be identified and the behavior regressed, but that could result in
two different outputs for the same subcomponent (depending on whether
the component was called by object or by path). Of course, there is
already an established way way to disable the wrapping chain, but I
don't think it currently applies to subcomponents (for good reason)....

To be fully backward compatible while retaining all of the new features,
I suppose that the proper solution would be to add support to methods to
respect the flags block (and its 'inherit' attribute); however, this
kind of change would go far beyond my current comfort level for hacking
on the Mason code. Since this seems to be the most idiomatic solution,
I would be interested to hear the pros and cons of this approach from
the experts. Personally, the thought of the possible class of site bugs
this could create makes my brain hurt.


Since this feature has significantly helped my site, I'd be interested
in seeing what it takes to have this patch included in a future release.
For obvious reasons, I'm reluctant to update my production site with a
patched version of a core module like this, so I need to start thinking
about my release schedule versus Mason's (versus deploying the patch).

I look forward to any response.

Cheers,

Zach Welch
Superlucidity Services
Dave Rolsky
2006-05-22 01:29:24 UTC
Permalink
Post by Zach Welch
I suppose it's too late for 1.33, but I posted this patch and never saw
any reply to it. I am using it in (light) production, so I would at
least like to hear a reply as to why it was not considered. I suppose
"because you posted it to the user's list" is valid, too.
Actually, the reasons are probably more like:

A) It's a long message, and I'm lazy ;)
B) The patch has no tests or docs, which means incorporating it is more
work for me.

But looking at it more closely, I suspect it's not something we want to
do, because then someone could call component methods directly via a URI.
While we've never said so in the docs, I think the implication has always
been that these should not be exposed via the web, unless someone wanted
to really go to the trouble of writing code that lets them do that.


-dave

/*===================================================
VegGuide.Org www.BookIRead.com
Your guide to all that's veg. My book blog
===================================================*/
Zach Welch
2006-05-22 02:51:01 UTC
Permalink
Post by Dave Rolsky
Post by Zach Welch
I suppose it's too late for 1.33, but I posted this patch and never
saw any reply to it. I am using it in (light) production, so I would
at least like to hear a reply as to why it was not considered. I
suppose "because you posted it to the user's list" is valid, too.
A) It's a long message, and I'm lazy ;)
Hrmph. You are not alone with that disposition. :/ I'm fairly convinced
that this shows there can be no such thing as the perfect bug report.

If a post too short, one skips it for lacking sufficient details; if it
is too long, no one ever reads it in the first place. Further, it seems
that trimming a long message ends up making it too short, and adding
details to a short message makes it too long. If it is even possible, I
just can't find a way to win. :)
Post by Dave Rolsky
B) The patch has no tests or docs, which means incorporating it is more
work for me.
I tried to imply these deficiencies, but I can fix them - with feedback.
My writing should show that adequate documentation will not be a
problem, once the system reaches a state where it justifies such work.
The tests also require the perspective that your feedback is giving me.
Post by Dave Rolsky
But looking at it more closely, I suspect it's not something we want to
do, because then someone could call component methods directly via a
URI. While we've never said so in the docs, I think the implication has
always been that these should not be exposed via the web, unless someone
wanted to really go to the trouble of writing code that lets them do that.
Eeek. :) It took me a few tries, but yeah, this is bad. This is why I
sent it to the list for review; I thought there might be stuff I missed.
Well, I am glad that I have held off deploying it publicly.

This might be avoided by adding an check in the proper exec method, if I
recall correctly. In other words, only the top-level (i.e. exec) request
would need to be checked for this pre-condition. Really, I only need the
subexec, so I completely agree that an additional fix will be required.

I am ready to do what it takes to get this included, so give me the
proper direction and I'll get it done to your specification. Still, I
know we're now talking 1.34, so I guess you're off the hook for now. ;)
I'll try to come up with a patch myself, but you may have further
insight that can make my life a little easier.

Cheers,

Zach
Alex Robinson
2006-05-22 10:08:11 UTC
Permalink
I am ready to do what it takes to get this included...
Why not subclass Mason instead? Stick it on CPAN under MasonX and if
people start clamouring for the features you provide to be rolled
into the core functionality, that's the point at which to talk about
including it.

Indeed, why include your new, largely untested thingamijob, instead
of any of the pre-existing MasonX modules, or indeed plugins?
Zach Welch
2006-05-22 18:45:02 UTC
Permalink
Post by Alex Robinson
I am ready to do what it takes to get this included...
Why not subclass Mason instead? Stick it on CPAN under MasonX and if
people start clamouring for the features you provide to be rolled into
the core functionality, that's the point at which to talk about
including it.
Indeed, why include your new, largely untested thingamijob, instead of
any of the pre-existing MasonX modules, or indeed plugins?
I would have subclassed, but the existing implementation is not simply
designed for that purpose. The function is fairly monolithic and would
require large chunks of copied code. That's just not correct.

If I factor out those chunks of code that I patched into new helper
functions, then yes, I could subclass and be happy. Such changes would
be more disruptive than what I have proposed, but this would be an
acceptable solution for me.

Moreover, I would have assumed that a refactoring would likely be
rejected without this broader perspective of its applicability. Besides,
I am still concerned about fundamental correctness. Until I know that
this kind of thing is even possible to do correctly, there is absolutely
no point in taking the time to munge the patch into a form that will be
merged.

Further. this was listed as TODO on the main mason wiki; if that feature
is not meant to be included in the main package, then it probably should
not be listed there. It gives people like me the wrong impression that
such a contribution would be welcome. If this same feature had not
already been listed there, I would not have even contacted the list.

So that's "why not". ;)

Cheers,

Zach
Alex Robinson
2006-05-22 19:14:16 UTC
Permalink
Post by Zach Welch
Further. this was listed as TODO on the main mason wiki;
...
Post by Zach Welch
So that's "why not". ;)
Er, yes, my bad. Despite the clue "hidden" in the subject line and
your explicit reference to it in your first post I allowed my
personal feelings on the matter to make Dave's comment

"But looking at it more closely, I suspect it's not something we want
to do, because then someone could call component methods directly via
a URI."

obscure all else.
Post by Zach Welch
if that feature
Post by Zach Welch
is not meant to be included in the main package, then it probably should
not be listed there.
I think you have nailed this on the head here
Dave Rolsky
2006-05-22 19:34:02 UTC
Permalink
Post by Zach Welch
I would have subclassed, but the existing implementation is not simply
designed for that purpose. The function is fairly monolithic and would
require large chunks of copied code. That's just not correct.
Well, it's not like we _want_ the code to suck, it just sometimes gets
that way ;)

I do agree that it's monolithic.
Post by Zach Welch
If I factor out those chunks of code that I patched into new helper
functions, then yes, I could subclass and be happy. Such changes would
be more disruptive than what I have proposed, but this would be an
acceptable solution for me.
A patch along these lines (that passes all the existing tests) would be
very cool.
Post by Zach Welch
Moreover, I would have assumed that a refactoring would likely be
rejected without this broader perspective of its applicability. Besides,
Not at all.
Post by Zach Welch
Further. this was listed as TODO on the main mason wiki; if that feature
Err, I'll let Jon answer to that, since he created it.

It seems from the standpoint of "why not, I can do it via a comp call" and
wrong from a security perspective.

At the very least, it probably needs to be something that can be turned on
or off, and we need to make sure it _can't_ be done by default via
the ApacheHandler or CGIHandler code, simply because we don't want folks
to install Mason 1.34 and introduce a security hole to an existing site.

Jon, do you remember what motivated that todo item? I think it'd be good
to update it based on this discussion.


-dave

/*===================================================
VegGuide.Org www.BookIRead.com
Your guide to all that's veg. My book blog
===================================================*/

Loading...