From 7f9ffdf15ea6ae862b7a6be950ee106c8c933e6e Mon Sep 17 00:00:00 2001 From: Gavin Cornwell Date: Wed, 21 Sep 2011 08:10:25 +0000 Subject: [PATCH] Partial fix for ALF-10179: Some webscripts are declared "readwrite" but should be declared "readonly" if they do not update the repository. The de-lucening work has already moved to the required pattern where a check for container existence should be performed rather than just creating it, this allows GET requests to be declared to use a 'readonly' transaction which in turn provides performance benefits as more caching can be done. These webscripts have therefore been marked as 'readonly' in their descriptor, a few doclib scripts also work without code changes so have been marked as readonly too. I've added a new unit test (org.alfresco.repo.web.scripts.ReadOnlyTransactionInGetRestApiTest) that calls all scripts mentioned in the JIRA issue immediately after creating a site (meaning there are no containers present) as we identify more scripts that need the same treatment they can be added to this test. To help identify other GET webscripts using 'readwrite' transactions i've added a log WARN to RepositoryContainer which will output the following in the server console: 2011-09-21 08:57:46,431 WARN [web.scripts.RepositoryContainer] [main] Webscript with URL '/alfresco/service/slingshot/doclib/treenode/site/readOnlyTestSite/documentLibrary' is a GET request but it's descriptor has declared a readwrite transaction is required I'll suggest that QA raise new bugs when they see these and we can prioritise individually, we can disable this warning before release if necessary. git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@30664 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261 --- .../forum/forum-posts.get.desc.xml | 2 +- .../repository/links/link/link.get.desc.xml | 2 +- .../repository/links/links.get.desc.xml | 2 +- .../tagging/tagscope-tags.get.desc.xml | 2 +- .../documentlibrary-v2/node.get.desc.xml | 2 +- .../documentlibrary/categorynode.get.desc.xml | 2 +- .../documentlibrary/location.get.desc.xml | 2 +- .../documentlibrary/node.get.desc.xml | 2 +- .../alfresco/slingshot/wiki/page.get.desc.xml | 2 +- .../slingshot/wiki/pagelist.get.desc.xml | 2 +- .../slingshot/wiki/version.get.desc.xml | 2 +- .../ReadOnlyTransactionInGetRestApiTest.java | 284 ++++++++++++++++++ .../repo/web/scripts/RepositoryContainer.java | 9 + 13 files changed, 304 insertions(+), 11 deletions(-) create mode 100644 source/java/org/alfresco/repo/web/scripts/ReadOnlyTransactionInGetRestApiTest.java diff --git a/config/alfresco/templates/webscripts/org/alfresco/repository/discussions/forum/forum-posts.get.desc.xml b/config/alfresco/templates/webscripts/org/alfresco/repository/discussions/forum/forum-posts.get.desc.xml index 027bf9d26f..0c032e187f 100644 --- a/config/alfresco/templates/webscripts/org/alfresco/repository/discussions/forum/forum-posts.get.desc.xml +++ b/config/alfresco/templates/webscripts/org/alfresco/repository/discussions/forum/forum-posts.get.desc.xml @@ -6,5 +6,5 @@ /api/forum/node/{store_type}/{store_id}/{id}/posts argument user - required + required \ No newline at end of file diff --git a/config/alfresco/templates/webscripts/org/alfresco/repository/links/link/link.get.desc.xml b/config/alfresco/templates/webscripts/org/alfresco/repository/links/link/link.get.desc.xml index 0f42bc72ad..dec0aea1dd 100644 --- a/config/alfresco/templates/webscripts/org/alfresco/repository/links/link/link.get.desc.xml +++ b/config/alfresco/templates/webscripts/org/alfresco/repository/links/link/link.get.desc.xml @@ -4,5 +4,5 @@ /api/links/link/site/{site}/{container}/{path} argument user - required + required \ No newline at end of file diff --git a/config/alfresco/templates/webscripts/org/alfresco/repository/links/links.get.desc.xml b/config/alfresco/templates/webscripts/org/alfresco/repository/links/links.get.desc.xml index 15642727b6..b1decc658f 100644 --- a/config/alfresco/templates/webscripts/org/alfresco/repository/links/links.get.desc.xml +++ b/config/alfresco/templates/webscripts/org/alfresco/repository/links/links.get.desc.xml @@ -4,5 +4,5 @@ /api/links/site/{site}/{container} argument user - required + required \ No newline at end of file diff --git a/config/alfresco/templates/webscripts/org/alfresco/repository/tagging/tagscope-tags.get.desc.xml b/config/alfresco/templates/webscripts/org/alfresco/repository/tagging/tagscope-tags.get.desc.xml index ca7c533790..6ecf6ca8b6 100644 --- a/config/alfresco/templates/webscripts/org/alfresco/repository/tagging/tagscope-tags.get.desc.xml +++ b/config/alfresco/templates/webscripts/org/alfresco/repository/tagging/tagscope-tags.get.desc.xml @@ -8,7 +8,7 @@ /api/tagscopes/site/{site}/{container}/tags argument user - required + required draft_public_api Tagging \ No newline at end of file diff --git a/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary-v2/node.get.desc.xml b/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary-v2/node.get.desc.xml index 3d8399223f..0d47c3abb8 100644 --- a/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary-v2/node.get.desc.xml +++ b/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary-v2/node.get.desc.xml @@ -4,6 +4,6 @@ /slingshot/doclib2/node/{store_type}/{store_id}/{id} argument user - required + required internal \ No newline at end of file diff --git a/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/categorynode.get.desc.xml b/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/categorynode.get.desc.xml index 5f2f33d213..2bd6b47e79 100644 --- a/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/categorynode.get.desc.xml +++ b/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/categorynode.get.desc.xml @@ -5,6 +5,6 @@ /slingshot/doclib/categorynode/node/{store_type}/{store_id}/{id} argument user - required + required internal \ No newline at end of file diff --git a/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/location.get.desc.xml b/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/location.get.desc.xml index 9277ab04a4..5cc5522d42 100644 --- a/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/location.get.desc.xml +++ b/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/location.get.desc.xml @@ -4,6 +4,6 @@ /slingshot/doclib/node/{store_type}/{store_id}/{id}/location argument user - required + required internal \ No newline at end of file diff --git a/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/node.get.desc.xml b/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/node.get.desc.xml index e9ed5635d7..9675060892 100644 --- a/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/node.get.desc.xml +++ b/config/alfresco/templates/webscripts/org/alfresco/slingshot/documentlibrary/node.get.desc.xml @@ -4,6 +4,6 @@ /slingshot/doclib/node/{store_type}/{store_id}/{id} argument user - required + required internal \ No newline at end of file diff --git a/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/page.get.desc.xml b/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/page.get.desc.xml index dfc2b28ab2..e76c291155 100644 --- a/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/page.get.desc.xml +++ b/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/page.get.desc.xml @@ -4,6 +4,6 @@ /slingshot/wiki/page/{siteId}/{pageTitle} argument user - required + required internal diff --git a/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/pagelist.get.desc.xml b/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/pagelist.get.desc.xml index a50c5a040a..61b5f6e48d 100644 --- a/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/pagelist.get.desc.xml +++ b/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/pagelist.get.desc.xml @@ -4,6 +4,6 @@ /slingshot/wiki/pages/{siteId} argument user - required + required internal \ No newline at end of file diff --git a/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/version.get.desc.xml b/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/version.get.desc.xml index 4b606f0226..5ffbf6f12a 100644 --- a/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/version.get.desc.xml +++ b/config/alfresco/templates/webscripts/org/alfresco/slingshot/wiki/version.get.desc.xml @@ -4,6 +4,6 @@ /slingshot/wiki/version/{siteId}/{pageTitle}/{versionId} argument user - required + required internal \ No newline at end of file diff --git a/source/java/org/alfresco/repo/web/scripts/ReadOnlyTransactionInGetRestApiTest.java b/source/java/org/alfresco/repo/web/scripts/ReadOnlyTransactionInGetRestApiTest.java new file mode 100644 index 0000000000..643f1e25e1 --- /dev/null +++ b/source/java/org/alfresco/repo/web/scripts/ReadOnlyTransactionInGetRestApiTest.java @@ -0,0 +1,284 @@ +/* + * Copyright (C) 2005-2011 Alfresco Software Limited. + * + * This file is part of Alfresco + * + * Alfresco is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Alfresco is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Alfresco. If not, see . + */ +package org.alfresco.repo.web.scripts; + +import java.text.MessageFormat; +import java.util.List; + +import org.alfresco.repo.security.authentication.AuthenticationUtil; +import org.alfresco.repo.transaction.RetryingTransactionHelper.RetryingTransactionCallback; +import org.alfresco.service.cmr.repository.ChildAssociationRef; +import org.alfresco.service.cmr.repository.NodeRef; +import org.alfresco.service.cmr.repository.NodeService; +import org.alfresco.service.cmr.site.SiteInfo; +import org.alfresco.service.cmr.site.SiteService; +import org.alfresco.service.cmr.site.SiteVisibility; +import org.alfresco.service.transaction.TransactionService; +import org.springframework.context.ApplicationContext; +import org.springframework.extensions.webscripts.Status; +import org.springframework.extensions.webscripts.TestWebScriptServer.GetRequest; +import org.springframework.extensions.webscripts.TestWebScriptServer.Response; + +/** + * Set of tests that ensure GET REST APIs are run successfully in a read-only + * transaction (ALF-10179). + * + * Some webscripts have a side effect of creating a "container" these tests + * are to ensure this is handled gracefully in a way that allows the main + * transaction to be declared as readonly for performance reasons. + * + * @author Gavin Cornwell + * @since 4.0 + */ +public class ReadOnlyTransactionInGetRestApiTest extends BaseWebScriptTest +{ + private static final String TEST_SITE_NAME = "readOnlyTestSite"; + + private static final String URL_GET_SITE_BLOG = "/api/blog/site/" + TEST_SITE_NAME + "/blog"; + private static final String URL_GET_SITE_FORUM_POSTS = "/api/forum/site/" + TEST_SITE_NAME + "/discussions/posts"; + private static final String URL_GET_SITE_LINKS = "/api/links/site/" + TEST_SITE_NAME + "/links?page=1&pageSize=10"; + private static final String URL_GET_SITE_LINK = "/api/links/link/site/" + TEST_SITE_NAME + "/links/123456789"; + private static final String URL_GET_SITE_TAGS = "/api/tagscopes/site/" + TEST_SITE_NAME + "/tags"; + private static final String URL_GET_SITE_DATALISTS = "/slingshot/datalists/lists/site/" + TEST_SITE_NAME + "/dataLists"; + private static final String URL_GET_SITE_WIKI = "/slingshot/wiki/pages/" + TEST_SITE_NAME; + private static final String URL_GET_SITE_WIKI_PAGE = "/slingshot/wiki/page/" + TEST_SITE_NAME + "/AWikiPage"; + private static final String URL_GET_SITE_WIKI_PAGE_VERSION = "/slingshot/wiki/version/" + TEST_SITE_NAME + "/AWikiPage/123456789"; + private static final String URL_GET_DOCLIB_CATEGORYNODE = "/slingshot/doclib/categorynode/node/{0}"; + private static final String URL_GET_DOCLIB_TREENODE = "/slingshot/doclib/treenode/site/" + TEST_SITE_NAME + "/documentLibrary"; + private static final String URL_GET_DOCLIB_NODE = "/slingshot/doclib/node/{0}"; + private static final String URL_GET_DOCLIB2_NODE = "/slingshot/doclib2/node/{0}"; + private static final String URL_GET_DOCLIB_LOCATION = "/slingshot/doclib/node/{0}/location"; + private static final String URL_GET_DOCLIB_DOCLIST = "/slingshot/doclib/doclist/documents/site/" + TEST_SITE_NAME + "/documentLibrary"; + private static final String URL_GET_DOCLIB2_DOCLIST = "/slingshot/doclib2/doclist/documents/site/" + TEST_SITE_NAME + "/documentLibrary"; + private static final String URL_GET_DOCLIB_IMAGES = "/slingshot/doclib/images/site/" + TEST_SITE_NAME + "/documentLibrary"; + + private SiteService siteService; + private NodeService nodeService; + private TransactionService transactionService; + + private NodeRef testSiteNodeRef; + private String testSiteNodeRefString; + + private boolean logEnabled = false; + + @Override + protected void setUp() throws Exception + { + super.setUp(); + ApplicationContext appContext = getServer().getApplicationContext(); + + this.siteService = (SiteService)appContext.getBean("SiteService"); + this.nodeService = (NodeService)appContext.getBean("NodeService"); + this.transactionService = (TransactionService)appContext.getBean("TransactionService"); + + // set admin as current user + AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName()); + + // delete the test site if it's still hanging around from previous runs + if (siteService.getSite(TEST_SITE_NAME) != null) + { + siteService.deleteSite(TEST_SITE_NAME); + } + + // create the test site, this should create a site but it won't have any containers created + SiteInfo siteInfo = this.siteService.createSite("collaboration", TEST_SITE_NAME, "Read Only Test Site", + "Test site for ReadOnlyTransactionRestApiTest", SiteVisibility.PUBLIC); + this.testSiteNodeRef = siteInfo.getNodeRef(); + this.testSiteNodeRefString = this.testSiteNodeRef.toString().replace("://", "/"); + + // ensure there are no containers present at the start of the test + List children = nodeService.getChildAssocs(this.testSiteNodeRef); + assertTrue("The test site should not have any containers", children.isEmpty()); + } + + /* (non-Javadoc) + * @see junit.framework.TestCase#tearDown() + */ + @Override + protected void tearDown() throws Exception + { + super.tearDown(); + + // use retrying transaction to delete the site + this.transactionService.getRetryingTransactionHelper().doInTransaction(new RetryingTransactionCallback() + { + @Override + public Void execute() throws Throwable + { + // delete the test site + siteService.deleteSite(TEST_SITE_NAME); + + return null; + } + }); + + AuthenticationUtil.clearCurrentSecurityContext(); + } + + public void testGetSiteBlog() throws Exception + { + // TODO: Fixme - This REST API still requires a readwrite transaction to be successful + // Also add tests for all other blog GET REST APIs + + Response response = sendRequest(new GetRequest(URL_GET_SITE_BLOG), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetSiteForumPosts() throws Exception + { + Response response = sendRequest(new GetRequest(URL_GET_SITE_FORUM_POSTS), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetSiteLinks() throws Exception + { + Response response = sendRequest(new GetRequest(URL_GET_SITE_LINKS), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetSiteLink() throws Exception + { + Response response = sendRequest(new GetRequest(URL_GET_SITE_LINK), 404); + logResponse(response); + assertEquals(Status.STATUS_NOT_FOUND, response.getStatus()); + } + + public void testGetSiteTags() throws Exception + { + Response response = sendRequest(new GetRequest(URL_GET_SITE_TAGS), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetSiteDataLists() throws Exception + { + // TODO: Fixme - This REST API still requires a readwrite transaction to be successful + + Response response = sendRequest(new GetRequest(URL_GET_SITE_DATALISTS), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetSiteWiki() throws Exception + { + Response response = sendRequest(new GetRequest(URL_GET_SITE_WIKI), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetSiteWikiPage() throws Exception + { + Response response = sendRequest(new GetRequest(URL_GET_SITE_WIKI_PAGE), 404); + logResponse(response); + assertEquals(Status.STATUS_NOT_FOUND, response.getStatus()); + } + + public void testGetSiteWikiPageVersion() throws Exception + { + Response response = sendRequest(new GetRequest(URL_GET_SITE_WIKI_PAGE_VERSION), 404); + logResponse(response); + assertEquals(Status.STATUS_NOT_FOUND, response.getStatus()); + } + + public void testGetDoclibTreeNode() throws Exception + { + // TODO: Fixme - This REST API still requires a readwrite transaction to be successful + + Response response = sendRequest(new GetRequest(URL_GET_DOCLIB_TREENODE), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetDoclibDoclist() throws Exception + { + // TODO: Fixme - This REST API still requires a readwrite transaction to be successful + + Response response = sendRequest(new GetRequest(URL_GET_DOCLIB_DOCLIST), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetDoclib2Doclist() throws Exception + { + // TODO: Fixme - This REST API still requires a readwrite transaction to be successful + + Response response = sendRequest(new GetRequest(URL_GET_DOCLIB2_DOCLIST), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetDoclibImages() throws Exception + { + // TODO: Fixme - This REST API still requires a readwrite transaction to be successful + + Response response = sendRequest(new GetRequest(URL_GET_DOCLIB_IMAGES), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetDoclibCategoryNode() throws Exception + { + Response response = sendRequest(new GetRequest(MessageFormat.format(URL_GET_DOCLIB_CATEGORYNODE, + testSiteNodeRefString)), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetDoclibNode() throws Exception + { + Response response = sendRequest(new GetRequest(MessageFormat.format(URL_GET_DOCLIB_NODE, + testSiteNodeRefString)), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetDoclib2Node() throws Exception + { + Response response = sendRequest(new GetRequest(MessageFormat.format(URL_GET_DOCLIB2_NODE, + testSiteNodeRefString)), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + public void testGetDoclibNodeLocation() throws Exception + { + Response response = sendRequest(new GetRequest(MessageFormat.format(URL_GET_DOCLIB_LOCATION, + testSiteNodeRefString)), 200); + logResponse(response); + assertEquals(Status.STATUS_OK, response.getStatus()); + } + + private void logResponse(Response response) + { + if (this.logEnabled) + { + try + { + System.out.println(response.getContentAsString()); + } + catch (Exception e) + { + System.err.println("Unable to log response: " + e.toString()); + } + } + } +} diff --git a/source/java/org/alfresco/repo/web/scripts/RepositoryContainer.java b/source/java/org/alfresco/repo/web/scripts/RepositoryContainer.java index ee82558733..2d09b3e147 100644 --- a/source/java/org/alfresco/repo/web/scripts/RepositoryContainer.java +++ b/source/java/org/alfresco/repo/web/scripts/RepositoryContainer.java @@ -448,6 +448,15 @@ public class RepositoryContainer extends AbstractRuntimeContainer implements Ten boolean readonly = description.getRequiredTransactionParameters().getCapability() == TransactionCapability.readonly; boolean requiresNew = description.getRequiredTransaction() == RequiredTransaction.requiresnew; + + // log a warning if we detect a GET webscript being run in a readwrite transaction, GET calls should + // NOT have any side effects so this scenario as a warning sign something maybe amiss, see ALF-10179. + if (logger.isWarnEnabled() && !readonly && "GET".equalsIgnoreCase(description.getMethod())) + { + logger.warn("Webscript with URL '" + scriptReq.getURL() + + "' is a GET request but it's descriptor has declared a readwrite transaction is required"); + } + try { retryingTransactionHelper.doInTransaction(work, readonly, requiresNew);