You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jetspeed-dev@portals.apache.org by "David Jencks (JIRA)" <je...@portals.apache.org> on 2006/01/19 19:16:42 UTC

[jira] Created: (JS2-473) Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid

Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid
------------------------------------------------------------------------------------------------------

         Key: JS2-473
         URL: http://issues.apache.org/jira/browse/JS2-473
     Project: Jetspeed 2
        Type: Bug
    Versions: 2.1-dev    
    Reporter: David Jencks


While trying to see if my proposed solution to JS2-472 would break anything, I noticed that many uses of Fragment.getFragments() assume they are getting the original mutable list, and proceed to modify it.  Since they may get a copy, any modifications would be discarded.   Here's a usage search from intellij with some annotations: I've marked obviously dangerous uses with >>>>.  There is no reason to suppose the other uses are safe without looking into them further.  I don't think I'm qualified to fix this without at least some guidance.

Method
    getFragments():List of interface org.apache.jetspeed.om.page.Fragment
Found usages  (70 usages)
    Unclassified usage  (70 usages)
        jetspeed-layouts  (5 usages)
            org.apache.jetspeed.portlets.layout  (5 usages)
                LayoutPortlet  (2 usages)
                    addPortletToPage(String, String)  (1 usage)
>>>>                        (279, 18) root.getFragments().add(fragment);
                    removeFragment(String, String)  (1 usage)
>>>>                        (257, 18) root.getFragments().remove(f);
                MultiColumnPortlet  (2 usages)
                    doView(RenderRequest, RenderResponse)  (1 usage)
                        (94, 65) layout = new ColumnLayout(numColumns, layoutType, f.getFragments(), this.colSizes.split("\\,") );
                    processAction(ActionRequest, ActionResponse)  (1 usage)
                        (242, 80) layout = new ColumnLayout(numColumns, layoutType, rootFragment.getFragments(), this.colSizes.split("\\,") );
                PageManagerLayoutEventListener  (1 usage)
                    handleEvent(LayoutEvent)  (1 usage)
>>>>                        (28, 40) page.getRootFragment().getFragments().add(event.getFragment());
        jetspeed-page-manager  (60 usages)
            org.apache.jetspeed.om.page  (15 usages)
                ContentFragmentImpl.ContentFragmentList  (1 usage)
                    (441, 42) private List baseList = fragment.getFragments();
                TestPageObjectModel  (14 usages)
                    testFragmentManipulation()  (14 usages)
                        (108, 28) assertNotNull(root.getFragments());
>>>>                        (114, 14) root.getFragments().add(frag1);
>>>>                        (128, 15) frag2.getFragments().add(frag3);
>>>>                        (129, 14) root.getFragments().add(frag2);
                        (132, 25) assertTrue(root.getFragments().size()==2);
                        (133, 27) Iterator i = root.getFragments().iterator();
                        (142, 22) assertTrue(f.getFragments().size()==0);
                        (147, 22) assertTrue(f.getFragments().size()==1);
                        (149, 22) assertTrue(f.getFragments().size()==1);
                        (150, 15) i = f.getFragments().iterator();
>>>>                        (164, 11) f.getFragments().remove(frag3);
>>>>                        (167, 11) f.getFragments().add(frag2);
                        (168, 22) assertTrue(f.getFragments().size()==1);
                        (169, 29) f = (FragmentImpl)f.getFragments().get(0);
            org.apache.jetspeed.om.page.psml  (4 usages)
                PageImpl  (4 usages)
                    getFragmentById(String)  (1 usage)
                        (177, 28) Iterator i = f.getFragments().iterator();
                    getFragmentsByName(String)  (1 usage)
                        (276, 28) Iterator i = f.getFragments().iterator();
                    removeFragmentById(String)  (2 usages)
                        (209, 28) Iterator i = f.getFragments().iterator();
>>>>                        (234, 28) if (parent.getFragments().remove(f))
            org.apache.jetspeed.page  (41 usages)
                AbstractPageManager  (2 usages)
                    copyFragment(Fragment, String)  (2 usages)
                        (889, 37) Iterator fragments = source.getFragments().iterator();
>>>>                        (894, 18) copy.getFragments().add(copiedFragment);
                PageManagerTestShared.Shared  (15 usages)
                    testSecurePageManager(TestCase, PageManager)  (15 usages)
