struts-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Apache Wiki <wikidi...@apache.org>
Subject [Struts Wiki] Update of "RoughSpots" by Bob Lee
Date Wed, 19 Apr 2006 18:15:27 GMT
Dear Wiki user,

You have subscribed to a wiki page or wiki category on "Struts Wiki" for change notification.

The following page has been changed by Bob Lee:
http://wiki.apache.org/struts/RoughSpots

The comment on the change is:
Responses to Jason's comments.

------------------------------------------------------------------------------
  Some things that could be addresssed before Action 2.0.0. (If we don't address them, we'll
be stuck supporting them throughout all eternity or until Struts 3.0.0, whichever comes first.
;))
+ 
+ We have a small number of existing WebWork users compared to the number of Struts users
we'll (hopefully) eventually have. This is a new framework (if only in name) and a major release.
This is our one chance to break compatibility. We must get it right now, because we will *not*
be able to fix most of these problems later (the cost of breaking existing Struts users will
almost always outweigh the value of the fix).
+ 
+ We do not need to expose Struts users to XWork; they do not care. At the very least we should
build a thin abstraction layer to isolate users from XWork. XWork should be an implementation
detail not part of the SAF API. We can make most of the following changes in SAF's abstraction
layer and avoid breaking existing XWork users. 
  
    1. Looking up a `ResultConfig` should be a one-liner. Right now we have to: {{{
  ActionConfig config = invocation.getProxy().getConfig();
@@ -14, +18 @@

    1. We don't really need the `Action` interface anymore. Should we get rid of it? It has
constant fields for result names. Should we move these to a class named `ResultNames` and
encourage users to static import them as needed?
  
      * [jcarreira] I'm not sure about this... The Action interface is kind of just a marker
interface, but at least it gives us SOMETHING to point users to
+     * [crazybob] I'll buy that. We do need to move the constants out and encourage users
to use static import (Effective Java Item 17).
  
    1. Only put classes in root package that most users need to know about. For example, most
don't need to know about `Default*` or `ObjectFactory`.
  
@@ -24, +29 @@

    1. Specify `Interceptor` lifecycle. Right now if we apply an interceptor to a single action,
we get a new instance every time. If we define an interceptor in a stack, the same instance
gets reused.
  
      * [jcarreira] A new instance per action configuration, right? Not per-invocation...

+     * [crazybob] Last I tested it was per invocation (I remember because it surprised me).
This is actually a non-issue. We'll create a custom `ConfigurationProvider` for Struts which
won't have this problem.
  
    1. Get rid of `AroundInterceptor`. Having `before()` and `after()` methods doesn't make
things simpler. It reduces flexibility. We can't return a different result. You can't handle
exceptions cleanly. The actual interceptor class doesn't appear in the stack trace (we see
`AroundInterceptor` over and over).
  
      * [jcarreira] The idea was that people would forget to do invocation.invoke() and be
confused... Easier for users just to implement a before() method when that's all they need.
I agree on the stack traces though.
+     * [crazybob] It's kind of hard to forget to call `invocation.invoke()`; you have to
return something. ;) Interceptors are already an "expert" feature anyway.
  
    1. Try to get rid of thread locals: `ActionContext` and `ServletActionContext`. At least
make them package-private. Sometimes interceptors need access to the servlet API. In this
case, they should implement a servlet-aware interceptor interface. For example: {{{
  class MyInterceptor implements HttpInterceptor {
@@ -39, +46 @@

  }}}
  
      * [jcarreira] These 2 are orthogonal... Getting rid of ThreadLocals is problematic.
I think we'd end up breaking 90% of old WebWork apps if we did, and it's still not clear that
everything could be covered if we did... I like the idea though, and Patrick and I really
wanted to do this out of the gate, but backwards compatibility with WebWork 1.x at a macro-level
made us think otherwise...
+     * [crazybob] Interceptors need access to the servlet API. They shouldn't have to call
a `ThreadLocal` if we can avoid it and they shouldn't need to cast. We shouldn't worry about
breaking old WebWork apps (see new opening paragraphs). Let's get it right the first time
around because we will not be able to fix it later. 
  
    1. Is `ValidationAware` a good name? Perhaps `Errors` or `ErrorList` would be a better
name.
  
    1. Merge `ActionContext` and `ActionProxy` into `ActionInvocation` (at least from the
users' perspective). Better specify what happens during chaining/action tags.
  
      * [jcarreira] It __is__ well specified... There are some things that the ActionProxy
/ ActionInvocation let you do that a merged one doesn't... for instance easily knowing when
you're done :-)
+     * [crazybob] Does "specified" == "documented"? Can you elaborate on "easily knowing
when you're done" and why we can't address that use case with one interface? We should expose
the user to one interface: `Invocation`. We can have as many objects as we like when it comes
to the internal implementation.
  
    1. Should `ActionInvocation.getResult()` recurse over chain results? Maybe we should have
two methods? `getResult()` and `getFinalResult()`. Is there a good use case for this?
  
      * [jcarreira] See the TokenSessionInterceptor and the stuff it does to re-render the
same result if you post the form more than once. That was the reason for the complexity in
finding the result to execute. It's a nice feature, but I agree it makes the code harder to
read.
+     * [crazybob] We should move this logic to TokenSessionInterceptor until we need it somewhere
else. TokenSessionInterceptor can access the functionality using package-private access if
need be so we don't have to expose it through the published API.
  
    1. `ActionInvocation.invokeActionOnly()`. Does this need to be public? Sounds dangerous.
  
      * [jcarreira] Not sure... This may be part of the same TokenSession stuff... can't remember
exactly.
+     * [crazybob] See above.
  
    1. Eliminate non-private fields. Protected fields in `ActionConfig` for example.
  
      * [jcarreira] We don't want to allow for extension?
+     * [crazybob] Extension through interfaces and methods? Yes. Public/protected fields?
Absolutely not!
  
    1. Rename `ActionInvocation` to `Invocation` or `Request`. Shorter is better.
  
      * [jcarreira] Most users don't see these... Let's not change names on a whim, since
it will be more work for the power users who already use them.
+     * [crazybob] We can make the change in our abstraction layer and not impact existing
XWork users. This is our one chance to get this stuff right.
  
    1. Is `TextProvider` a good name? The JDK refers to these as "messages" everywhere.
  
    1. Come up with a clean way to separate "view" actions from "update" actions. For example,
we might have `view()` and `update()` methods in the same action class. Right now XWork has
`MethodFilterInterceptor`, but that's not a very clean solution. Do we want validation or
the `DefaultWorkflowInterceptor` to run for the `view()` method? One solution is separate
interceptor stacks, but it would be nice if there were some first class support for this.
We could flag action invocations as "view" or "update" (using an enum). We could automatically
choose a mode based on whether the request is an HTTP GET or POST. Or we could set the mode
based on an annotation on the action method. Or some other way... 
  
      * [jcarreira] This is where I think the power of annotations can be great for us...
If we had some common annotations like @view, @edit, etc. then we could just let users map
configurations to those stereotypes (to use an UML-ism) and reduce configuration quite a bit.
Maybe if we just had a generic @Action annotation the stereotype could be a String parameter
so we don't limit them to the ones we pre-define...    
- 
+       * [crazybob] I'd prefer to avoid arbitrary strings when possible. Use the annotation
class itself as the "stereotype" identifier. Apply a @Stereotype annotation to these annotation
classes to mark them as such.
+  
-     * MJ: Using GET for render and POST for submit works well unless you want to trigger
event with a link. Also, these links might help: DataEntryForm, EventActionDispatcher
+     * [MJ] Using GET for render and POST for submit works well unless you want to trigger
event with a link. Also, these links might help: DataEntryForm, EventActionDispatcher
-     * crazybob: Triggering an event should still be a POST (though the framework should
make it easy). From the HTTP spec.: "GET and HEAD methods SHOULD NOT have the significance
of taking an action other than retrieval."
+       * [crazybob] Triggering an event should still be a POST (though the framework should
make it easy). From the HTTP spec.: "GET and HEAD methods SHOULD NOT have the significance
of taking an action other than retrieval."
- 
-     * [jcarreira] I think it's great that you want to take the HTTP spec at its word...
most users don't care though.
+       * [jcarreira] I think it's great that you want to take the HTTP spec at its word...
most users don't care though.
+       * [crazybob] I'm not arguing semantics. There are real security implications to using
GET when you should use POST, not to mention products like Google Web Accelerator will reak
havok on your site. As framework developers, we should make using POST as easy as GET for
users. To not help users do the right thing in this situation would be irresponsible, and
not in the letter of the law sense.
  
    1. On the OGNL value stack `#request` refers to request attributes and `#parameters` refers
to the parameters. We could rename these `#request` for request parameters and `#requestAttributes`
for request attributes.
  
@@ -82, +96 @@

    1. Add better support for file uploads.
  
      * [jcarreira] Anything specific? We're using them at work and they work well... Maybe
we could pull out the file upload progress bar with DWR thing that we've got here...
+     * [crazybob] We have an `UploadedFile` value object which has properties for the `File`
object, the file name in the form, the content type string, and the name specified by the
user. An interceptor passes that object to a setter on our action and then deletes the file
at the end of the request.
  
    1. Don't eat/wrap exceptions. Throw them through to the container. Don't eat exceptions
that occur in getters.
  
    1. Modify `ParametersInterceptor` to sort parameter names by depth (using bucket sort)
and then map them in that order (shallowest first), so we can create objects and then map
fields to those objects in the same action invocation without hacks like applying the `ParametersInterceptor`
twice or chaining.
  
      * [jcarreira] I'm not sure that's useful... We discussed it at some length on the mailing
list and it wasn't clear. mapping the param interceptor twice isn't for that problem, though,
it's for model-driven actions.
+     * [crazybob] I'm not sure what you discussed, but it's *very* useful, and there should
be no reason not to do it. Say for example my form has a 'userId' and fields to set on the
user 'user.name', 'user.address'. With the sorting, 'userId' gets set first at which point
we load a `User` object. Then the other parameters get mapped to that `User` object. Without
the sorting, there's no guarantee on the ordering. You have to load the user in one action
and then chain to another. This is a common use case; might as well make it simple.
  
  == Nice to haves ==
  
@@ -101, +117 @@

  }}} We could specify mixin implementation classes by convention (defaults), in the configuration
file, or using annotations. This could also be a simpler alternative to action chaining in
many cases. 
  
      * [jcarreira] You had me until the abstract class bit... Does it have to be abstract?
Also, this limits testability in not-ok-ways... 
- 
+     * [crazybob] It only has to be abstract if you want your action to be able to call methods
on the mixin without casting. If it doesn't need to call those methods, there's no need for
your action to explicitly implement that interface. You could also say `((ValidationAware)
this).addActionError()`. I personally don't mind making the action abstract. In IntelliJ,
you just make a mock class that extends your action and it will automatically generate stubs
for the methods.
  
  == What JDK Version? ==
  
    * [jcarreira] We've been using JDK 1.5 on Tomcat 5+ for over a year... Everything we write
and wire together is using generics and annotations.
+   * [crazybob] +1 for JDK 1.5 since it came out. I have a lot of code I could contribute
which depends on the new concurrency libraries, etc.
  

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Mime
View raw message