You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tiles.apache.org by Nicolas Le Bas <ma...@nlebas.net> on 2015/06/21 21:00:44 UTC
Pull Request #4: Object-Type Attribute Value Evaluation
Hi team,
I've been looking at this pull request, and I like it. I understand its
value, but it raises a number of interesting challenges and questions.
*Summary*
Brett wants to support evaluation of expressions in the attributes of
<item>.
Sample use case:
<definition name="customers/orders" extends="customers/base">
<put-list-attribute name="breadcrumbs">
<item value="${customer.id}" link="/customers/${customer.id}" />
<item value="Orders" link="/customers/${customer.id}/salesOrders" />
<item value="${order.id}"
link="/customers/${customer.id}/salesOrders/${order.id}" />
</put-list-attribute>
<put-list-attribute name="body" cascade="true" inherit="true">
<add-attribute value="/WEB-INF/views/customers/cust-orders.jsp" />
</put-list-attribute>
</definition>
*Challenges*
- <item> and <bean> are excluded from Tiles 3.x, they are only supported
as backwards compatibility in tiles-compat, and only in the XML
definition file. They cannot be used as part of an inline definition
(through the use of
<tiles:definition><tiles:put-list-attribute>...</...> in a JSP template).
- This change would somehow break backwards compatibility (an old
application would display the string "${customer.id}" instead of
"12345", this might be expected by those applications that came all the
way from struts 1)
- All other tags in tiles-defs.xml require the developer to specify
explicitely that she wants the expression evaluated: <add-attribute
expression="${customer.id}"/> instead of <add-attribute value="Customer
ID"/>. Doing differently for <item> would make this XML even more
confusing that it already is.
*Questions*
- Do we want to keep maintaining <item> and <bean>, given that today DI
frameworks provide several better ways to declare those beans? Do we
want to reintegrate these out of tiles-compat into tiles-core?
- I see the value in merging the "value" and "expression" attributes.
However I do not think it should be done as part of a minor release in
TILES_3_0_X and not without automated tests. So IMHO options would be:
use the "expression" attribute in <item> for a uniform syntax and
backwards compatibility, or merge the "expression" and "value"
attributes accross the board in a major release (3.1?).
I think it is difficult to understand all aspects of Tiles as it is now,
and we should assist Brett in prefecting his design.
Please share your thoughts,
Thanks
Nick
Re: Pull Request #4: Object-Type Attribute Value Evaluation
Posted by Brett Ryan <br...@gmail.com>.
> On 8 Jul 2015, at 23:21, Brett Ryan <br...@gmail.com> wrote:
>
>>
>> On 8 Jul 2015, at 21:32, Brett Ryan <br...@gmail.com> wrote:
>>
>> Nicolas Le Bas <ma...@...> writes:
>>
>>> I've been looking at this pull request, and I like it. I understand its
>>> value, but it raises a number of interesting challenges and questions.
>>>
>>> *Summary*
>>>
>>> Brett wants to support evaluation of expressions in the attributes of
>>> <item>.
>>
>> Thankyou. To be precise, I'm looking for evaluations of expressions on any
>> attribute that can contain multiple values. mck's solution that
>> `add-list-attribute` can be used, while verbose is sufficient, however;
>> presently an NPE is being thrown on the use of the `expression` attribute on
>> `add-list-attribute`.
>
> I've tracked down the NPE to two things. First, if Attribute has a null value, it returns the value null, which IMHO is something I discourage, toString should always be a string representation of a value, not null, alternatives would be "null" or "(null)".
>
> While this is the cause of the NPE, I've now found that uses of `<add-attribute expression="${some.val}"/>` within `<add-list-attribute>` are presently not supported.
>
> I shall attempt an enhancement to support this but due to the way tiles is structured it would ultimately be returning nested copies of lists which is similar to my original solution (with the added nesting for child properties).
>
> I would like to complete this sooner but might not be able to provide something before the weekend. Is there a preferred branch? I was going to submit this on the 3_0_X branch.
>
> Also, I've only created a few pull requests in the past for other projects, what's the best practice for submitting an enhancement? I've created a new branch rooted at commit cc11955, but I'm not sure how to create a pull request from that, will need to test, sorry.
>
> -Brett
So I've come up with a solution, though it kinda sucks. In what I'm starting to learn would I be correct in stating that tiles is somewhat inconsistent with how it presents attributes to the view? What I mean by this is that there is some discrepancy with how tiles presents attributes from `put-attribute` vs `put-list-attribute`.
With `put-attribute` the attribute itself is not exposed to the view, however with the `put-list-attribute` everything is. This makes things tricky as what I was originally intending to alter AbstractAttributeEvaluator#evaluate to process the list elements and recursively build up a new list for the view which contains an evaluated version of all attributes.
I've done this, and it works, but; I'm now faced with the dilemma that previously accessing a list-attribute would require accessing the value property of the attribute as it's exposed to the view.
So, what should I do to keep backwards compatible? I've had to clone Attributes and set the value to the evaluated expression (where value is null) and return the new mutated attribute, but I don't like the approach, it's unnecessary IMHO. I was required to do it though, it turns out that the following:
<tiles:importAttribute name="main" toName="main" ignore="true"/>
<c:forEach var="x" items="${main}">
<tiles:insertAttribute value="${x}" flush="true" />
</c:forEach>
Would break as for whatever known reason it's trying to cast "x" back to an attribute, which I couldn't see why and honestly couldn't be bothered figuring out right now.
If anyone can give me guidance on what I should do I can have a pull request completed tomorrow for allowing add-list-attribute's to evaluate expressions.
Following is the change toAbstractAttributeEvaluator#evaluate(Attribute, Request) that would be submitted if anyones interested:
@Override
public Object evaluate(Attribute attribute, Request request) {
if (attribute == null) {
throw new IllegalArgumentException("The attribute cannot be null");
}
Object retValue = attribute.getValue();
if (retValue == null) {
Expression expression = attribute.getExpressionObject();
if (expression != null) {
log.debug("Evaluating expression: [attribute={},expression={}]", attribute, expression);
retValue = evaluate(attribute.getExpressionObject()
.getExpression(), request);
}
} else if (retValue instanceof List) {
log.debug("Evaluating list for expressions: {}", retValue);
retValue = evaluateList((List) retValue, request);
}
log.debug("Returning result: {}", retValue);
return retValue;
}
private List evaluateList(List src, Request request) {
List res = new ArrayList();
for (Object val : src) {
if (val instanceof ListAttribute) {
log.debug("Evaluating list entry (ListAttribute): {}", val);
res.add(evaluateList(((ListAttribute) val).getValue(), request));
} else if (val instanceof Attribute) {
log.debug("Evaluating list entry (Attribute): {}", val);
Attribute att = (Attribute) val;
if (att.getValue() != null) {
res.add(att.getValue() instanceof List
? evaluateList((List) att.getValue(), request)
: att);
} else {
Expression expression = att.getExpressionObject();
if (expression != null) {
log.debug("Evaluating list entry expression: {}", expression);
att = att.clone();
att.setValue(evaluate(expression.getExpression(), request));
res.add(att);
} else {
res.add(att);
}
}
} else {
log.debug("Evaluating list entry ({}): {}", val.getClass(), val);
res.add(val);
}
}
return res;
}
Re: Pull Request #4: Object-Type Attribute Value Evaluation
Posted by Brett Ryan <br...@gmail.com>.
> On 8 Jul 2015, at 21:32, Brett Ryan <br...@gmail.com> wrote:
>
> Nicolas Le Bas <ma...@...> writes:
>
>> I've been looking at this pull request, and I like it. I understand its
>> value, but it raises a number of interesting challenges and questions.
>>
>> *Summary*
>>
>> Brett wants to support evaluation of expressions in the attributes of
>> <item>.
>
> Thankyou. To be precise, I'm looking for evaluations of expressions on any
> attribute that can contain multiple values. mck's solution that
> `add-list-attribute` can be used, while verbose is sufficient, however;
> presently an NPE is being thrown on the use of the `expression` attribute on
> `add-list-attribute`.
I've tracked down the NPE to two things. First, if Attribute has a null value, it returns the value null, which IMHO is something I discourage, toString should always be a string representation of a value, not null, alternatives would be "null" or "(null)".
While this is the cause of the NPE, I've now found that uses of `<add-attribute expression="${some.val}"/>` within `<add-list-attribute>` are presently not supported.
I shall attempt an enhancement to support this but due to the way tiles is structured it would ultimately be returning nested copies of lists which is similar to my original solution (with the added nesting for child properties).
I would like to complete this sooner but might not be able to provide something before the weekend. Is there a preferred branch? I was going to submit this on the 3_0_X branch.
Also, I've only created a few pull requests in the past for other projects, what's the best practice for submitting an enhancement? I've created a new branch rooted at commit cc11955, but I'm not sure how to create a pull request from that, will need to test, sorry.
-Brett
Re: Pull Request #4: Object-Type Attribute Value Evaluation
Posted by Brett Ryan <br...@gmail.com>.
Nicolas Le Bas <ma...@...> writes:
> I've been looking at this pull request, and I like it. I understand its
> value, but it raises a number of interesting challenges and questions.
>
> *Summary*
>
> Brett wants to support evaluation of expressions in the attributes of
> <item>.
Thankyou. To be precise, I'm looking for evaluations of expressions on any
attribute that can contain multiple values. mck's solution that
`add-list-attribute` can be used, while verbose is sufficient, however;
presently an NPE is being thrown on the use of the `expression` attribute on
`add-list-attribute`.
My exceptions are somewhat swallowed, but as far as I can gather without further
traces I'm recieving:
Caused by: java.io.IOException:
ServletException including path '/WEB-INF/layout/web-base.jsp'.
at org.apache.tiles.request.servlet.ServletUtil
.wrapServletException(ServletUtil.java:61)
at org.apache.tiles.request.servlet.ServletRequest
.forward(ServletRequest.java:267)
at org.apache.tiles.request.servlet.ServletRequest
.doForward(ServletRequest.java:228)
at org.apache.tiles.request.AbstractClientRequest
.dispatch(AbstractClientRequest.java:57)
at org.apache.tiles.request.render.DispatchRenderer
.render(DispatchRenderer.java:47)
at org.apache.tiles.impl.BasicTilesContainer
.render(BasicTilesContainer.java:259)
at org.apache.tiles.impl.BasicTilesContainer
.render(BasicTilesContainer.java:397)
... 78 more
Caused by: java.lang.NullPointerException
at org.apache.taglibs.standard.tag.common.core.OutSupport
.out(OutSupport.java:179)
at org.apache.taglibs.standard.tag.common.core.OutSupport
.doStartTag(OutSupport.java:99)
...
at org.apache.tiles.request.servlet.ServletRequest
.forward(ServletRequest.java:265)
... 83 more
> *Challenges*
>
> - <item> and <bean> are excluded from Tiles 3.x, they are only supported
> as backwards compatibility in tiles-compat, and only in the XML
> definition file. They cannot be used as part of an inline definition
> (through the use of
> <tiles:definition><tiles:put-list-attribute>...</...> in a JSP template).
That's fine, I'm happy to use newer approaches, I just found `MenuItem` at the
time of a way to create tuples as attributes, previously I thought all
attributes were single values/expressions in lists.
> - This change would somehow break backwards compatibility (an old
> application would display the string "${customer.id}" instead of
> "12345", this might be expected by those applications that came all the
> way from struts 1)
>
> - All other tags in tiles-defs.xml require the developer to specify
> explicitely that she wants the expression evaluated: <add-attribute
> expression="${customer.id}"/> instead of <add-attribute value="Customer
> ID"/>. Doing differently for <item> would make this XML even more
> confusing that it already is.
>
> *Questions*
>
> - Do we want to keep maintaining <item> and <bean>, given that today DI
> frameworks provide several better ways to declare those beans? Do we
> want to reintegrate these out of tiles-compat into tiles-core?
>
> - I see the value in merging the "value" and "expression" attributes.
> However I do not think it should be done as part of a minor release in
> TILES_3_0_X and not without automated tests. So IMHO options would be:
> use the "expression" attribute in <item> for a uniform syntax and
> backwards compatibility, or merge the "expression" and "value"
> attributes accross the board in a major release (3.1?).
I would LOVE to see tiles have a single attribute for both expression and value
and an attribute to turn expression evaluation on/off. The reason I say this is
it would IMHO simplify the definition and should simplify tiles internally by
use of a single value. A second attribute "evaluate" could be supplied on any
attribute which could cascade to lower levels.
> I think it is difficult to understand all aspects of Tiles as it is now,
> and we should assist Brett in prefecting his design.
My solution has come about quite likely to my lack of understanding tiles in
full. I would love to help contribute more with some of the documentation, as
prior today I didn't really know about `add-list-attribute`; apart from the DTD
and API documentation there's no reference to it in the examples or any uses I
could find within google searches.