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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
