[Struts Wiki] Update of "RoughSpots" by JasonCarreira

[Apache Wiki <wi...@apache.org>]

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 JasonCarreira:
http://wiki.apache.org/struts/RoughSpots

------------------------------------------------------------------------------
  
    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
+ 
    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`.
  
    1. Only make classes/members public if we're willing to guarantee future compatibility. Everything else should be package-private or excluded from the Javadocs.
@@ -21, +23 @@

  
    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... 
+ 
    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.
  
    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 {
@@ -32, +38 @@

  }
  }}}
  
+ [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...
+ 
    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 :-)
+ 
    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.
  
    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.
+ 
    1. Eliminate non-private fields. Protected fields in `ActionConfig` for example.
  
+ [jcarreira] We don't want to allow for extension?
+ 
    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.
  
    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...    
+ 
-     * 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."
  
+ [jcarreira] I think it's great that you want to take the HTTP spec at its word... most users don't care though.
+ 
    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.
+ 
+ [jcarreira] That one always has been confusing...
  
    1. Warnings and failing silently is not the best practice. For example, if we can't find a method in a given action class, blow up ASAP (at load time). We shouldn't be fuzzy and do stuff like try to find a method named `doXxx()` when we can't find a method named `xxx()` (WebWork had backward compatibility restrictions, but we don't need to perpetuate them).
  
    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...
+ 
    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.
  
  == Nice to haves ==
  
@@ -71, +100 @@

  } 
  }}} 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... 
+ 

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