Bugzilla – Full Text Bug Listing
| Summary: | Changes to WebAppMerger causes regression in meger behavior. | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Java EE Tools | Reporter: | Larry Isaacs <larryisaacs> | ||||||||||||||
| Component: | jst.j2ee | Assignee: | Dimitar Giormov <dimitar.giormov> | ||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Chuck Bridgham <cbridgha> | ||||||||||||||
| Severity: | major | ||||||||||||||||
| Priority: | P3 | CC: | ccc, dimitar.giormov, kaloyan | ||||||||||||||
| Version: | 3.2 | Flags: | dimitar.giormov:
pmc_approved?
(david_williams) dimitar.giormov: pmc_approved? (raghunathan.srinivasan) dimitar.giormov: pmc_approved? (naci.dai) dimitar.giormov: pmc_approved? (deboer) dimitar.giormov: pmc_approved? (neil.hauge) kaloyan: pmc_approved+ cbridgha: review+ |
||||||||||||||
| Target Milestone: | 3.2.1 | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Whiteboard: | PMC_approved | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Larry Isaacs
Actually, WebAppMerger.mergeFilters() changed from using copyAllContentInBase() to using copyMissingContentInBase() when merging the filter mappings. WebAppMerger.mergeServlets() made the same change, so perhaps these are the only elements whose behavior has been affected. Created attachment 170884 [details]
JUnit patch to add tests for this use case
The patch adds a couple of tests to WebAppMergerTest and WebApp3MergerTest that exercises this use case.
The issue appears to be that merging behavior needs to differ between when the web.xml model is merged into the merged model and when annotations model are merged into the merged model. I think merging the web.xml model needs to behave like a "copy" and while merging the annotation model needs to behave like the currently implemented "add missing". The JUnit tests in the patch fail because they are testing for "copy" behavior when the merge is performing the "add missing". I think the merger will need to add support for "copy" so the Web25MergedModelProvider can use it for merging the web.xml model and continue to use the "add missing" support for merging the annotation model. I'll see if I can come up with a patch to the code that takes this approach and update the JUnit tests to test both "copy" and "add missing" modes of merging.
Created attachment 170897 [details]
Possible patch to address merge behavior for servlet and filter mappings
There may be a more appropriate way to fix this, but I believe this patch will address the issue while being minimally intrusive. It defines a new COPY type for merging. This new type is only used to alter merging of filter mappings and servlet mappings, which I believe will restore prior behavior with respect to merging the web.xml model into the merged model. Web25MergedModelProvider is updated to use the appropriate type of merge when merging the web.xml and annotations models.
Created attachment 170898 [details]
Improved patch to add JUnit tests to check serlvet and filter mapping merge behavior
This version of JUnit tests patch includes servlet and filter mapping tests that covers both "copy" and "add" merging.
Kaloyan, can your team handle this? Please retarget as appropriate. Just start digging into this. From first impressions the problem is obvious, the model utilizes the list of url-patterns and considers as duplicated all other filter/servlet mappings that it finds. This causes problems when the filter/servlet mapping is written in web 2.4 style in DD, where the filter mapping had for value an objet instead of list. The problem worsens with Java EE 6 where annotations are involved. And on top of it the annotation model for Web 3.0 does not process correctly the value annotations of servlets and filters. So I think I will start with Isaacs patch and work on the issue. Created attachment 171093 [details] patch addresses mapping merging problem in web 2.5 and 3.0 https://bugs.eclipse.org/bugs/show_bug.cgi?id=315755 was found during processing this bug report. for web 2.5 I used Isaacs solution and fixed a little bit web 3.0 processing. But there should be some careful testing of this. Created attachment 171094 [details]
almost the same test patch min corrections
Created attachment 171102 [details] Updated JUnit patch so WebApp3MergerTest tests WebApp3Merger Thanks Dimitar, Unfortunately, I copy and pasted the tests from WebAppMergerTest to WebApp3MergerTests and forgot to update the WebAppMerger to WebApp3Merger. This patch corrects that oversight. I also updated the method name testServletMappingsCaseSame() to testServletMappingsCaseSameAdd() to be consistent with the others. This patch is derived from your test patch, but I didn't notice any changes from the original patch, so maybe some of these changes were intented to be fixed by your patch. With this update, the testFilterMappingsCaseSameAdd() now fails for the WebApp3MergetTests because the two URL patterns are being merged into the one filter mapping. My understanding is that web.xml overrides annotations, so I wouldn't think the url-patterns would be additive. I'm not well connected to the Servlet spec process, so I could be out of date about that. There was a discussion about url-patterns between web.xml and web-fragment.xml on a Tomcat private list where the EG response was that these are not additive, web.xml wins. So I would think this would be true of annotations as well. Though I don't think it matters for servlet mappings, this merging the url-patterns would also alter the order of filters in a web.xml if it contained something like: <?xml version="1.0" encoding="UTF-8"?> <web-app xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://java.sun.com/xml/ns/javaee" xmlns:web="http://java.sun.com/xml/ns/javaee/web-app_2_5.xsd" xsi:schemaLocation="http://java.sun.com/xml/ns/javaee http://java.sun.com/xml/ns/javaee/web-app_2_5.xsd" id="WebApp_ID" version="2.5"> <filter> <filter-name>TestFilter</filter-name> <filter-class>test.TestFilter</filter-class> </filter> <filter> <filter-name>TestFilter2</filter-name> <filter-class>test.TestFilter2</filter-class> </filter> <filter-mapping> <filter-name>TestFilter</filter-name> <url-pattern>/bar/*</url-pattern> </filter-mapping> <filter-mapping> <filter-name>TestFilter2</filter-name> <url-pattern>/foo/*</url-pattern> </filter-mapping> <filter-mapping> <filter-name>TestFilter</filter-name> <url-pattern>/foo/*</url-pattern> </filter-mapping> </web-app> I would expect the merged model to accurately reflect this organization from the web.xml for both Servlet 2.5 and 3.0. About the additive merging, I actually spoke with a colleague from our runtime department, but did not check the spec, I will do so before proceeding. About web 3.0 merging into single object. servlet/filter mapping was altered in 2.5 to support list, in my opinion this gives us the opportunity to simplify the presentation of this relation. Going to web 2.4 approach does not seem right to me. And another reason for combining them is that it becomes scientifically harder to merge them correctly. Okay, I'm fine with the merged model differing in structure from the original web.xml. When the structure is important, the model for web.xml can be obtained. However, examining the implementation of Web25MergedModelProvider, you will have to ask for the merged model first before asking for the web.xml model, otherwise you will keep getting null for the web.xml model. I don't think the API makes this clear. Maybe this aspect of Web25MergedModelProvider should be improved at some point. Larry I am not sure if I understood you correctly, but basically you can get the xml model by using getModelObject(IPath). Is this what you are asking for? Yes. An adopter would use getModelObject(IPath) to get the web.xml model when seeing the actual web.xml structure is important. But adopters will need to be aware that WTP 3.2 in its current form requires that you call getModelObject() first, otherwise getModelObject(IPath) will keep returning null. Given that WTP 3.2 has already shipped this way, changing this in 3.2.x may not be worth it since the adopter would have to depend on 3.2.1 or later to avoid calling getModelObject() first. Adopters will also need to be aware that if their web project contains no annotations, the merged model may still differ from the web.xml model. It complicates usage, but is manageable. So as I understand basically you are ok with the current, but it will require a better one for the next version right? So I can proceed with commit? Maybe it will be best if we open another bug for this. Going back on what you said. You are right that the adopter should be aware that the merged model might be slightly different, than the xml model if there are no annotations. (maybe we can work on this and make it return untouched xml model) This I do not understand completely: "But adopters will need to be aware that WTP 3.2 in its current form requires that you call getModelObject() first, otherwise getModelObject(IPath) will keep returning null." Calling getModelObject(IPath) should return in all cases the xml or annotation model. getModelObject() is not requiired to be called before that. There are special cases where getModelObject() will return only xml model within modify call, since we have no support for editing and adding entries to the annotation model. SAS as an adopter routinely requires a specific version of Eclipse when using our product. Thus, our plug-ins can adapt to whatever behavior is implemented in Eclipse and WTP. Having the merged model not preserve the structure of the web.xml contribution complicates our implementation but is something we can deal with. So your changes are fine for committing from our point of view. I don't know about other adopters though. With respect to the calling getModelObject() before getModelObject(IPath), I'm refering to the use case where the model is being inspected and not necessarily modified, i.e. validateEdit() hasn't be called. For web.xml, getModelObject(IPath) will return null if ddProvider is null and will not force ddProvider to be set if it isn't already. Calling getModelObject() will force ddProvider to be set. I've seen this issue in our JUnit tests, so it might be a dispose() that's causing ddProvider to be null and calling getModelObject() helps ensure it is restored since getModelObject(IPath) won't do it. @Larry Thanks now I understand. So I will create a separate bug for this and try to fix it for SR1 if this is ok for you? @Chuck can you review the fix? approved This bug is caused by adapting web merged model to work for Web 3.0 and changes the behavior of the MergedModel in Web 2.5 Current behavior will ignore certain servlet mappings declarations if they are defined in Web 1.4 style. where it is possible to have many declarations of servlet mapping with one and the same servlet name and different url pattern. The fix considers this behavior by adding copy mechanism from xml to the merged model where copy is used where no merge is necessary. Additionally to cover the Web 3.0 scenario the Mappings are merged in name > list structure. To test the issue additional junit tests were created. The patch is reviewed by Chuck I think this has a low risk. committed and released |