portals-jetspeed-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Jencks (JIRA)" <jetspeed-...@portals.apache.org>
Subject [jira] Updated: (JS2-473) Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid
Date Mon, 23 Jan 2006 01:21:45 GMT
     [ 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


Mime
View raw message