This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 175548 - EL completion for Iterable types can be wrong
Summary: EL completion for Iterable types can be wrong
Status: VERIFIED FIXED
Alias: None
Product: javaee
Classification: Unclassified
Component: JSF Editor (show other bugs)
Version: 6.x
Hardware: All All
: P1 blocker (vote)
Assignee: Marek Fukala
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-27 20:37 UTC by Marek Fukala
Modified: 2009-11-25 05:00 UTC (History)
4 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Marek Fukala 2009-10-27 20:37:02 UTC
Beans:

public List<String> getItems() {
      return Arrays.asList(new String[]{"red", "green", "blue"});
} 

"#{Beans.items.bytes}" is incorrect. Because "items" doesn't have "String" type.
This is correct :
"#{Beans.items[0].bytes}". 

OTOH:

<h:body>
       <h:dataTable var="x" value="#{Bean.items}">
           <h:column>
               <h:outputText value="#{x.toUpperCase()}"/>
           </h:column>
       </h:dataTable>"
   </h:body>

Where the Bean methods are:

public List<String> getItems() {
       return Arrays.asList(new String[]{"red", "green", "blue"});
   }
Comment 1 Marek Fukala 2009-11-13 03:33:24 UTC
Nasty issue, completely wrong items are offered by the completion in most cases. Should be fixed somehow => P2.

As for the fix, Denis, I am likely not able to manage to fix this one by the code freeze. The proper fix would be quite complicated. 

What about offering both variants in the completion? Probably separated by a items separator in the completion. Top items would be the Iterable properties/methods itself, below after a separator there could be the Iterable item type items.

What do you think? And could you do it today? It seems to me to be quite easy fix, but I had such feeling in the past and it turns into nasty regressions.

Can you please help me whit that if you do not have any higher priority issues? If you are busy with other isssues, just let me know, I'll fix and you'll just do a code revieq of the fix.

Thank you in advance!
Comment 2 Marek Fukala 2009-11-13 03:35:17 UTC
QE, your opinion on the proposed fix?
Comment 3 Denis Anisimov 2009-11-13 04:49:16 UTC
This bug is regression .
All works fine before you changed the logic inside JSF EL expression related
to JSF parsing ( it is something about Nodes of JSP parser. I'm not sure
exactly ).
After this you have added incorrect logic into getTypePreceedingCaret().

I'm absolutely sure that the latter code should be removed from method 
getTypePreceedingCaret(). It should be applied to dataTable case ( ONLY 
this case should consider Iterable<T> from the point of view its parametrized
type T ) outside of the method getTypePreceedingCaret().
This algorithm was exactly implemented by me before your change.

I looked at the code some time ago ( when I pointed to you this issue ) and 
realized that I mostly don't understand its current state. This is why I 
asked the number of questions to you about it.

So, I VERY AFRAID to introduce a huge number of regressions to this code
one more time ( because as I said I don't have clear understanding of current code ).

Surely , I'm able and can try to help to you with this . But I will need 
some time for understanding current code for avoiding break of its logic and
accurate corrections. Probably I will need more time than just today . 
But today is last day before code freeze. So I don't know whether I can solve 
it......
But I will try at least to find the solution......
Comment 4 Marek Fukala 2009-11-13 05:24:11 UTC
OK, Denis. I'll handle the issue. You do not have to bother yourself with it. Thanks.

I'll likely duplicate the method getTypePreceedingCaret() and use the current version for the 'datatable' cases and the original for the rest. There is a support for the recursive variables resolvation so I'll use it somehow to distinguish these two cases.
Comment 5 Denis Anisimov 2009-11-13 07:29:44 UTC
Sounds reasonable. It is similar that was done originally by me in this area.
But probably you just need to drop code related to Itarable inside getTypePreceedingCaret() .
Please inspect method JsfElExpression.getTypeName(). You can find comment for
it. It was introduced as fix for 172824 ( this issue about dataTable case ).
It was written EXACTLY for the "dataTable" purpose.
But now is see its usages in many places.
Probably it is wrong.
"varType" argument in this method should be type of "var" attribute in the
"dataTable" declaration ( in the first example of this issue this is type
 of "x" ). 
I don't see source code that was implemented by me which perform all this logic
( with getTypeName() usage ).
But it seems this method was really originally used only for "dataTable" purpose.
So you can check previous revisions of JsfElExpression class.

I hope this helps.

I really don't refuse to work on issues and help with any problem.
But I'm really afraid of damage in the existing logic because it is already
out of my control.

Thanks for understanding.
Comment 6 Marek Fukala 2009-11-13 07:37:05 UTC
Thans for the evaluation Denis. I'll try handle it somehow if time permits. The problem with me is that the code always has been out of my control :-) But you are right that I was the maker of the recent changes so I should sweep what I messed up.
Comment 7 Denis Anisimov 2009-11-13 08:04:19 UTC
One more thing ( I'm trying to remember as it was originally done ):
- argument of method getTypeName() should be expression which defines variable.
F.e. 
<h:dataTable var="x" value="#{Bean.items}">
           <h:column>
               <h:outputText value="#{x.toUpperCase()}"/>
           </h:column>
</h:dataTable>

One needs to resolve "x.toUpperCase()" expression.
"x" should be found in the its declaration.
String "Bean.items" is declaration for "x" var.
This string should be passed into getTypeName() method.
This method returns type which "x" will have in further usage 
( if Bean.items instanceof Iterable<T> then getTypeName() returns T as type
for "x" ).
It is worth to look at implementation of getTypeName().
Once type of "x" is known one can use the same code pattern as used in
getTypeName() method ( but skip snippet which is related to Iterable )
with class InspectPropertiesTask usage.
It has CTOR with string as bean class name.
Previously found type of "x" should be passed to CTOR.
Then getTypePreceedingCaret method should be called once again ( as 
it was done in getTypeName() ) with appropriate arguments.

It seems originally it was written in this described manner.
But this should be present in some old revisions of JsfElExpression
class for sure.
Comment 8 Marek Fukala 2009-11-16 06:20:14 UTC
Agreed with QE this is a stopper.

fixed in web-main#ea339b40ad21

The fix is quite simplified so the Iterable - to its parameter type conversion is applied on the WHOLE expression, not just on the iterated object.

For example if in the following code items is of List<Item> and the Item has method of List<InnerItem> getInnerItems() type the completion will offer InnerItems' properties instead of List's properties.

<h:dataTable var="x" value="#{Bean.items}">
           <h:column>
               <h:outputText value="#{x.innerItems.|}"/>
           </h:column>
</h:dataTable>

I consider this as a P3 bug since it applies only to the iterating components and the most common case - EL completion out of an iterating component works fine now.

QE, please verify, Denis please review.
Comment 9 Quality Engineering 2009-11-17 03:06:29 UTC
Integrated into 'main-golden', will be available in build *200911170201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/ea339b40ad21
User: Marek Fukala <mfukala@netbeans.org>
Log: #175548 -  EL completion for Iterable types can be wrong
Comment 10 Martin Schovanek 2009-11-25 05:00:39 UTC
Verified at the release68 by QE. But entered the #177534.