|
Did I miss something, or is the patch incomplete?
+ if (forwardPath.startsWith(Globals.FORWARD_PREFIX_ACTION)) { + ActionConfig config = moduleConfig.findActionConfig(forwardPath); Shouldn't there be code in ModuleConfig to handle the modified forwardPath? Created an attachment (id=16984)
action protocol only Per comment #2: My fault. I caught this yesterday but couldn't get a corrected
patch back in time. See the new attachment in #3. For the time being, I am removing the "forward:" prefix because it raises the question of an indefinite number of forward linking. I haven't figured out how that should be accomplished. Hubert, I would like to discuss with you this aspect. Also I think it might be a good idea to factor out getActionMappingPath and getActionMappingName. They are similar (not identical) versions taken from the Tag library project. (In reply to comment #0)
> Many times I need to redirect to another Struts actions. Recently I've come > across the problem of needing to rename my action paths but this became a real > nightmare to keep the action names as well as my redirect (forward) paths in > synchronization. > I'm uncomfortable with this proposal for the reasons Martin is (see also comments on the dev mailing list thread about this issue), but also for some others. * You are introducing a syntax that creates two unique identifiers for an action ... id and path. That seems like an unnecessary redundancy. * You are overloading the "path" element inside the forward element to mean multiple things, and requiring a namespace escape to disambiguate things. Besides being more complicated to understand, this will break tools that understand the current semantics of Struts configuration files so badly that 1.3 users would need to wait longer than otherwise necessary to get support for 1.3. * The most important reason I dislike this change, however, is an architectural desgn issue that few in the Struts community seem to appreciate ... an ActionForward should represent a *logical outcome* of an Action, not a *menu choice*. Let me explain further. Consider the little search box (with a "Go" button) that we like to put on pages of our applications, to allow users to do text based searching on things. Now, consider that you are authoring the Struts action that actually performs the search. The author of this code should require *no* knowledge of where in the application they wil be sent after the search is completed -- his/her only job is to say "what happened" when the search took place. Logically, there are three interesting outcomes we might want to describe: * No results at all were found -- outcome "none". * Exactly one result was found -- outcome "single". * More than one result was found -- outcome "multiple". I would argue that the search Action should return these three results as three separate logical outcomes. It is up to the application architect to decide to send all three outcomes to the "here's the list of responses" table page. It's also up to the application architect (perhaps later, in response to user feedback) to say "let's do this instead": * If there's no results, go to a page that says "sorry, no results were found, please try your search again." * If there's exactly one response, go to the details page to display the corresponding data. * If there's more than one response, go to the list page (as per the previous behavior). Note that *nothing* in the forwards defined for the search action, or the code inside it, is affected by this decision. Indeed, the outcomes returned by an action are *much* more stable than the places that you (as the application architect) mght want to send the user next. The only time you have to change them is when you add new interesting outcomes that need to be accomodated in the navigation architecture of your app. I agree with you that the names of forwards and the paths they go to can be fragile at times ... but I submit that this is primarily the result of how we are using them. Adding yet another identifier doesn't help, because now you've got *two* fragile identifiers for the same concept. If you think in terms of logical outcomes, though, you'll find this problem to be much less serious. Simply have each Action document the logical outcomes it returns, with meaningful nouns to describe them, and let the architect stitch together the navigation as the struts-config.xml file is composed. (Even if it is *you* playing both roles -- developer and archtect -- you will find that this change of viewpoint really helps the long term maintainability of your application's navigation architecture.) PS: If you ever hear me talk or write about JSF navgation, you'll hear exactly the same thing ... the strings returned by action methods should be *outcomes*, not *destinations*. Craig, I don't think you understood my suggestion. Did you view my patch at all?
If you haven't seen the patch, it will generate the action path when you reference an action in a forward. I am not taking away logical outcomes from an action - all I am doing is enhanching the logical outcomes so that they can automatically build the path to the actions they want to invoke. You would still forward to "success" (or whatever) and the "success" outcome could then build the URI to the correct action. I unfortunately have to press on with my disagreement. How can you co-develop Shale which relies on Spring but say, in so many words, that referencing is not good for Struts? Look also at WebWork which actually assigns aliases to their actions: <action name="LoginAction" alias="loginJSP">...</action> I really think there is *added value* to allow forwards to reference action idenifiers. There's nothing wrong with referencing other actions, and it *is* because URL's are fragile and sometimes complex. I don't know about you, but when I have a project with alot of redirecting, it becomes a big pain to maintain. With that said, I would like to find a compromise here. Perhaps "action:" is not an acceptable nomenclature to you, but what is? Is it another pattern or new attributes? Perhaps we can think outside my proposed solution to come up with one that we all like. Here's a new suggestion based on WebWork: <action alias="viewJournal" path="/journal/view" ...>...</action> <action alias="saveJournal" path="/journal/save" ...> <forward name="success" action-ref="viewJournal" /> </action> This is actually an SAF1 issue, so I'm going to move it over.
I have some preliminary code ready.
I added an alias attribute to ActionConfig. (Whether to make alias a top level item in 1.3 or 1.4 can be decided later.) This gives every mapping an internal identifier from which the action can be referenced. Aliases must be alphanumerics, are not inheritable, and must be unique to a module. <action path="/journal/view" ...> <set-property name="alias" value="viewJournal" /> </action> So now I have some questions I'd like our team to consider: [1] I belive <html:link> should accept aliases in the action attribute. Right now "action" must begin with a /, but if it does not, we should treat it as an alias and lookup the action path. [2] How should action forwards be handled? I am thinking for Action.findForward: if the forward path does not begin with a slash, take the name before any question mark, and convert it to to the action path before returning the ActionForward. My goal is not to add any new attributes except on the action mapping. With the alias being a transparent mapping mechanism that happens at runtime. Why is this important? To give links and forwards (and maybe even forms) an internal consistency. For me URIs change often during development, but their internal symbolism does not. I know that I may have a CustomerEdit link, but what the URI is does not really matter. I believe this kind of addition can bring greater cohesion between the XML, Java, and JSP. Attached is my patch.
Discussion of patch: http://mail-archives.apache.org/mod_mbox/struts-dev/200608.mbox/%3c44EBE21C.30000@apache.org%3e 1) Added actionId attribute to action mapping (ActionConfig) 2) Modified PerformInclude and PerformForward 3) Added method to find ActionConfig by actionId in ModuleConfig 4) Added actionId -> URL resolution to RequestUtils 5) Backported for old RequestProcessor Attached is a sample Struts Config that I used to make sure everything is working.
I created a separate module in the examples app and installed this. Just place an index.jsp in the module jsp folder and give the actionId "welcomeAction" to the /welcome action in the ROOT module. To test the old RequestProcessor, uncomment the controller block. After many moons, my way of accomplishing this addresses Craig's two main objections:
1) I proposed forwards being pointed to other forwards. I agree, that's a bad decision :-) I don't even know how I proposed it because it didn't seem workable anyway. 2) The id attribute is a W3C proposal/standard that specifies a *document*-wide unique identifier. We don't need something that global. So Ted and Martin have recommended the actionId attribute which provides uniqueness to the action mappings. What's left is still the action aliasing I really require. It's just too darn tough to manage links in XML and JSP and I'd rather refer to actions via a symbolic name. This patch only addresses the resolving of the id-based URLs. If it is accepted, I will then submit the html taglib patches which I have ready. One thing I forgot was how I used the "action:" prefix to specifically identify in a forward that it is not a straight URL, but an aliased URL. That's still open for suggestion. I didn't do that in my patch -- /person/edit.do or editPersonAction -- and so if the prefix notation helps, please raise that issue. Related commit: http://svn.apache.org/viewvc?rev=437226&view=rev
> To employ uniqueness checking of an actionId, I added to
> ModuleConfigImpl.addActionConfig. That method currently overwrites any > action configs that are duplicate -- the last one is the winner. Should > I take the same approach for duplicate ids? Or should I throw an > exception on duplication? I'd like some guidance here because I don't > know which makes more sense; I tend towards throwing an error (duplicate > ids smell), but history allows the last one win. Whatever we do, it should be consistent with what is done with other configuration elements. If the actionid throws an exception, then the path should throw an exception too. -Ted. The current code just logs when an action config is going to be overwritten:
if (actionConfigs.containsKey(key)) { log.warn("Overriding ActionConfig of path " + key); } I have two problems with this. First, this condition should be an error because you cannot map a path to the multiple actions. When this occurs today, one loses out, but why should one lose? It sounds like a configuration error that should stop Struts from loading. Second, it's a warning. This will do no good if the logger is set to ERROR level. If nothing changes, then we should also warn for a duplicate id. But I'd like to see both conditions throw an exception. Would you? URL: http://svn.apache.org/viewvc?rev=439533&view=rev
Log: STR-2864: FormTag to recognize actionId, plus any tags that can build URLs from an action Modified: struts/struts1/trunk/taglib/src/main/java/org/apache/struts/taglib/TagUtils.java struts/struts1/trunk/taglib/src/main/java/org/apache/struts/taglib/html/FormTag.java Author: pbenedict
Date: Sun Sep 24 23:01:35 2006 New Revision: 449580 URL: http://svn.apache.org/viewvc?view=rev&rev=449580 Log: STR-2864: demonstrate actionId configuration and using the tag libraries Author: pbenedict
Date: Fri Nov 17 22:39:29 2006 New Revision: 476456 URL: http://svn.apache.org/viewvc?view=rev&rev=476456 Log: STR-2864: Disallow forward slashes in actionId |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
New Feature
Closed
Major
Add actionId attribute to action mapping
Proposed Implementation
I decided to take a crack at my own idea (and this is my first time submitting
a patch!) I did this with the current nightlies of Struts 1.3 and I had a
successful time of hammering out the code. I tested it locally and it works
pretty good, and there are a few additional suggestions I even have in mind.
Basically this patch allows forwards to be pointers to other actions or
forwards. I took a page out of the Spring handbook and used the protocol
notation to be a shorthand to indicate the type of forward. Forwards beginning
with "action:" reference an action mapping of the specified id; forwards
beginning with "forward:" reference another forward (either locally or
globally).
I find this an exciting patch because I've continuously come across these
cases:
[1] Most global forwards are nothing but redudant URIs to existing action
mappings. Instead of mapping them out twice or more (especially when they are
locall), just reference the id of the action. This in a sense can reduce -- or
even eliminate -- the need of global forwards.
[2] Sometimes it is too costly to go into the Java code and change the name of
an existing forward. Here I simply redirect the forward to another forward, as
an alias.
Here is an example:
<action-mappings>
<action id="home" path="/home" forward="/home.jsp" />
<action id="viewCatalog" path="/viewCatalog" forward="/catalog.jsp" />
<action id="addItem" path="/addItem"...>
<forward name="success" path="action:viewCatalog" />
</action>
<action id="confirmPurchase" path="/buy"...>
<forward name="success" path="action:home" redirect="true" />
</action>
</action-mappings>
Notice the new "id" attribute on the action. It's not really new -- it exists
in all the DTD mappings as an attribute -- but no one implemented its datapoint
in the Config modules. So I use that as an identifier.
I am dedicated to owning the time to bring out this solution for 1.3, if the
commiters think it's a good idea. Please give it a try.