struts-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Don Brown <mr...@twdata.org>
Subject Re: [action2] Public API first draft
Date Thu, 04 May 2006 21:09:58 GMT
Bob Lee wrote:
>> o.a.s.action2 - I'd like to hear the design reasoning behind the
>> Messages changes.  I liked the use of Maps in the XWork design as it
>> made it easier to work with.  On the other hand, encapsulating message
>> operations in the Messages object does reduce the number of
>> message-handling methods required.  Perhaps Messages could extend Map?
>>  Also, I agree we should continue to support plain Messages, as not
>> all apps need to be localized.
> 
> - Passing in keys vs. actual messages - I think always passing in keys
> is one thing Struts got right. Even if you only support one language,
> abstracting messages out of your code is still a good practice.

It is generally possible to do both, perhaps through a configuration switch that 
controls whether to report errors on missing message keys or to treat them as 
the message.

> - This design doesn't force inheritance. Notice we don't have
> ActionSupport anymore? If users still want to have the implicit
> methods, they can statically import them from a support class. The
> support class implementation would look up the thread local Request
> and delegate to it. I didn't add this yet because I'm not sure if we
> really need it.

I agree that we should move away from basically requiring ActionSupport to be 
extended, which is why I was wondering about the possibility of moving those 
*Aware interfaces over to Request to decorate the Request instance.

> - In the old design, messages were scoped to an action which doesn't
> work very well when you're chaining and you want to display all the
> errors from all the actions. I think we want messages to be scoped to
> the request (i.e. spanning multiple actions). We actually use our own
> message handling and ignore WebWork's at the moment.

Agreed.

> - Regarding your Map comment, we've considered having
> Messages.forFields() return a Map<String, List<String>>. Would that
> work? That would be identical to what WebWork supports today.

I'd like to see it implement Map directly, allowing you to treat it as a simple 
Map if your requirements are simple enough.  The use of standard collections is 
something I'd like to retain from WebWork 2.

>> I'm not sure I understand the separation of Validatable and
>> ErrorAware.  In particular, Validatable only has a validate() method
>> that returns void.  Is there really a case you want to validate, but
>> not have errors sent anywhere?  I think of the use case of plugging in
>> a different validation engine like commons-validator, and this
>> separation makes it harder.  Why not return Messages?
> 
> I've been thinking Validatable should extend ErrorAware, but I wanted
> to see what you guys thought first. I don't like the idea of
> validate() return Messages. I never liked the fact that I had to
> create an errors object in Struts. I also want to be able to easily
> add validation messages from setters (I like doing field level
> validation in the setter and cross field validations in validate()).

I think Jason had a good observation here concerning ValidationContext.  I like 
having a context (or perhaps Request itself) that implements Error/MessageAware 
and have that passed around.  I agree the user shouldn't be creating this 
themselves.

>> o.a.s.action2.attribute - This is again a feature I'd like to hear the
>> design reasonings of.  I liked the previous use of Maps as they were
>> easy to test and pass around.  Maps can also be extended to provide
>> the automatic synchonrization feature talked about easily.
> 
> This is something I've been kicking around since generics came out.
> Now that we have generics, it seems funny to be passing around a
> heterogeneously typed map. The new way results in better type safety
> and less code. Here are some examples for comparison:
> 
> Old way:
> 
>    public class OldWay implements SessionAware {
> 
>        static final String FOO_KEY = Foo.class.getName();
> 
>        Map<String, Object> session;
> 
>        public void setSession(Map<String, Object> session) {
>            this.session = session;
>        }
> 
>        public String execute() {
>            Foo foo = (Foo) session.get(FOO_KEY);
>            if (foo == null) {
>                foo = new Foo();
>                session.put(FOO_KEY, foo);
>            }
>            foo.doSomething();
>            return SUCCESS;
>        }
>    }
> 
> New way:
> 
>    public class NewWay {
> 
>        Attribute<Foo> fooAttribute = new
> SessionAttribute<Foo>(Foo.class.getName()) {
>            protected Foo initialValue() {
>                return new Foo();
>            }
>        };
> 
>        public String execute() {
>            Foo foo = fooAttribute.get();
>            foo.doSomething();
>            return SUCCESS;
>        }
>    }
> 
> Testing is actually easier--it's easier to mock out Attribute than Map.
> 
> I also proposed an annotation based approach, but Patrick preferred
> the attribute API. Here's how that could work:
> 
>    public class AnnotationWay {
> 
>        Foo foo;
> 
>        // result is stored on session, called after execute() but
> before result.
>        @SessionAttribute
>        public Foo getFoo() {
>            if (foo == null) {
>                this.foo = new Foo();
>            }
>            return foo;
>        }
> 
>        // parameter pulled from session, called before anything else.
>        @SessionAttribute
>        public void setFoo(Foo foo) {
>            this.foo = foo;
>        }
> 
>        public String execute() {
>            getFoo().doSomething();
>            return SUCCESS;
>        }
>    }
> 
> I'm inclined to agree with Pat. The annotation approach is clean and
> simple, but it conflicts with setting parameters (i.e. how do you
> prevent setting parameters on session objects for security reasons?)
> and is a little magical.

