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 "Randy Watler (JIRA)" <je...@portals.apache.org> on 2006/01/24 08:17:13 UTC
[jira] Resolved: (JS2-473) Many uses of Fragment.getFragments()
assume access to the underlying list, not a copy: this is invalid
[ 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