[ 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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