While I can appreciate the type safety and perhaps we do want to keep it as an 
option, it just looks way to complicated for new users.  If people need to put 
things in the session, they can just get the map from Request and put it in 
there.  The annotations are nice, and would be an interesting addition when we 
do that layer.

>> o.a.s.action2.servlet - I like how the servlet-related classes are
>> contained in their own package.  This distinction is very important as
>> I plan to be personally using Action 2 mostly for Portlets.
> 
> I talked with Pat about whether we should build an abstraction layer
> over the servlet/portlet APIs, and we decided against it. The portlet
> API really should have tried harder to embrace the servlet API. To
> support the portlet API, we plan to implement a servlet API adapter to
> it.

Well, I'm of the opinion that the Portlet API specifically shouldn't have 
extended the Servlet API, but I think there is something to use implementing 
Servlet API classes with Portlet implementations.  As I've previously mentioned, 
this is the approach Cocoon is either looking to do or has done by now, and I 
think we should work with them to complete that library and perhaps move it to 
Jakarta Commons.

> The deal breaker for me was that users will be able to reuse the same
> code inside and outside of our framework. They won't have to port
> their code from the servlet API to our new abstraction API. And it
> makes less work for us and fewer new things for users to learn. ;)
> 
>> o.a.s.action2.spi - Again, I love this separation - move all the
>> framework classes the user rarely deals with out into its own package.
>>  I like making ValueStack a first class interface, and the Interceptor
>> changes seem reasonable.  I am somewhat concerned about the Request
>> interface though.  While the goal of putting everything you need for a
>> request in one object, it seems to be combining too many roles.  For
>> one, it has servlet object retrieval methods in there that should be
>> separated, and I'm not sure if it should be handing the execution of
>> actions and interceptors as well.  If the goal is to turn this into a
>> Tapestry-like Visitor object which holds all the request state, we
>> should separate out the servlet getters into their own interface to be
>> combined by the user.
> 
> We've been playing with some ideas here. First off, actions should
> pretty much never see this Request object. They still get everything
> they need through the *Aware interfaces, etc.
> 
> I'm thinking of creating an ActionContext interface and moving all the
> information about the currently executing action into there. Then we'd
> have Request.getCurrentActionContext(), and we could even provide
> access to the contexts of other actions in the chain if we need to.
> 
> I'm not sure I understand what you mean about moving the servlet
> methods into a separate interface. Can you provide an example? To be
> clear, users wouldn't implement the Request interface, but framework
> implementors would.

What I'm talking about is making Request the decorated core object used to share 
and access information, however rather than build all possible methods into it, 
split them out in a similar way to *Provider interfaces.  Then, an advanced user 
_could_ create their own Request object with special application-specific methods.

For example:

o.a.s.action2.spi.ServletProvider
o.a.s.action2.spi.MessagesProvider
o.a.s.action2.spi.LocaleProvider

then o.a.s.action2.spi.Request would extend all these interfaces.  Then, code 
that uses the Request could only specify the more specific interface like 
ServletProvider, making it easier to mock and extend.

I haven't thought it out fully, but I'm wondering if that would help reduce our 
need for a buff'ed up ActionSupport parent class.

Don

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


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


Mime
View raw message