Bug 319550

Comment 1 Carl Anderson 2010-07-12 08:58:07 EDT

Assigning to Dimitar for initial investigation.  

If this can be quickly fixed, we would love to see it fixed in WTP 3.2.1

The problem here comes from org.eclipse.jst.j2ee.web/templates/filter_classHeader.template
There is a section wrapped by:

if (model.isAnnotated()) {

That section contains the proper code for handling the older style dispatchers.  However, in the section wrapped by:

if ("3.0".equals(model.getJavaEEVersion())) {

There is no equivalent section for the Web 3.0 dispatcherTypes

Comment 2 Dimitar Giormov 2010-07-12 10:58:57 EDT

Created attachment 174039 [details]
fix for dispatcher annotation parameter

Comment 3 Dimitar Giormov 2010-07-12 11:03:19 EDT

Here is the fix.
I think we can push it in 3.2.1

Additionally Carl can you review the fix and the behavior, I had a talk with our run-time colleagues and we came up with this implementation. In the specification the information is really scarce. 
Basically we took the aggregation approach, where all dispatcher types are aggregated in the annotation without duplication.

@Chuck can you review the fix.

Comment 4 Chuck Bridgham 2010-07-12 13:36:19 EDT

Patch looks good

Comment 5 Dimitar Giormov 2010-07-13 10:38:46 EDT

Carl can you review the fix, so I can ask for PMC review.

Comment 6 Carl Anderson 2010-07-13 11:28:28 EDT

Dimitar, the fix is incomplete (see the end of this comment for the main reason).  I apologize for the delay, but I am still working to verify what the proper fix should be.

So far, here is what I am certain of:
As of the Servlet 2.5 specification, a filter mapping can have one set of dispatcher types, and then it can have one or more URL patterns and/or Servlet mappings.

Our current UI limits a filter mapping to the Servlet 2.4 limitations- each filter mapping has one set of dispatcher types, and only one of either a URL pattern or a Servlet mapping.  I will open a separate enhancement to address that limitation, but we can live with the current UI for now.

Our current UI allows for multiple filter mappings on a filter.  I am investigating how that should be put into an @WebFilter, but do not have a good answer yet.

Your current patch adds in dispatcherType support for URL patterns.  It does not add in dispatcherType support for Servlet mappings.  And the way it is coded in CreateFilterTemplateModel.getClassAnnotationParams(), it worries about the type of the mapping first.  The dispatcherType should be handled first, and then the type (URL pattern/Servlet name) should then be considered.

Comment 7 Carl Anderson 2010-07-13 14:12:38 EDT

I opened bug 391745 to cover the missing spec coverage.  I am still investigating the @WebFilter annotations specifics.

Comment 8 Dimitar Giormov 2010-07-14 03:32:43 EDT

Created attachment 174261 [details]
processing of servletmapping added

Here is the new patch.
I will spare you the long comment that I thought that I should check this and completely forgot about it:)

This looks like a good short term solution for now. Is it already too late for 3.2.1?

Comment 9 Carl Anderson 2010-07-14 10:21:45 EDT

It still doesn't work for the servlet name only case- the following code block stops it:

if (urlMappings.size() > 0) {
	result.put(ATT_URL_PATTERNS, urlMappings);
	if (dispatcherTypes.size() > 0) {
		result.put(ATT_DISPATCHER_TYPES, dispatcherTypes);
	}
}

Today is the last day for non-critical changes to WTP 3.2.1.

Comment 10 Dimitar Giormov 2010-07-15 07:46:33 EDT

Created attachment 174389 [details]
version 3

@Chuck, Carl - Can you review the latest patch.

Comment 11 Carl Anderson 2010-07-15 10:03:58 EDT

I approve of this patch.

Comment 12 Chuck Bridgham 2010-07-15 10:20:44 EDT

approved

Comment 13 Dimitar Giormov 2010-07-15 10:31:54 EDT

Dear PMC members:

Description: Because of this bug the dispatcherTypes stated in the Filter wizard are completely ignored and not added to the Filter annotations.

Workaround: To workaround this issue is adding the dispatcherTypes manually  in the annotation

Testing: The fix has been tested manually

Reviewers: The fix was reviewed by: Chuck and Carl

What is the risk associated with this fix? - Low.

Comment 14 David Williams 2010-07-15 10:52:35 EDT

Pretty big code change (in terms of number of lines) but they seem relatively "routine", and does sound like "missing function" for this area, so I'm fine if this can get into this week's build.

Comment 15 Dimitar Giormov 2010-07-15 11:51:10 EDT

Committed and released.

Thanks to all.