>>>>                        (281, 34) root.getFragments().add(portlet);
>>>>                        (287, 34) root.getFragments().add(portlet);
                        (290, 71) test.assertNotNull(page.getRootFragment().getFragments());
                        (291, 73) test.assertEquals(2, page.getRootFragment().getFragments().size());
                        (292, 106) test.assertEquals("some-app::SomePortlet", ((Fragment)page.getRootFragment().getFragments().get(1)).getName());
                        (293, 91) test.assertFalse("0".equals(((Fragment)page.getRootFragment().getFragments().get(1)).getId()));
                        (294, 82) somePortletId[0] = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
                        (348, 72) test.assertNotNull(page0.getRootFragment().getFragments());
                        (349, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
                        (389, 72) test.assertNotNull(page0.getRootFragment().getFragments());
                        (390, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
                        (458, 72) test.assertNotNull(page0.getRootFragment().getFragments());
                        (459, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
                        (518, 72) test.assertNotNull(page0.getRootFragment().getFragments());
                        (519, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
                TestCastorXmlPageManager  (15 usages)
                    testClonePage()  (4 usages)
                        (861, 30) List children = root.getFragments();
                        (862, 40) List cloneChildren = cloneRoot.getFragments();
                        (905, 26) assertNotNull(cf.getFragments());
                        (906, 23) assertTrue(cf.getFragments().size() == 2);
                    testCreatePage()  (3 usages)
>>>>                        (266, 14) root.getFragments().add(f);
                        (302, 43) assertTrue(page.getRootFragment().getFragments().size() == 1);
                        (304, 47) f = (Fragment) page.getRootFragment().getFragments().get(0);
                    testGetPage()  (3 usages)
                        (190, 30) List children = root.getFragments();
                        (237, 25) assertNotNull(f.getFragments());
                        (238, 22) assertTrue(f.getFragments().size() == 2);
                    testUpdatePage()  (5 usages)
                        (373, 28) assertNotNull(root.getFragments());
                        (374, 30) assertEquals(1, root.getFragments().size());
                        (375, 41) String testId = ((Fragment)root.getFragments().get(0)).getId();
                        (394, 28) assertNotNull(root.getFragments());
                        (395, 25) assertTrue(root.getFragments().isEmpty());
                TestDatabasePageManager  (9 usages)
                    testCreates()  (2 usages)
>>>>                        (317, 14) root.getFragments().add(portlet);
>>>>                        (328, 14) root.getFragments().add(portlet);
                    testGets()  (4 usages)
                        (620, 51) assertNotNull(check.getRootFragment().getFragments());
                        (621, 53) assertEquals(2, check.getRootFragment().getFragments().size());
                        (622, 65) Fragment check0 = (Fragment)check.getRootFragment().getFragments().get(0);
                        (643, 65) Fragment check1 = (Fragment)check.getRootFragment().getFragments().get(1);
                    testUpdates()  (3 usages)
                        (814, 46) assertNotNull(page.getRootFragment().getFragments());
                        (815, 48) assertEquals(2, page.getRootFragment().getFragments().size());
                        (816, 61) String removeId = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
        jetspeed-portal  (4 usages)
            org.apache.jetspeed.aggregator.impl  (1 usage)
                BasicAggregator  (1 usage)
                    build(RequestContext)  (1 usage)
                        (97, 34) for (Iterator fit = root.getFragments().iterator(); fit.hasNext();)
            org.apache.jetspeed.decoration  (1 usage)
                PageTheme  (1 usage)
                    PageTheme(Page, DecorationFactory, RequestContext)  (1 usage)
                        (57, 43) Iterator fragments = rootFragment.getFragments().iterator();
            org.apache.jetspeed.layout.impl  (2 usages)
                AddPortletAction  (1 usage)
                    run(RequestContext, Map)  (1 usage)
>>>>                        (126, 18) root.getFragments().add(fragment);            
                PortletPlacementContextImpl  (1 usage)
                    processFragment(Fragment)  (1 usage)
                        (145, 29) List children = fragment.getFragments();
        jetspeed-registry  (1 usage)
            org.apache.jetspeed.components.portletentity  (1 usage)
                TestPortletEntityDAO.ContentFragmentImpl  (1 usage)
                    getFragments()  (1 usage)
                        (145, 22) return f.getFragments();


-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


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


Re: [jira] Commented: (JS2-473) Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid

Posted by Randy Watler <wa...@wispertel.net>.
David,

David Jencks wrote:
>
> On Jan 22, 2006, at 6:59 PM, Randy Watler (JIRA) wrote:
>
>>     [ 
>> http://issues.apache.org/jira/browse/JS2-473?page=comments#action_12363599 
>> ]
>>
>> Randy Watler commented on JS2-473:
>> ----------------------------------
>>
>> Yes, this is a bug as you read in the comment... constraints and 
>> permissions checks on individual fragments are relatively new. We 
>> made that change too close to the 2.0 cutoff, so we were not in the 
>> mood to change the PageManager API then... :-).
>
> I hope I am not demonstrating "fools rush in where angels fear to 
> tread" :-)
Nah... it is time to get this stuff fixed...
>
>>
>> I am fine with the proposed API change, but I cannot read the diffs 
>> very well. I am concerned about the DB version of the FragmentImpl 
>> since it appears you moved functionality into the constructor of this 
>> persistent class. I think we should probably go ahead and apply the 
>> patch when others get s few moments to comment. Then, I can more 
>> propery review the changes.
>
> In general I am concerned about the thread safety of a lot of the code 
> I've looked at.  In the DB FragmentImpl a list was being created 
> lazily on access, with no synchronization.  I moved list construction 
> into the constructor, rather than write a new method to lazily create 
> the list.  It's still not thread safe for modifications, but at least 
> everyone will get the same copy of the list.  If all these classes are 
> only accessed by a single thread, that fact would be useful to 
> document somewhere easy to find.
Thread safety is an issue in the DB PM code. It is very new and somewhat 
immature. I am going to add a separate JIRA issue so that it does not 
get forgotten. Thanks for pointing it out!

Randy


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


Re: [jira] Commented: (JS2-473) Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid

Posted by David Jencks <da...@yahoo.com>.
On Jan 22, 2006, at 6:59 PM, Randy Watler (JIRA) wrote:

>     [ http://issues.apache.org/jira/browse/JS2-473? 
> page=comments#action_12363599 ]
>
> Randy Watler commented on JS2-473:
> ----------------------------------
>
> Yes, this is a bug as you read in the comment... constraints and  
> permissions checks on individual fragments are relatively new. We  
> made that change too close to the 2.0 cutoff, so we were not in the  
> mood to change the PageManager API then... :-).

I hope I am not demonstrating "fools rush in where angels fear to  
tread" :-)
>
> I am fine with the proposed API change, but I cannot read the diffs  
> very well. I am concerned about the DB version of the FragmentImpl  
> since it appears you moved functionality into the constructor of  
> this persistent class. I think we should probably go ahead and  
> apply the patch when others get s few moments to comment. Then, I  
> can more propery review the changes.

In general I am concerned about the thread safety of a lot of the  
code I've looked at.  In the DB FragmentImpl a list was being created  
lazily on access, with no synchronization.  I moved list construction  
into the constructor, rather than write a new method to lazily create  
the list.  It's still not thread safe for modifications, but at least  
everyone will get the same copy of the list.  If all these classes  
are only accessed by a single thread, that fact would be useful to  
document somewhere easy to find.

thanks
david jencks

>
>
>> Many uses of Fragment.getFragments() assume access to the  
>> underlying list, not a copy: this is invalid
>> --------------------------------------------------------------------- 
>> ---------------------------------
>>
>>          Key: JS2-473
>>          URL: http://issues.apache.org/jira/browse/JS2-473
>>      Project: Jetspeed 2
>>         Type: Bug
>>     Versions: 2.1-dev
>>     Reporter: David Jencks
>>  Attachments: permissions-fragment2.diff, permissions-fragment2.jar
>>
>> While trying to see if my proposed solution to JS2-472 would break  
>> anything, I noticed that many uses of Fragment.getFragments()  
>> assume they are getting the original mutable list, and proceed to  
>> modify it.  Since they may get a copy, any modifications would be  
>> discarded.   Here's a usage search from intellij with some  
>> annotations: I've marked obviously dangerous uses with >>>>.   
>> There is no reason to suppose the other uses are safe without  
>> looking into them further.  I don't think I'm qualified to fix  
>> this without at least some guidance.
>> Method
>>     getFragments():List of interface  
>> org.apache.jetspeed.om.page.Fragment
>> Found usages  (70 usages)
>>     Unclassified usage  (70 usages)
>>         jetspeed-layouts  (5 usages)
>>             org.apache.jetspeed.portlets.layout  (5 usages)
>>                 LayoutPortlet  (2 usages)
>>                     addPortletToPage(String, String)  (1 usage)
>>>>>>                        (279, 18) root.getFragments().add 
>>>>>> (fragment);
>>                     removeFragment(String, String)  (1 usage)
>>>>>>                        (257, 18) root.getFragments().remove(f);
>>                 MultiColumnPortlet  (2 usages)
>>                     doView(RenderRequest, RenderResponse)  (1 usage)
>>                         (94, 65) layout = new ColumnLayout 
>> (numColumns, layoutType, f.getFragments(), this.colSizes.split("\ 
>> \,") );
>>                     processAction(ActionRequest, ActionResponse)   
>> (1 usage)
>>                         (242, 80) layout = new ColumnLayout 
>> (numColumns, layoutType, rootFragment.getFragments(),  
>> this.colSizes.split("\\,") );
>>                 PageManagerLayoutEventListener  (1 usage)
>>                     handleEvent(LayoutEvent)  (1 usage)
>>>>>>                        (28, 40) page.getRootFragment 
>>>>>> ().getFragments().add(event.getFragment());
>>         jetspeed-page-manager  (60 usages)
>>             org.apache.jetspeed.om.page  (15 usages)
>>                 ContentFragmentImpl.ContentFragmentList  (1 usage)
>>                     (441, 42) private List baseList =  
>> fragment.getFragments();
>>                 TestPageObjectModel  (14 usages)
>>                     testFragmentManipulation()  (14 usages)
>>                         (108, 28) assertNotNull(root.getFragments());
>>>>>>                        (114, 14) root.getFragments().add(frag1);
>>>>>>                        (128, 15) frag2.getFragments().add(frag3);
>>>>>>                        (129, 14) root.getFragments().add(frag2);
>>                         (132, 25) assertTrue(root.getFragments 
>> ().size()==2);
>>                         (133, 27) Iterator i = root.getFragments 
>> ().iterator();
>>                         (142, 22) assertTrue(f.getFragments().size 
>> ()==0);
>>                         (147, 22) assertTrue(f.getFragments().size 
>> ()==1);
>>                         (149, 22) assertTrue(f.getFragments().size 
>> ()==1);
>>                         (150, 15) i = f.getFragments().iterator();
>>>>>>                        (164, 11) f.getFragments().remove(frag3);
>>>>>>                        (167, 11) f.getFragments().add(frag2);
>>                         (168, 22) assertTrue(f.getFragments().size 
>> ()==1);
>>                         (169, 29) f = (FragmentImpl)f.getFragments 
>> ().get(0);
>>             org.apache.jetspeed.om.page.psml  (4 usages)
>>                 PageImpl  (4 usages)
>>                     getFragmentById(String)  (1 usage)
>>                         (177, 28) Iterator i = f.getFragments 
>> ().iterator();
>>                     getFragmentsByName(String)  (1 usage)
>>                         (276, 28) Iterator i = f.getFragments 
>> ().iterator();
>>                     removeFragmentById(String)  (2 usages)
>>                         (209, 28) Iterator i = f.getFragments 
>> ().iterator();
>>>>>>                        (234, 28) if (parent.getFragments 
>>>>>> ().remove(f))
>>             org.apache.jetspeed.page  (41 usages)
>>                 AbstractPageManager  (2 usages)
>>                     copyFragment(Fragment, String)  (2 usages)
>>                         (889, 37) Iterator fragments =  
>> source.getFragments().iterator();
>>>>>>                        (894, 18) copy.getFragments().add 
>>>>>> (copiedFragment);
>>                 PageManagerTestShared.Shared  (15 usages)
>>                     testSecurePageManager(TestCase, PageManager)   
>> (15 usages)
>>>>>>                        (281, 34) root.getFragments().add 
>>>>>> (portlet);
>>>>>>                        (287, 34) root.getFragments().add 
>>>>>> (portlet);
>>                         (290, 71) test.assertNotNull 
>> (page.getRootFragment().getFragments());
>>                         (291, 73) test.assertEquals(2,  
>> page.getRootFragment().getFragments().size());
>>                         (292, 106) test.assertEquals("some- 
>> app::SomePortlet", ((Fragment)page.getRootFragment().getFragments 
>> ().get(1)).getName());
>>                         (293, 91) test.assertFalse("0".equals 
>> (((Fragment)page.getRootFragment().getFragments().get(1)).getId()));
>>                         (294, 82) somePortletId[0] = ((Fragment) 
>> page.getRootFragment().getFragments().get(1)).getId();
>>                         (348, 72) test.assertNotNull 
>> (page0.getRootFragment().getFragments());
>>                         (349, 74) test.assertEquals(2,  
>> page0.getRootFragment().getFragments().size());
>>                         (389, 72) test.assertNotNull 
>> (page0.getRootFragment().getFragments());
>>                         (390, 74) test.assertEquals(2,  
>> page0.getRootFragment().getFragments().size());
>>                         (458, 72) test.assertNotNull 
>> (page0.getRootFragment().getFragments());
>>                         (459, 74) test.assertEquals(1,  
>> page0.getRootFragment().getFragments().size());
>>                         (518, 72) test.assertNotNull 
>> (page0.getRootFragment().getFragments());
>>                         (519, 74) test.assertEquals(1,  
>> page0.getRootFragment().getFragments().size());
>>                 TestCastorXmlPageManager  (15 usages)
>>                     testClonePage()  (4 usages)
>>                         (861, 30) List children = root.getFragments 
>> ();
>>                         (862, 40) List cloneChildren =  
>> cloneRoot.getFragments();
>>                         (905, 26) assertNotNull(cf.getFragments());
>>                         (906, 23) assertTrue(cf.getFragments().size 
>> () == 2);
>>                     testCreatePage()  (3 usages)
>>>>>>                        (266, 14) root.getFragments().add(f);
>>                         (302, 43) assertTrue(page.getRootFragment 
>> ().getFragments().size() == 1);
>>                         (304, 47) f = (Fragment)  
>> page.getRootFragment().getFragments().get(0);
>>                     testGetPage()  (3 usages)
>>                         (190, 30) List children = root.getFragments 
>> ();
>>                         (237, 25) assertNotNull(f.getFragments());
>>                         (238, 22) assertTrue(f.getFragments().size 
>> () == 2);
>>                     testUpdatePage()  (5 usages)
>>                         (373, 28) assertNotNull(root.getFragments());
>>                         (374, 30) assertEquals(1, root.getFragments 
>> ().size());
>>                         (375, 41) String testId = ((Fragment) 
>> root.getFragments().get(0)).getId();
>>                         (394, 28) assertNotNull(root.getFragments());
>>                         (395, 25) assertTrue(root.getFragments 
>> ().isEmpty());
>>                 TestDatabasePageManager  (9 usages)
>>                     testCreates()  (2 usages)
>>>>>>                        (317, 14) root.getFragments().add 
>>>>>> (portlet);
>>>>>>                        (328, 14) root.getFragments().add 
>>>>>> (portlet);
>>                     testGets()  (4 usages)
>>                         (620, 51) assertNotNull 
>> (check.getRootFragment().getFragments());
>>                         (621, 53) assertEquals(2,  
>> check.getRootFragment().getFragments().size());
>>                         (622, 65) Fragment check0 = (Fragment) 
>> check.getRootFragment().getFragments().get(0);
>>                         (643, 65) Fragment check1 = (Fragment) 
>> check.getRootFragment().getFragments().get(1);
>>                     testUpdates()  (3 usages)
>>                         (814, 46) assertNotNull 
>> (page.getRootFragment().getFragments());
>>                         (815, 48) assertEquals(2,  
>> page.getRootFragment().getFragments().size());
>>                         (816, 61) String removeId = ((Fragment) 
>> page.getRootFragment().getFragments().get(1)).getId();
>>         jetspeed-portal  (4 usages)
>>             org.apache.jetspeed.aggregator.impl  (1 usage)
>>                 BasicAggregator  (1 usage)
>>                     build(RequestContext)  (1 usage)
>>                         (97, 34) for (Iterator fit =  
>> root.getFragments().iterator(); fit.hasNext();)
>>             org.apache.jetspeed.decoration  (1 usage)
>>                 PageTheme  (1 usage)
>>                     PageTheme(Page, DecorationFactory,  
>> RequestContext)  (1 usage)
>>                         (57, 43) Iterator fragments =  
>> rootFragment.getFragments().iterator();
>>             org.apache.jetspeed.layout.impl  (2 usages)
>>                 AddPortletAction  (1 usage)
>>                     run(RequestContext, Map)  (1 usage)
>>>>>>                        (126, 18) root.getFragments().add 
>>>>>> (fragment);
>>                 PortletPlacementContextImpl  (1 usage)
>>                     processFragment(Fragment)  (1 usage)
>>                         (145, 29) List children =  
>> fragment.getFragments();
>>         jetspeed-registry  (1 usage)
>>             org.apache.jetspeed.components.portletentity  (1 usage)
>>                 TestPortletEntityDAO.ContentFragmentImpl  (1 usage)
>>                     getFragments()  (1 usage)
>>                         (145, 22) return f.getFragments();
>
> -- 
> This message is automatically generated by JIRA.
> -
> If you think it was sent incorrectly contact one of the  
> administrators:
>    http://issues.apache.org/jira/secure/Administrators.jspa
> -
> For more information on JIRA, see:
>    http://www.atlassian.com/software/jira
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
> For additional commands, e-mail: jetspeed-dev-help@portals.apache.org
>


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


[jira] Commented: (JS2-473) Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid

Posted by "Randy Watler (JIRA)" <je...@portals.apache.org>.
    [ http://issues.apache.org/jira/browse/JS2-473?page=comments#action_12363599 ] 

Randy Watler commented on JS2-473:
----------------------------------

Yes, this is a bug as you read in the comment... constraints and permissions checks on individual fragments are relatively new. We made that change too close to the 2.0 cutoff, so we were not in the mood to change the PageManager API then... :-).

I am fine with the proposed API change, but I cannot read the diffs very well. I am concerned about the DB version of the FragmentImpl since it appears you moved functionality into the constructor of this persistent class. I think we should probably go ahead and apply the patch when others get s few moments to comment. Then, I can more propery review the changes.


> Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid
> ------------------------------------------------------------------------------------------------------
>
>          Key: JS2-473
>          URL: http://issues.apache.org/jira/browse/JS2-473
>      Project: Jetspeed 2
>         Type: Bug
>     Versions: 2.1-dev
>     Reporter: David Jencks
>  Attachments: permissions-fragment2.diff, permissions-fragment2.jar
>
> While trying to see if my proposed solution to JS2-472 would break anything, I noticed that many uses of Fragment.getFragments() assume they are getting the original mutable list, and proceed to modify it.  Since they may get a copy, any modifications would be discarded.   Here's a usage search from intellij with some annotations: I've marked obviously dangerous uses with >>>>.  There is no reason to suppose the other uses are safe without looking into them further.  I don't think I'm qualified to fix this without at least some guidance.
> Method
>     getFragments():List of interface org.apache.jetspeed.om.page.Fragment
> Found usages  (70 usages)
>     Unclassified usage  (70 usages)
>         jetspeed-layouts  (5 usages)
>             org.apache.jetspeed.portlets.layout  (5 usages)
>                 LayoutPortlet  (2 usages)
>                     addPortletToPage(String, String)  (1 usage)
> >>>>                        (279, 18) root.getFragments().add(fragment);
>                     removeFragment(String, String)  (1 usage)
> >>>>                        (257, 18) root.getFragments().remove(f);
>                 MultiColumnPortlet  (2 usages)
>                     doView(RenderRequest, RenderResponse)  (1 usage)
>                         (94, 65) layout = new ColumnLayout(numColumns, layoutType, f.getFragments(), this.colSizes.split("\\,") );
>                     processAction(ActionRequest, ActionResponse)  (1 usage)
>                         (242, 80) layout = new ColumnLayout(numColumns, layoutType, rootFragment.getFragments(), this.colSizes.split("\\,") );
>                 PageManagerLayoutEventListener  (1 usage)
>                     handleEvent(LayoutEvent)  (1 usage)
> >>>>                        (28, 40) page.getRootFragment().getFragments().add(event.getFragment());
>         jetspeed-page-manager  (60 usages)
>             org.apache.jetspeed.om.page  (15 usages)
>                 ContentFragmentImpl.ContentFragmentList  (1 usage)
>                     (441, 42) private List baseList = fragment.getFragments();
>                 TestPageObjectModel  (14 usages)
>                     testFragmentManipulation()  (14 usages)
>                         (108, 28) assertNotNull(root.getFragments());
> >>>>                        (114, 14) root.getFragments().add(frag1);
> >>>>                        (128, 15) frag2.getFragments().add(frag3);
> >>>>                        (129, 14) root.getFragments().add(frag2);
>                         (132, 25) assertTrue(root.getFragments().size()==2);
>                         (133, 27) Iterator i = root.getFragments().iterator();
>                         (142, 22) assertTrue(f.getFragments().size()==0);
>                         (147, 22) assertTrue(f.getFragments().size()==1);
>                         (149, 22) assertTrue(f.getFragments().size()==1);
>                         (150, 15) i = f.getFragments().iterator();
> >>>>                        (164, 11) f.getFragments().remove(frag3);
> >>>>                        (167, 11) f.getFragments().add(frag2);
>                         (168, 22) assertTrue(f.getFragments().size()==1);
>                         (169, 29) f = (FragmentImpl)f.getFragments().get(0);
>             org.apache.jetspeed.om.page.psml  (4 usages)
>                 PageImpl  (4 usages)
>                     getFragmentById(String)  (1 usage)
>                         (177, 28) Iterator i = f.getFragments().iterator();
>                     getFragmentsByName(String)  (1 usage)
>                         (276, 28) Iterator i = f.getFragments().iterator();
>                     removeFragmentById(String)  (2 usages)
>                         (209, 28) Iterator i = f.getFragments().iterator();
> >>>>                        (234, 28) if (parent.getFragments().remove(f))
>             org.apache.jetspeed.page  (41 usages)
>                 AbstractPageManager  (2 usages)
>                     copyFragment(Fragment, String)  (2 usages)
>                         (889, 37) Iterator fragments = source.getFragments().iterator();
> >>>>                        (894, 18) copy.getFragments().add(copiedFragment);
>                 PageManagerTestShared.Shared  (15 usages)
>                     testSecurePageManager(TestCase, PageManager)  (15 usages)
> >>>>                        (281, 34) root.getFragments().add(portlet);
> >>>>                        (287, 34) root.getFragments().add(portlet);
>                         (290, 71) test.assertNotNull(page.getRootFragment().getFragments());
>                         (291, 73) test.assertEquals(2, page.getRootFragment().getFragments().size());
>                         (292, 106) test.assertEquals("some-app::SomePortlet", ((Fragment)page.getRootFragment().getFragments().get(1)).getName());
>                         (293, 91) test.assertFalse("0".equals(((Fragment)page.getRootFragment().getFragments().get(1)).getId()));
>                         (294, 82) somePortletId[0] = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>                         (348, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (349, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (389, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (390, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (458, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (459, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                         (518, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (519, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                 TestCastorXmlPageManager  (15 usages)
>                     testClonePage()  (4 usages)
>                         (861, 30) List children = root.getFragments();
>                         (862, 40) List cloneChildren = cloneRoot.getFragments();
>                         (905, 26) assertNotNull(cf.getFragments());
>                         (906, 23) assertTrue(cf.getFragments().size() == 2);
>                     testCreatePage()  (3 usages)
> >>>>                        (266, 14) root.getFragments().add(f);
>                         (302, 43) assertTrue(page.getRootFragment().getFragments().size() == 1);
>                         (304, 47) f = (Fragment) page.getRootFragment().getFragments().get(0);
>                     testGetPage()  (3 usages)
>                         (190, 30) List children = root.getFragments();
>                         (237, 25) assertNotNull(f.getFragments());
>                         (238, 22) assertTrue(f.getFragments().size() == 2);
>                     testUpdatePage()  (5 usages)
>                         (373, 28) assertNotNull(root.getFragments());
>                         (374, 30) assertEquals(1, root.getFragments().size());
>                         (375, 41) String testId = ((Fragment)root.getFragments().get(0)).getId();
>                         (394, 28) assertNotNull(root.getFragments());
>                         (395, 25) assertTrue(root.getFragments().isEmpty());
>                 TestDatabasePageManager  (9 usages)
>                     testCreates()  (2 usages)
> >>>>                        (317, 14) root.getFragments().add(portlet);
> >>>>                        (328, 14) root.getFragments().add(portlet);
>                     testGets()  (4 usages)
>                         (620, 51) assertNotNull(check.getRootFragment().getFragments());
>                         (621, 53) assertEquals(2, check.getRootFragment().getFragments().size());
>                         (622, 65) Fragment check0 = (Fragment)check.getRootFragment().getFragments().get(0);
>                         (643, 65) Fragment check1 = (Fragment)check.getRootFragment().getFragments().get(1);
>                     testUpdates()  (3 usages)
>                         (814, 46) assertNotNull(page.getRootFragment().getFragments());
>                         (815, 48) assertEquals(2, page.getRootFragment().getFragments().size());
>                         (816, 61) String removeId = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>         jetspeed-portal  (4 usages)
>             org.apache.jetspeed.aggregator.impl  (1 usage)
>                 BasicAggregator  (1 usage)
>                     build(RequestContext)  (1 usage)
>                         (97, 34) for (Iterator fit = root.getFragments().iterator(); fit.hasNext();)
>             org.apache.jetspeed.decoration  (1 usage)
>                 PageTheme  (1 usage)
>                     PageTheme(Page, DecorationFactory, RequestContext)  (1 usage)
>                         (57, 43) Iterator fragments = rootFragment.getFragments().iterator();
>             org.apache.jetspeed.layout.impl  (2 usages)
>                 AddPortletAction  (1 usage)
>                     run(RequestContext, Map)  (1 usage)
> >>>>                        (126, 18) root.getFragments().add(fragment);            
>                 PortletPlacementContextImpl  (1 usage)
>                     processFragment(Fragment)  (1 usage)
>                         (145, 29) List children = fragment.getFragments();
>         jetspeed-registry  (1 usage)
>             org.apache.jetspeed.components.portletentity  (1 usage)
>                 TestPortletEntityDAO.ContentFragmentImpl  (1 usage)
>                     getFragments()  (1 usage)
>                         (145, 22) return f.getFragments();

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


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


[jira] Resolved: (JS2-473) Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid

Posted by "Randy Watler (JIRA)" <je...@portals.apache.org>.
     [ http://issues.apache.org/jira/browse/JS2-473?page=all ]
     
Randy Watler resolved JS2-473:
------------------------------

    Fix Version: 2.1-dev
     Resolution: Fixed
      Assign To: Randy Watler

This problem has been resolved with a lighter footprint fix that is compatible with the 2.0.1 release by preserving the getFragments() API, (i.e. by supportting a mutable collection in all cases).

The PageManager API has been ripe for a major refactoring for some time. Elimination of mutable collections should be considered then as an API design goal.

> Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid
> ------------------------------------------------------------------------------------------------------
>
>          Key: JS2-473
>          URL: http://issues.apache.org/jira/browse/JS2-473
>      Project: Jetspeed 2
>         Type: Bug
>     Versions: 2.1-dev
>     Reporter: David Jencks
>     Assignee: Randy Watler
>      Fix For: 2.1-dev
>  Attachments: permissions-fragment2.diff, permissions-fragment2.jar
>
> While trying to see if my proposed solution to JS2-472 would break anything, I noticed that many uses of Fragment.getFragments() assume they are getting the original mutable list, and proceed to modify it.  Since they may get a copy, any modifications would be discarded.   Here's a usage search from intellij with some annotations: I've marked obviously dangerous uses with >>>>.  There is no reason to suppose the other uses are safe without looking into them further.  I don't think I'm qualified to fix this without at least some guidance.
> Method
>     getFragments():List of interface org.apache.jetspeed.om.page.Fragment
> Found usages  (70 usages)
>     Unclassified usage  (70 usages)
>         jetspeed-layouts  (5 usages)
>             org.apache.jetspeed.portlets.layout  (5 usages)
>                 LayoutPortlet  (2 usages)
>                     addPortletToPage(String, String)  (1 usage)
> >>>>                        (279, 18) root.getFragments().add(fragment);
>                     removeFragment(String, String)  (1 usage)
> >>>>                        (257, 18) root.getFragments().remove(f);
>                 MultiColumnPortlet  (2 usages)
>                     doView(RenderRequest, RenderResponse)  (1 usage)
>                         (94, 65) layout = new ColumnLayout(numColumns, layoutType, f.getFragments(), this.colSizes.split("\\,") );
>                     processAction(ActionRequest, ActionResponse)  (1 usage)
>                         (242, 80) layout = new ColumnLayout(numColumns, layoutType, rootFragment.getFragments(), this.colSizes.split("\\,") );
>                 PageManagerLayoutEventListener  (1 usage)
>                     handleEvent(LayoutEvent)  (1 usage)
> >>>>                        (28, 40) page.getRootFragment().getFragments().add(event.getFragment());
>         jetspeed-page-manager  (60 usages)
>             org.apache.jetspeed.om.page  (15 usages)
>                 ContentFragmentImpl.ContentFragmentList  (1 usage)
>                     (441, 42) private List baseList = fragment.getFragments();
>                 TestPageObjectModel  (14 usages)
>                     testFragmentManipulation()  (14 usages)
>                         (108, 28) assertNotNull(root.getFragments());
> >>>>                        (114, 14) root.getFragments().add(frag1);
> >>>>                        (128, 15) frag2.getFragments().add(frag3);
> >>>>                        (129, 14) root.getFragments().add(frag2);
>                         (132, 25) assertTrue(root.getFragments().size()==2);
>                         (133, 27) Iterator i = root.getFragments().iterator();
>                         (142, 22) assertTrue(f.getFragments().size()==0);
>                         (147, 22) assertTrue(f.getFragments().size()==1);
>                         (149, 22) assertTrue(f.getFragments().size()==1);
>                         (150, 15) i = f.getFragments().iterator();
> >>>>                        (164, 11) f.getFragments().remove(frag3);
> >>>>                        (167, 11) f.getFragments().add(frag2);
>                         (168, 22) assertTrue(f.getFragments().size()==1);
>                         (169, 29) f = (FragmentImpl)f.getFragments().get(0);
>             org.apache.jetspeed.om.page.psml  (4 usages)
>                 PageImpl  (4 usages)
>                     getFragmentById(String)  (1 usage)
>                         (177, 28) Iterator i = f.getFragments().iterator();
>                     getFragmentsByName(String)  (1 usage)
>                         (276, 28) Iterator i = f.getFragments().iterator();
>                     removeFragmentById(String)  (2 usages)
>                         (209, 28) Iterator i = f.getFragments().iterator();
> >>>>                        (234, 28) if (parent.getFragments().remove(f))
>             org.apache.jetspeed.page  (41 usages)
>                 AbstractPageManager  (2 usages)
>                     copyFragment(Fragment, String)  (2 usages)
>                         (889, 37) Iterator fragments = source.getFragments().iterator();
> >>>>                        (894, 18) copy.getFragments().add(copiedFragment);
>                 PageManagerTestShared.Shared  (15 usages)
>                     testSecurePageManager(TestCase, PageManager)  (15 usages)
> >>>>                        (281, 34) root.getFragments().add(portlet);
> >>>>                        (287, 34) root.getFragments().add(portlet);
>                         (290, 71) test.assertNotNull(page.getRootFragment().getFragments());
>                         (291, 73) test.assertEquals(2, page.getRootFragment().getFragments().size());
>                         (292, 106) test.assertEquals("some-app::SomePortlet", ((Fragment)page.getRootFragment().getFragments().get(1)).getName());
>                         (293, 91) test.assertFalse("0".equals(((Fragment)page.getRootFragment().getFragments().get(1)).getId()));
>                         (294, 82) somePortletId[0] = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>                         (348, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (349, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (389, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (390, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (458, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (459, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                         (518, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (519, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                 TestCastorXmlPageManager  (15 usages)
>                     testClonePage()  (4 usages)
>                         (861, 30) List children = root.getFragments();
>                         (862, 40) List cloneChildren = cloneRoot.getFragments();
>                         (905, 26) assertNotNull(cf.getFragments());
>                         (906, 23) assertTrue(cf.getFragments().size() == 2);
>                     testCreatePage()  (3 usages)
> >>>>                        (266, 14) root.getFragments().add(f);
>                         (302, 43) assertTrue(page.getRootFragment().getFragments().size() == 1);
>                         (304, 47) f = (Fragment) page.getRootFragment().getFragments().get(0);
>                     testGetPage()  (3 usages)
>                         (190, 30) List children = root.getFragments();
>                         (237, 25) assertNotNull(f.getFragments());
>                         (238, 22) assertTrue(f.getFragments().size() == 2);
>                     testUpdatePage()  (5 usages)
>                         (373, 28) assertNotNull(root.getFragments());
>                         (374, 30) assertEquals(1, root.getFragments().size());
>                         (375, 41) String testId = ((Fragment)root.getFragments().get(0)).getId();
>                         (394, 28) assertNotNull(root.getFragments());
>                         (395, 25) assertTrue(root.getFragments().isEmpty());
>                 TestDatabasePageManager  (9 usages)
>                     testCreates()  (2 usages)
> >>>>                        (317, 14) root.getFragments().add(portlet);
> >>>>                        (328, 14) root.getFragments().add(portlet);
>                     testGets()  (4 usages)
>                         (620, 51) assertNotNull(check.getRootFragment().getFragments());
>                         (621, 53) assertEquals(2, check.getRootFragment().getFragments().size());
>                         (622, 65) Fragment check0 = (Fragment)check.getRootFragment().getFragments().get(0);
>                         (643, 65) Fragment check1 = (Fragment)check.getRootFragment().getFragments().get(1);
>                     testUpdates()  (3 usages)
>                         (814, 46) assertNotNull(page.getRootFragment().getFragments());
>                         (815, 48) assertEquals(2, page.getRootFragment().getFragments().size());
>                         (816, 61) String removeId = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>         jetspeed-portal  (4 usages)
>             org.apache.jetspeed.aggregator.impl  (1 usage)
>                 BasicAggregator  (1 usage)
>                     build(RequestContext)  (1 usage)
>                         (97, 34) for (Iterator fit = root.getFragments().iterator(); fit.hasNext();)
>             org.apache.jetspeed.decoration  (1 usage)
>                 PageTheme  (1 usage)
>                     PageTheme(Page, DecorationFactory, RequestContext)  (1 usage)
>                         (57, 43) Iterator fragments = rootFragment.getFragments().iterator();
>             org.apache.jetspeed.layout.impl  (2 usages)
>                 AddPortletAction  (1 usage)
>                     run(RequestContext, Map)  (1 usage)
> >>>>                        (126, 18) root.getFragments().add(fragment);            
>                 PortletPlacementContextImpl  (1 usage)
>                     processFragment(Fragment)  (1 usage)
>                         (145, 29) List children = fragment.getFragments();
>         jetspeed-registry  (1 usage)
>             org.apache.jetspeed.components.portletentity  (1 usage)
>                 TestPortletEntityDAO.ContentFragmentImpl  (1 usage)
>                     getFragments()  (1 usage)
>                         (145, 22) return f.getFragments();

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


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


[jira] Updated: (JS2-473) Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid

Posted by "David Jencks (JIRA)" <je...@portals.apache.org>.
     [ http://issues.apache.org/jira/browse/JS2-473?page=all ]

David Jencks updated JS2-473:
-----------------------------

    Attachment: permissions-fragment2.jar
                permissions-fragment2.diff

I've implemented a possible solution for this by adding methods to the Fragment interface:
addFragment
removeFragment
setFragments

However, almost all the files involved were already changed in my jetspeed copy from the work on JS2-475.  I'm attaching files that are modified from my first patch to JS2-475 to this issue as an svk diff and the entire files.  I will attach full diffs as well to JS2-475.  If JS2-475 is applied I can try to make a diff for this issue only.  Comparing the diffs for JS2-475 and this issue will show what is actually changed for this issue.

> Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid
> ------------------------------------------------------------------------------------------------------
>
>          Key: JS2-473
>          URL: http://issues.apache.org/jira/browse/JS2-473
>      Project: Jetspeed 2
>         Type: Bug
>     Versions: 2.1-dev
>     Reporter: David Jencks
>  Attachments: permissions-fragment2.diff, permissions-fragment2.jar
>
> While trying to see if my proposed solution to JS2-472 would break anything, I noticed that many uses of Fragment.getFragments() assume they are getting the original mutable list, and proceed to modify it.  Since they may get a copy, any modifications would be discarded.   Here's a usage search from intellij with some annotations: I've marked obviously dangerous uses with >>>>.  There is no reason to suppose the other uses are safe without looking into them further.  I don't think I'm qualified to fix this without at least some guidance.
> Method
>     getFragments():List of interface org.apache.jetspeed.om.page.Fragment
> Found usages  (70 usages)
>     Unclassified usage  (70 usages)
>         jetspeed-layouts  (5 usages)
>             org.apache.jetspeed.portlets.layout  (5 usages)
>                 LayoutPortlet  (2 usages)
>                     addPortletToPage(String, String)  (1 usage)
> >>>>                        (279, 18) root.getFragments().add(fragment);
>                     removeFragment(String, String)  (1 usage)
> >>>>                        (257, 18) root.getFragments().remove(f);
>                 MultiColumnPortlet  (2 usages)
>                     doView(RenderRequest, RenderResponse)  (1 usage)
>                         (94, 65) layout = new ColumnLayout(numColumns, layoutType, f.getFragments(), this.colSizes.split("\\,") );
>                     processAction(ActionRequest, ActionResponse)  (1 usage)
>                         (242, 80) layout = new ColumnLayout(numColumns, layoutType, rootFragment.getFragments(), this.colSizes.split("\\,") );
>                 PageManagerLayoutEventListener  (1 usage)
>                     handleEvent(LayoutEvent)  (1 usage)
> >>>>                        (28, 40) page.getRootFragment().getFragments().add(event.getFragment());
>         jetspeed-page-manager  (60 usages)
>             org.apache.jetspeed.om.page  (15 usages)
>                 ContentFragmentImpl.ContentFragmentList  (1 usage)
>                     (441, 42) private List baseList = fragment.getFragments();
>                 TestPageObjectModel  (14 usages)
>                     testFragmentManipulation()  (14 usages)
>                         (108, 28) assertNotNull(root.getFragments());
> >>>>                        (114, 14) root.getFragments().add(frag1);
> >>>>                        (128, 15) frag2.getFragments().add(frag3);
> >>>>                        (129, 14) root.getFragments().add(frag2);
>                         (132, 25) assertTrue(root.getFragments().size()==2);
>                         (133, 27) Iterator i = root.getFragments().iterator();
>                         (142, 22) assertTrue(f.getFragments().size()==0);
>                         (147, 22) assertTrue(f.getFragments().size()==1);
>                         (149, 22) assertTrue(f.getFragments().size()==1);
>                         (150, 15) i = f.getFragments().iterator();
> >>>>                        (164, 11) f.getFragments().remove(frag3);
> >>>>                        (167, 11) f.getFragments().add(frag2);
>                         (168, 22) assertTrue(f.getFragments().size()==1);
>                         (169, 29) f = (FragmentImpl)f.getFragments().get(0);
>             org.apache.jetspeed.om.page.psml  (4 usages)
>                 PageImpl  (4 usages)
>                     getFragmentById(String)  (1 usage)
>                         (177, 28) Iterator i = f.getFragments().iterator();
>                     getFragmentsByName(String)  (1 usage)
>                         (276, 28) Iterator i = f.getFragments().iterator();
>                     removeFragmentById(String)  (2 usages)
>                         (209, 28) Iterator i = f.getFragments().iterator();
> >>>>                        (234, 28) if (parent.getFragments().remove(f))
>             org.apache.jetspeed.page  (41 usages)
>                 AbstractPageManager  (2 usages)
>                     copyFragment(Fragment, String)  (2 usages)
>                         (889, 37) Iterator fragments = source.getFragments().iterator();
> >>>>                        (894, 18) copy.getFragments().add(copiedFragment);
>                 PageManagerTestShared.Shared  (15 usages)
>                     testSecurePageManager(TestCase, PageManager)  (15 usages)
> >>>>                        (281, 34) root.getFragments().add(portlet);
> >>>>                        (287, 34) root.getFragments().add(portlet);
>                         (290, 71) test.assertNotNull(page.getRootFragment().getFragments());
>                         (291, 73) test.assertEquals(2, page.getRootFragment().getFragments().size());
>                         (292, 106) test.assertEquals("some-app::SomePortlet", ((Fragment)page.getRootFragment().getFragments().get(1)).getName());
>                         (293, 91) test.assertFalse("0".equals(((Fragment)page.getRootFragment().getFragments().get(1)).getId()));
>                         (294, 82) somePortletId[0] = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>                         (348, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (349, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (389, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (390, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (458, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (459, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                         (518, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (519, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                 TestCastorXmlPageManager  (15 usages)
>                     testClonePage()  (4 usages)
>                         (861, 30) List children = root.getFragments();
>                         (862, 40) List cloneChildren = cloneRoot.getFragments();
>                         (905, 26) assertNotNull(cf.getFragments());
>                         (906, 23) assertTrue(cf.getFragments().size() == 2);
>                     testCreatePage()  (3 usages)
> >>>>                        (266, 14) root.getFragments().add(f);
>                         (302, 43) assertTrue(page.getRootFragment().getFragments().size() == 1);
>                         (304, 47) f = (Fragment) page.getRootFragment().getFragments().get(0);
>                     testGetPage()  (3 usages)
>                         (190, 30) List children = root.getFragments();
>                         (237, 25) assertNotNull(f.getFragments());
>                         (238, 22) assertTrue(f.getFragments().size() == 2);
>                     testUpdatePage()  (5 usages)
>                         (373, 28) assertNotNull(root.getFragments());
>                         (374, 30) assertEquals(1, root.getFragments().size());
>                         (375, 41) String testId = ((Fragment)root.getFragments().get(0)).getId();
>                         (394, 28) assertNotNull(root.getFragments());
>                         (395, 25) assertTrue(root.getFragments().isEmpty());
>                 TestDatabasePageManager  (9 usages)
>                     testCreates()  (2 usages)
> >>>>                        (317, 14) root.getFragments().add(portlet);
> >>>>                        (328, 14) root.getFragments().add(portlet);
>                     testGets()  (4 usages)
>                         (620, 51) assertNotNull(check.getRootFragment().getFragments());
>                         (621, 53) assertEquals(2, check.getRootFragment().getFragments().size());
>                         (622, 65) Fragment check0 = (Fragment)check.getRootFragment().getFragments().get(0);
>                         (643, 65) Fragment check1 = (Fragment)check.getRootFragment().getFragments().get(1);
>                     testUpdates()  (3 usages)
>                         (814, 46) assertNotNull(page.getRootFragment().getFragments());
>                         (815, 48) assertEquals(2, page.getRootFragment().getFragments().size());
>                         (816, 61) String removeId = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>         jetspeed-portal  (4 usages)
>             org.apache.jetspeed.aggregator.impl  (1 usage)
>                 BasicAggregator  (1 usage)
>                     build(RequestContext)  (1 usage)
>                         (97, 34) for (Iterator fit = root.getFragments().iterator(); fit.hasNext();)
>             org.apache.jetspeed.decoration  (1 usage)
>                 PageTheme  (1 usage)
>                     PageTheme(Page, DecorationFactory, RequestContext)  (1 usage)
>                         (57, 43) Iterator fragments = rootFragment.getFragments().iterator();
>             org.apache.jetspeed.layout.impl  (2 usages)
>                 AddPortletAction  (1 usage)
>                     run(RequestContext, Map)  (1 usage)
> >>>>                        (126, 18) root.getFragments().add(fragment);            
>                 PortletPlacementContextImpl  (1 usage)
>                     processFragment(Fragment)  (1 usage)
>                         (145, 29) List children = fragment.getFragments();
>         jetspeed-registry  (1 usage)
>             org.apache.jetspeed.components.portletentity  (1 usage)
>                 TestPortletEntityDAO.ContentFragmentImpl  (1 usage)
>                     getFragments()  (1 usage)
>                         (145, 22) return f.getFragments();

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


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


[jira] Commented: (JS2-473) Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid

Posted by "Randy Watler (JIRA)" <je...@portals.apache.org>.
    [ http://issues.apache.org/jira/browse/JS2-473?page=comments#action_12363783 ] 

Randy Watler commented on JS2-473:
----------------------------------

I am still pondering this fix. I think there is an easy way to fix this w/o changing the API. I'll keep you posted.

> Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid
> ------------------------------------------------------------------------------------------------------
>
>          Key: JS2-473
>          URL: http://issues.apache.org/jira/browse/JS2-473
>      Project: Jetspeed 2
>         Type: Bug
>     Versions: 2.1-dev
>     Reporter: David Jencks
>  Attachments: permissions-fragment2.diff, permissions-fragment2.jar
>
> While trying to see if my proposed solution to JS2-472 would break anything, I noticed that many uses of Fragment.getFragments() assume they are getting the original mutable list, and proceed to modify it.  Since they may get a copy, any modifications would be discarded.   Here's a usage search from intellij with some annotations: I've marked obviously dangerous uses with >>>>.  There is no reason to suppose the other uses are safe without looking into them further.  I don't think I'm qualified to fix this without at least some guidance.
> Method
>     getFragments():List of interface org.apache.jetspeed.om.page.Fragment
> Found usages  (70 usages)
>     Unclassified usage  (70 usages)
>         jetspeed-layouts  (5 usages)
>             org.apache.jetspeed.portlets.layout  (5 usages)
>                 LayoutPortlet  (2 usages)
>                     addPortletToPage(String, String)  (1 usage)
> >>>>                        (279, 18) root.getFragments().add(fragment);
>                     removeFragment(String, String)  (1 usage)
> >>>>                        (257, 18) root.getFragments().remove(f);
>                 MultiColumnPortlet  (2 usages)
>                     doView(RenderRequest, RenderResponse)  (1 usage)
>                         (94, 65) layout = new ColumnLayout(numColumns, layoutType, f.getFragments(), this.colSizes.split("\\,") );
>                     processAction(ActionRequest, ActionResponse)  (1 usage)
>                         (242, 80) layout = new ColumnLayout(numColumns, layoutType, rootFragment.getFragments(), this.colSizes.split("\\,") );
>                 PageManagerLayoutEventListener  (1 usage)
>                     handleEvent(LayoutEvent)  (1 usage)
> >>>>                        (28, 40) page.getRootFragment().getFragments().add(event.getFragment());
>         jetspeed-page-manager  (60 usages)
>             org.apache.jetspeed.om.page  (15 usages)
>                 ContentFragmentImpl.ContentFragmentList  (1 usage)
>                     (441, 42) private List baseList = fragment.getFragments();
>                 TestPageObjectModel  (14 usages)
>                     testFragmentManipulation()  (14 usages)
>                         (108, 28) assertNotNull(root.getFragments());
> >>>>                        (114, 14) root.getFragments().add(frag1);
> >>>>                        (128, 15) frag2.getFragments().add(frag3);
> >>>>                        (129, 14) root.getFragments().add(frag2);
>                         (132, 25) assertTrue(root.getFragments().size()==2);
>                         (133, 27) Iterator i = root.getFragments().iterator();
>                         (142, 22) assertTrue(f.getFragments().size()==0);
>                         (147, 22) assertTrue(f.getFragments().size()==1);
>                         (149, 22) assertTrue(f.getFragments().size()==1);
>                         (150, 15) i = f.getFragments().iterator();
> >>>>                        (164, 11) f.getFragments().remove(frag3);
> >>>>                        (167, 11) f.getFragments().add(frag2);
>                         (168, 22) assertTrue(f.getFragments().size()==1);
>                         (169, 29) f = (FragmentImpl)f.getFragments().get(0);
>             org.apache.jetspeed.om.page.psml  (4 usages)
>                 PageImpl  (4 usages)
>                     getFragmentById(String)  (1 usage)
>                         (177, 28) Iterator i = f.getFragments().iterator();
>                     getFragmentsByName(String)  (1 usage)
>                         (276, 28) Iterator i = f.getFragments().iterator();
>                     removeFragmentById(String)  (2 usages)
>                         (209, 28) Iterator i = f.getFragments().iterator();
> >>>>                        (234, 28) if (parent.getFragments().remove(f))
>             org.apache.jetspeed.page  (41 usages)
>                 AbstractPageManager  (2 usages)
>                     copyFragment(Fragment, String)  (2 usages)
>                         (889, 37) Iterator fragments = source.getFragments().iterator();
> >>>>                        (894, 18) copy.getFragments().add(copiedFragment);
>                 PageManagerTestShared.Shared  (15 usages)
>                     testSecurePageManager(TestCase, PageManager)  (15 usages)
> >>>>                        (281, 34) root.getFragments().add(portlet);
> >>>>                        (287, 34) root.getFragments().add(portlet);
>                         (290, 71) test.assertNotNull(page.getRootFragment().getFragments());
>                         (291, 73) test.assertEquals(2, page.getRootFragment().getFragments().size());
>                         (292, 106) test.assertEquals("some-app::SomePortlet", ((Fragment)page.getRootFragment().getFragments().get(1)).getName());
>                         (293, 91) test.assertFalse("0".equals(((Fragment)page.getRootFragment().getFragments().get(1)).getId()));
>                         (294, 82) somePortletId[0] = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>                         (348, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (349, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (389, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (390, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (458, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (459, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                         (518, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (519, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                 TestCastorXmlPageManager  (15 usages)
>                     testClonePage()  (4 usages)
>                         (861, 30) List children = root.getFragments();
>                         (862, 40) List cloneChildren = cloneRoot.getFragments();
>                         (905, 26) assertNotNull(cf.getFragments());
>                         (906, 23) assertTrue(cf.getFragments().size() == 2);
>                     testCreatePage()  (3 usages)
> >>>>                        (266, 14) root.getFragments().add(f);
>                         (302, 43) assertTrue(page.getRootFragment().getFragments().size() == 1);
>                         (304, 47) f = (Fragment) page.getRootFragment().getFragments().get(0);
>                     testGetPage()  (3 usages)
>                         (190, 30) List children = root.getFragments();
>                         (237, 25) assertNotNull(f.getFragments());
>                         (238, 22) assertTrue(f.getFragments().size() == 2);
>                     testUpdatePage()  (5 usages)
>                         (373, 28) assertNotNull(root.getFragments());
>                         (374, 30) assertEquals(1, root.getFragments().size());
>                         (375, 41) String testId = ((Fragment)root.getFragments().get(0)).getId();
>                         (394, 28) assertNotNull(root.getFragments());
>                         (395, 25) assertTrue(root.getFragments().isEmpty());
>                 TestDatabasePageManager  (9 usages)
>                     testCreates()  (2 usages)
> >>>>                        (317, 14) root.getFragments().add(portlet);
> >>>>                        (328, 14) root.getFragments().add(portlet);
>                     testGets()  (4 usages)
>                         (620, 51) assertNotNull(check.getRootFragment().getFragments());
>                         (621, 53) assertEquals(2, check.getRootFragment().getFragments().size());
>                         (622, 65) Fragment check0 = (Fragment)check.getRootFragment().getFragments().get(0);
>                         (643, 65) Fragment check1 = (Fragment)check.getRootFragment().getFragments().get(1);
>                     testUpdates()  (3 usages)
>                         (814, 46) assertNotNull(page.getRootFragment().getFragments());
>                         (815, 48) assertEquals(2, page.getRootFragment().getFragments().size());
>                         (816, 61) String removeId = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>         jetspeed-portal  (4 usages)
>             org.apache.jetspeed.aggregator.impl  (1 usage)
>                 BasicAggregator  (1 usage)
>                     build(RequestContext)  (1 usage)
>                         (97, 34) for (Iterator fit = root.getFragments().iterator(); fit.hasNext();)
>             org.apache.jetspeed.decoration  (1 usage)
>                 PageTheme  (1 usage)
>                     PageTheme(Page, DecorationFactory, RequestContext)  (1 usage)
>                         (57, 43) Iterator fragments = rootFragment.getFragments().iterator();
>             org.apache.jetspeed.layout.impl  (2 usages)
>                 AddPortletAction  (1 usage)
>                     run(RequestContext, Map)  (1 usage)
> >>>>                        (126, 18) root.getFragments().add(fragment);            
>                 PortletPlacementContextImpl  (1 usage)
>                     processFragment(Fragment)  (1 usage)
>                         (145, 29) List children = fragment.getFragments();
>         jetspeed-registry  (1 usage)
>             org.apache.jetspeed.components.portletentity  (1 usage)
>                 TestPortletEntityDAO.ContentFragmentImpl  (1 usage)
>                     getFragments()  (1 usage)
>                         (145, 22) return f.getFragments();

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


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


[jira] Commented: (JS2-473) Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid

Posted by "David Jencks (JIRA)" <je...@portals.apache.org>.
    [ http://issues.apache.org/jira/browse/JS2-473?page=comments#action_12363377 ] 

David Jencks commented on JS2-473:
----------------------------------

On further thought, all or almost all of these problems can be solved by adding add and remove methods to Fragment that add and remove to the private fragment list, before security filtering.

> Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid
> ------------------------------------------------------------------------------------------------------
>
>          Key: JS2-473
>          URL: http://issues.apache.org/jira/browse/JS2-473
>      Project: Jetspeed 2
>         Type: Bug
>     Versions: 2.1-dev
>     Reporter: David Jencks

>
> While trying to see if my proposed solution to JS2-472 would break anything, I noticed that many uses of Fragment.getFragments() assume they are getting the original mutable list, and proceed to modify it.  Since they may get a copy, any modifications would be discarded.   Here's a usage search from intellij with some annotations: I've marked obviously dangerous uses with >>>>.  There is no reason to suppose the other uses are safe without looking into them further.  I don't think I'm qualified to fix this without at least some guidance.
> Method
>     getFragments():List of interface org.apache.jetspeed.om.page.Fragment
> Found usages  (70 usages)
>     Unclassified usage  (70 usages)
>         jetspeed-layouts  (5 usages)
>             org.apache.jetspeed.portlets.layout  (5 usages)
>                 LayoutPortlet  (2 usages)
>                     addPortletToPage(String, String)  (1 usage)
> >>>>                        (279, 18) root.getFragments().add(fragment);
>                     removeFragment(String, String)  (1 usage)
> >>>>                        (257, 18) root.getFragments().remove(f);
>                 MultiColumnPortlet  (2 usages)
>                     doView(RenderRequest, RenderResponse)  (1 usage)
>                         (94, 65) layout = new ColumnLayout(numColumns, layoutType, f.getFragments(), this.colSizes.split("\\,") );
>                     processAction(ActionRequest, ActionResponse)  (1 usage)
>                         (242, 80) layout = new ColumnLayout(numColumns, layoutType, rootFragment.getFragments(), this.colSizes.split("\\,") );
>                 PageManagerLayoutEventListener  (1 usage)
>                     handleEvent(LayoutEvent)  (1 usage)
> >>>>                        (28, 40) page.getRootFragment().getFragments().add(event.getFragment());
>         jetspeed-page-manager  (60 usages)
>             org.apache.jetspeed.om.page  (15 usages)
>                 ContentFragmentImpl.ContentFragmentList  (1 usage)
>                     (441, 42) private List baseList = fragment.getFragments();
>                 TestPageObjectModel  (14 usages)
>                     testFragmentManipulation()  (14 usages)
>                         (108, 28) assertNotNull(root.getFragments());
> >>>>                        (114, 14) root.getFragments().add(frag1);
> >>>>                        (128, 15) frag2.getFragments().add(frag3);
> >>>>                        (129, 14) root.getFragments().add(frag2);
>                         (132, 25) assertTrue(root.getFragments().size()==2);
>                         (133, 27) Iterator i = root.getFragments().iterator();
>                         (142, 22) assertTrue(f.getFragments().size()==0);
>                         (147, 22) assertTrue(f.getFragments().size()==1);
>                         (149, 22) assertTrue(f.getFragments().size()==1);
>                         (150, 15) i = f.getFragments().iterator();
> >>>>                        (164, 11) f.getFragments().remove(frag3);
> >>>>                        (167, 11) f.getFragments().add(frag2);
>                         (168, 22) assertTrue(f.getFragments().size()==1);
>                         (169, 29) f = (FragmentImpl)f.getFragments().get(0);
>             org.apache.jetspeed.om.page.psml  (4 usages)
>                 PageImpl  (4 usages)
>                     getFragmentById(String)  (1 usage)
>                         (177, 28) Iterator i = f.getFragments().iterator();
>                     getFragmentsByName(String)  (1 usage)
>                         (276, 28) Iterator i = f.getFragments().iterator();
>                     removeFragmentById(String)  (2 usages)
>                         (209, 28) Iterator i = f.getFragments().iterator();
> >>>>                        (234, 28) if (parent.getFragments().remove(f))
>             org.apache.jetspeed.page  (41 usages)
>                 AbstractPageManager  (2 usages)
>                     copyFragment(Fragment, String)  (2 usages)
>                         (889, 37) Iterator fragments = source.getFragments().iterator();
> >>>>                        (894, 18) copy.getFragments().add(copiedFragment);
>                 PageManagerTestShared.Shared  (15 usages)
>                     testSecurePageManager(TestCase, PageManager)  (15 usages)
> >>>>                        (281, 34) root.getFragments().add(portlet);
> >>>>                        (287, 34) root.getFragments().add(portlet);
>                         (290, 71) test.assertNotNull(page.getRootFragment().getFragments());
>                         (291, 73) test.assertEquals(2, page.getRootFragment().getFragments().size());
>                         (292, 106) test.assertEquals("some-app::SomePortlet", ((Fragment)page.getRootFragment().getFragments().get(1)).getName());
>                         (293, 91) test.assertFalse("0".equals(((Fragment)page.getRootFragment().getFragments().get(1)).getId()));
>                         (294, 82) somePortletId[0] = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>                         (348, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (349, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (389, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (390, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (458, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (459, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                         (518, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (519, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                 TestCastorXmlPageManager  (15 usages)
>                     testClonePage()  (4 usages)
>                         (861, 30) List children = root.getFragments();
>                         (862, 40) List cloneChildren = cloneRoot.getFragments();
>                         (905, 26) assertNotNull(cf.getFragments());
>                         (906, 23) assertTrue(cf.getFragments().size() == 2);
>                     testCreatePage()  (3 usages)
> >>>>                        (266, 14) root.getFragments().add(f);
>                         (302, 43) assertTrue(page.getRootFragment().getFragments().size() == 1);
>                         (304, 47) f = (Fragment) page.getRootFragment().getFragments().get(0);
>                     testGetPage()  (3 usages)
>                         (190, 30) List children = root.getFragments();
>                         (237, 25) assertNotNull(f.getFragments());
>                         (238, 22) assertTrue(f.getFragments().size() == 2);
>                     testUpdatePage()  (5 usages)
>                         (373, 28) assertNotNull(root.getFragments());
>                         (374, 30) assertEquals(1, root.getFragments().size());
>                         (375, 41) String testId = ((Fragment)root.getFragments().get(0)).getId();
>                         (394, 28) assertNotNull(root.getFragments());
>                         (395, 25) assertTrue(root.getFragments().isEmpty());
>                 TestDatabasePageManager  (9 usages)
>                     testCreates()  (2 usages)
> >>>>                        (317, 14) root.getFragments().add(portlet);
> >>>>                        (328, 14) root.getFragments().add(portlet);
>                     testGets()  (4 usages)
>                         (620, 51) assertNotNull(check.getRootFragment().getFragments());
>                         (621, 53) assertEquals(2, check.getRootFragment().getFragments().size());
>                         (622, 65) Fragment check0 = (Fragment)check.getRootFragment().getFragments().get(0);
>                         (643, 65) Fragment check1 = (Fragment)check.getRootFragment().getFragments().get(1);
>                     testUpdates()  (3 usages)
>                         (814, 46) assertNotNull(page.getRootFragment().getFragments());
>                         (815, 48) assertEquals(2, page.getRootFragment().getFragments().size());
>                         (816, 61) String removeId = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>         jetspeed-portal  (4 usages)
>             org.apache.jetspeed.aggregator.impl  (1 usage)
>                 BasicAggregator  (1 usage)
>                     build(RequestContext)  (1 usage)
>                         (97, 34) for (Iterator fit = root.getFragments().iterator(); fit.hasNext();)
>             org.apache.jetspeed.decoration  (1 usage)
>                 PageTheme  (1 usage)
>                     PageTheme(Page, DecorationFactory, RequestContext)  (1 usage)
>                         (57, 43) Iterator fragments = rootFragment.getFragments().iterator();
>             org.apache.jetspeed.layout.impl  (2 usages)
>                 AddPortletAction  (1 usage)
>                     run(RequestContext, Map)  (1 usage)
> >>>>                        (126, 18) root.getFragments().add(fragment);            
>                 PortletPlacementContextImpl  (1 usage)
>                     processFragment(Fragment)  (1 usage)
>                         (145, 29) List children = fragment.getFragments();
>         jetspeed-registry  (1 usage)
>             org.apache.jetspeed.components.portletentity  (1 usage)
>                 TestPortletEntityDAO.ContentFragmentImpl  (1 usage)
>                     getFragments()  (1 usage)
>                         (145, 22) return f.getFragments();

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


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


[jira] Commented: (JS2-473) Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid

Posted by "David Jencks (JIRA)" <je...@portals.apache.org>.
    [ http://issues.apache.org/jira/browse/JS2-473?page=comments#action_12363748 ] 

David Jencks commented on JS2-473:
----------------------------------

Latest version of these changes in JS2-444 geronimo-jetspeed10.zip

> Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid
> ------------------------------------------------------------------------------------------------------
>
>          Key: JS2-473
>          URL: http://issues.apache.org/jira/browse/JS2-473
>      Project: Jetspeed 2
>         Type: Bug
>     Versions: 2.1-dev
>     Reporter: David Jencks
>  Attachments: permissions-fragment2.diff, permissions-fragment2.jar
>
> While trying to see if my proposed solution to JS2-472 would break anything, I noticed that many uses of Fragment.getFragments() assume they are getting the original mutable list, and proceed to modify it.  Since they may get a copy, any modifications would be discarded.   Here's a usage search from intellij with some annotations: I've marked obviously dangerous uses with >>>>.  There is no reason to suppose the other uses are safe without looking into them further.  I don't think I'm qualified to fix this without at least some guidance.
> Method
>     getFragments():List of interface org.apache.jetspeed.om.page.Fragment
> Found usages  (70 usages)
>     Unclassified usage  (70 usages)
>         jetspeed-layouts  (5 usages)
>             org.apache.jetspeed.portlets.layout  (5 usages)
>                 LayoutPortlet  (2 usages)
>                     addPortletToPage(String, String)  (1 usage)
> >>>>                        (279, 18) root.getFragments().add(fragment);
>                     removeFragment(String, String)  (1 usage)
> >>>>                        (257, 18) root.getFragments().remove(f);
>                 MultiColumnPortlet  (2 usages)
>                     doView(RenderRequest, RenderResponse)  (1 usage)
>                         (94, 65) layout = new ColumnLayout(numColumns, layoutType, f.getFragments(), this.colSizes.split("\\,") );
>                     processAction(ActionRequest, ActionResponse)  (1 usage)
>                         (242, 80) layout = new ColumnLayout(numColumns, layoutType, rootFragment.getFragments(), this.colSizes.split("\\,") );
>                 PageManagerLayoutEventListener  (1 usage)
>                     handleEvent(LayoutEvent)  (1 usage)
> >>>>                        (28, 40) page.getRootFragment().getFragments().add(event.getFragment());
>         jetspeed-page-manager  (60 usages)
>             org.apache.jetspeed.om.page  (15 usages)
>                 ContentFragmentImpl.ContentFragmentList  (1 usage)
>                     (441, 42) private List baseList = fragment.getFragments();
>                 TestPageObjectModel  (14 usages)
>                     testFragmentManipulation()  (14 usages)
>                         (108, 28) assertNotNull(root.getFragments());
> >>>>                        (114, 14) root.getFragments().add(frag1);
> >>>>                        (128, 15) frag2.getFragments().add(frag3);
> >>>>                        (129, 14) root.getFragments().add(frag2);
>                         (132, 25) assertTrue(root.getFragments().size()==2);
>                         (133, 27) Iterator i = root.getFragments().iterator();
>                         (142, 22) assertTrue(f.getFragments().size()==0);
>                         (147, 22) assertTrue(f.getFragments().size()==1);
>                         (149, 22) assertTrue(f.getFragments().size()==1);
>                         (150, 15) i = f.getFragments().iterator();
> >>>>                        (164, 11) f.getFragments().remove(frag3);
> >>>>                        (167, 11) f.getFragments().add(frag2);
>                         (168, 22) assertTrue(f.getFragments().size()==1);
>                         (169, 29) f = (FragmentImpl)f.getFragments().get(0);
>             org.apache.jetspeed.om.page.psml  (4 usages)
>                 PageImpl  (4 usages)
>                     getFragmentById(String)  (1 usage)
>                         (177, 28) Iterator i = f.getFragments().iterator();
>                     getFragmentsByName(String)  (1 usage)
>                         (276, 28) Iterator i = f.getFragments().iterator();
>                     removeFragmentById(String)  (2 usages)
>                         (209, 28) Iterator i = f.getFragments().iterator();
> >>>>                        (234, 28) if (parent.getFragments().remove(f))
>             org.apache.jetspeed.page  (41 usages)
>                 AbstractPageManager  (2 usages)
>                     copyFragment(Fragment, String)  (2 usages)
>                         (889, 37) Iterator fragments = source.getFragments().iterator();
> >>>>                        (894, 18) copy.getFragments().add(copiedFragment);
>                 PageManagerTestShared.Shared  (15 usages)
>                     testSecurePageManager(TestCase, PageManager)  (15 usages)
> >>>>                        (281, 34) root.getFragments().add(portlet);
> >>>>                        (287, 34) root.getFragments().add(portlet);
>                         (290, 71) test.assertNotNull(page.getRootFragment().getFragments());
>                         (291, 73) test.assertEquals(2, page.getRootFragment().getFragments().size());
>                         (292, 106) test.assertEquals("some-app::SomePortlet", ((Fragment)page.getRootFragment().getFragments().get(1)).getName());
>                         (293, 91) test.assertFalse("0".equals(((Fragment)page.getRootFragment().getFragments().get(1)).getId()));
>                         (294, 82) somePortletId[0] = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>                         (348, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (349, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (389, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (390, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (458, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (459, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                         (518, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (519, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                 TestCastorXmlPageManager  (15 usages)
>                     testClonePage()  (4 usages)
>                         (861, 30) List children = root.getFragments();
>                         (862, 40) List cloneChildren = cloneRoot.getFragments();
>                         (905, 26) assertNotNull(cf.getFragments());
>                         (906, 23) assertTrue(cf.getFragments().size() == 2);
>                     testCreatePage()  (3 usages)
> >>>>                        (266, 14) root.getFragments().add(f);
>                         (302, 43) assertTrue(page.getRootFragment().getFragments().size() == 1);
>                         (304, 47) f = (Fragment) page.getRootFragment().getFragments().get(0);
>                     testGetPage()  (3 usages)
>                         (190, 30) List children = root.getFragments();
>                         (237, 25) assertNotNull(f.getFragments());
>                         (238, 22) assertTrue(f.getFragments().size() == 2);
>                     testUpdatePage()  (5 usages)
>                         (373, 28) assertNotNull(root.getFragments());
>                         (374, 30) assertEquals(1, root.getFragments().size());
>                         (375, 41) String testId = ((Fragment)root.getFragments().get(0)).getId();
>                         (394, 28) assertNotNull(root.getFragments());
>                         (395, 25) assertTrue(root.getFragments().isEmpty());
>                 TestDatabasePageManager  (9 usages)
>                     testCreates()  (2 usages)
> >>>>                        (317, 14) root.getFragments().add(portlet);
> >>>>                        (328, 14) root.getFragments().add(portlet);
>                     testGets()  (4 usages)
>                         (620, 51) assertNotNull(check.getRootFragment().getFragments());
>                         (621, 53) assertEquals(2, check.getRootFragment().getFragments().size());
>                         (622, 65) Fragment check0 = (Fragment)check.getRootFragment().getFragments().get(0);
>                         (643, 65) Fragment check1 = (Fragment)check.getRootFragment().getFragments().get(1);
>                     testUpdates()  (3 usages)
>                         (814, 46) assertNotNull(page.getRootFragment().getFragments());
>                         (815, 48) assertEquals(2, page.getRootFragment().getFragments().size());
>                         (816, 61) String removeId = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>         jetspeed-portal  (4 usages)
>             org.apache.jetspeed.aggregator.impl  (1 usage)
>                 BasicAggregator  (1 usage)
>                     build(RequestContext)  (1 usage)
>                         (97, 34) for (Iterator fit = root.getFragments().iterator(); fit.hasNext();)
>             org.apache.jetspeed.decoration  (1 usage)
>                 PageTheme  (1 usage)
>                     PageTheme(Page, DecorationFactory, RequestContext)  (1 usage)
>                         (57, 43) Iterator fragments = rootFragment.getFragments().iterator();
>             org.apache.jetspeed.layout.impl  (2 usages)
>                 AddPortletAction  (1 usage)
>                     run(RequestContext, Map)  (1 usage)
> >>>>                        (126, 18) root.getFragments().add(fragment);            
>                 PortletPlacementContextImpl  (1 usage)
>                     processFragment(Fragment)  (1 usage)
>                         (145, 29) List children = fragment.getFragments();
>         jetspeed-registry  (1 usage)
>             org.apache.jetspeed.components.portletentity  (1 usage)
>                 TestPortletEntityDAO.ContentFragmentImpl  (1 usage)
>                     getFragments()  (1 usage)
>                         (145, 22) return f.getFragments();

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


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