Author: tomekr Date: Tue Mar 13 13:44:57 2018 New Revision: 1826632 URL: http://svn.apache.org/viewvc?rev=1826632&view=rev Log: OAK-7335: oak-upgrade long name filter should consider the path length
Modified: jackrabbit/oak/branches/1.6/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositorySidegrade.java jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositoryUpgrade.java jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/nodestate/NameFilteringNodeState.java jackrabbit/oak/branches/1.6/oak-upgrade/src/test/java/org/apache/jackrabbit/oak/upgrade/LongNameTest.java Modified: jackrabbit/oak/branches/1.6/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.6/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java?rev=1826632&r1=1826631&r2=1826632&view=diff ============================================================================== --- jackrabbit/oak/branches/1.6/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java (original) +++ jackrabbit/oak/branches/1.6/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/util/Utils.java Tue Mar 13 13:44:57 2018 @@ -71,14 +71,14 @@ public class Utils { * possibly be too large to be used for the primary key for the document * store. */ - static final int PATH_SHORT = Integer.getInteger("oak.pathShort", 165); + public static final int PATH_SHORT = Integer.getInteger("oak.pathShort", 165); /** * The maximum length of the parent path, in bytes. If the parent path is * longer, then the id of a document is no longer the path, but the hash of * the parent, and then the node name. */ - static final int PATH_LONG = Integer.getInteger("oak.pathLong", 350); + public static final int PATH_LONG = Integer.getInteger("oak.pathLong", 350); /** * The maximum size a node name, in bytes. This is only a problem for long path. Modified: jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositorySidegrade.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositorySidegrade.java?rev=1826632&r1=1826631&r2=1826632&view=diff ============================================================================== --- jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositorySidegrade.java (original) +++ jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositorySidegrade.java Tue Mar 13 13:44:57 2018 @@ -523,7 +523,7 @@ public class RepositorySidegrade { wrapped = ReportingNodeState.wrap(wrapped, new LoggingReporter(LOG, "Copying", LOG_NODE_COPY, -1)); } if (filterLongNames) { - wrapped = NameFilteringNodeState.wrap(wrapped); + wrapped = NameFilteringNodeState.wrapRoot(wrapped); } return wrapped; } Modified: jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositoryUpgrade.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositoryUpgrade.java?rev=1826632&r1=1826631&r2=1826632&view=diff ============================================================================== --- jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositoryUpgrade.java (original) +++ jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/RepositoryUpgrade.java Tue Mar 13 13:44:57 2018 @@ -57,6 +57,7 @@ import javax.jcr.nodetype.NodeTypeTempla import javax.jcr.nodetype.PropertyDefinitionTemplate; import javax.jcr.security.Privilege; +import com.google.common.base.Charsets; import com.google.common.base.Function; import com.google.common.base.Stopwatch; import com.google.common.collect.HashBiMap; @@ -82,6 +83,7 @@ import org.apache.jackrabbit.oak.api.Roo import org.apache.jackrabbit.oak.api.Tree; import org.apache.jackrabbit.oak.commons.PathUtils; import org.apache.jackrabbit.oak.namepath.NamePathMapper; +import org.apache.jackrabbit.oak.plugins.document.util.Utils; import org.apache.jackrabbit.oak.plugins.index.CompositeIndexEditorProvider; import org.apache.jackrabbit.oak.plugins.index.IndexEditorProvider; import org.apache.jackrabbit.oak.plugins.index.IndexUpdate; @@ -483,7 +485,7 @@ public class RepositoryUpgrade { ); final NodeState sourceRoot; if (filterLongNames) { - sourceRoot = NameFilteringNodeState.wrap(reportingSourceRoot); + sourceRoot = NameFilteringNodeState.wrapRoot(reportingSourceRoot); } else { sourceRoot = reportingSourceRoot; } @@ -992,14 +994,16 @@ public class RepositoryUpgrade { continue; } String name = t.text(); - if (NameFilteringNodeState.isNameTooLong(name)) { + if (nameMayBeTooLong(name)) { TermDocs docs = reader.termDocs(t); if (docs.next()) { int docId = docs.doc(); String uuid = reader.document(docId).get(FieldNames.UUID); Node n = session.getNodeByIdentifier(uuid); - logger.warn("Name too long: {}", n.getPath()); - longNameFound = true; + if (isNameTooLong(n.getName(), n.getParent().getPath())) { + logger.warn("Name too long: {}", n.getPath()); + longNameFound = true; + } } } } @@ -1014,6 +1018,30 @@ public class RepositoryUpgrade { } } + private boolean nameMayBeTooLong(String name) { + if (name.length() <= Utils.NODE_NAME_LIMIT / 4) { + return false; + } + if (name.getBytes(Charsets.UTF_8).length <= Utils.NODE_NAME_LIMIT) { + return false; + } + return true; + } + + private boolean isNameTooLong(String name, String parentPath) { + if (!nameMayBeTooLong(name)) { + return false; + } + if (parentPath.length() < Utils.PATH_SHORT) { + return false; + } + if (parentPath.getBytes(Charsets.UTF_8).length < Utils.PATH_LONG) { + return false; + } + return true; + } + + static class LoggingCompositeHook implements CommitHook { private final Collection<CommitHook> hooks; private boolean started = false; Modified: jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/nodestate/NameFilteringNodeState.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/nodestate/NameFilteringNodeState.java?rev=1826632&r1=1826631&r2=1826632&view=diff ============================================================================== --- jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/nodestate/NameFilteringNodeState.java (original) +++ jackrabbit/oak/branches/1.6/oak-upgrade/src/main/java/org/apache/jackrabbit/oak/upgrade/nodestate/NameFilteringNodeState.java Tue Mar 13 13:44:57 2018 @@ -18,29 +18,32 @@ package org.apache.jackrabbit.oak.upgrad import com.google.common.base.Charsets; import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.commons.PathUtils; +import org.apache.jackrabbit.oak.plugins.document.util.Utils; import org.apache.jackrabbit.oak.spi.state.NodeState; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; +import java.util.ArrayList; +import java.util.List; public class NameFilteringNodeState extends AbstractDecoratedNodeState { private static final Logger LOG = LoggerFactory.getLogger(NameFilteringNodeState.class); - private static final int NODE_NAME_LIMIT = 150; - /** - * Max character size in bytes in UTF8 = 4. Therefore if the number of characters is smaller - * than NODE_NAME_LIMIT / 4 we don't need to count bytes. - */ - private static final int SAFE_NODE_NAME_LENGTH = NODE_NAME_LIMIT / 4; + private final NameFilteringNodeState parent; - public static NodeState wrap(final NodeState delegate) { - return new NameFilteringNodeState(delegate); + private final String name; + + public static NodeState wrapRoot(final NodeState delegate) { + return new NameFilteringNodeState(delegate, null, null); } - private NameFilteringNodeState(final NodeState delegate) { + private NameFilteringNodeState(final NodeState delegate, NameFilteringNodeState parent, String name) { super(delegate); + this.parent = parent; + this.name = name; } @Override @@ -55,7 +58,7 @@ public class NameFilteringNodeState exte @Override @Nonnull protected NodeState decorateChild(@Nonnull final String name, @Nonnull final NodeState delegateChild) { - return wrap(delegateChild); + return new NameFilteringNodeState(delegateChild, this, name); } @Override @@ -69,11 +72,41 @@ public class NameFilteringNodeState exte * * @param name * to check - * @return true if the name is longer than {@code org.apache.jackrabbit.oak.plugins.document.util.Utils#NODE_NAME_LIMIT} + * @return true if the name is longer than {@link org.apache.jackrabbit.oak.plugins.document.util.Utils#NODE_NAME_LIMIT} */ - public static boolean isNameTooLong(@Nonnull String name) { + private boolean isNameTooLong(@Nonnull String name) { // OAK-1589: maximum supported length of name for DocumentNodeStore // is 150 bytes. Skip the sub tree if the the name is too long - return name.length() > SAFE_NODE_NAME_LENGTH && name.getBytes(Charsets.UTF_8).length > NODE_NAME_LIMIT; + if (name.length() <= Utils.NODE_NAME_LIMIT / 4) { + return false; + } + if (name.getBytes(Charsets.UTF_8).length <= Utils.NODE_NAME_LIMIT) { + return false; + } + String path = getPath(); + if (path.length() <= Utils.PATH_SHORT) { + return false; + } + if (path.getBytes(Charsets.UTF_8).length < Utils.PATH_LONG) { + return false; + } + return true; + } + + private String getPath() { + List<String> names = new ArrayList<>(); + NameFilteringNodeState ns = this; + while (ns.parent != null) { + names.add(ns.name); + ns = ns.parent; + } + String[] reversed = new String[names.size()]; + + int i = reversed.length - 1; + for (String name : names) { + reversed[i--] = name; + } + + return PathUtils.concat(PathUtils.ROOT_PATH, reversed); } -} +} \ No newline at end of file Modified: jackrabbit/oak/branches/1.6/oak-upgrade/src/test/java/org/apache/jackrabbit/oak/upgrade/LongNameTest.java URL: http://svn.apache.org/viewvc/jackrabbit/oak/branches/1.6/oak-upgrade/src/test/java/org/apache/jackrabbit/oak/upgrade/LongNameTest.java?rev=1826632&r1=1826631&r2=1826632&view=diff ============================================================================== --- jackrabbit/oak/branches/1.6/oak-upgrade/src/test/java/org/apache/jackrabbit/oak/upgrade/LongNameTest.java (original) +++ jackrabbit/oak/branches/1.6/oak-upgrade/src/test/java/org/apache/jackrabbit/oak/upgrade/LongNameTest.java Tue Mar 13 13:44:57 2018 @@ -18,169 +18,208 @@ */ package org.apache.jackrabbit.oak.upgrade; -import static com.google.common.collect.Iterables.cycle; -import static com.google.common.collect.Iterables.limit; -import static org.junit.Assert.fail; - -import java.io.File; -import java.io.IOException; - -import javax.jcr.Credentials; -import javax.jcr.Node; -import javax.jcr.RepositoryException; -import javax.jcr.Session; -import javax.jcr.SimpleCredentials; - -import org.apache.commons.io.FileUtils; import org.apache.jackrabbit.core.RepositoryContext; import org.apache.jackrabbit.core.RepositoryImpl; import org.apache.jackrabbit.core.config.RepositoryConfig; +import org.apache.jackrabbit.oak.api.CommitFailedException; +import org.apache.jackrabbit.oak.commons.PathUtils; import org.apache.jackrabbit.oak.plugins.document.DocumentMK; import org.apache.jackrabbit.oak.plugins.document.DocumentNodeStore; -import org.apache.jackrabbit.oak.plugins.memory.MemoryNodeStore; import org.apache.jackrabbit.oak.segment.SegmentNodeStore; import org.apache.jackrabbit.oak.segment.SegmentNodeStoreBuilders; import org.apache.jackrabbit.oak.segment.memory.MemoryStore; +import org.apache.jackrabbit.oak.spi.commit.CommitInfo; +import org.apache.jackrabbit.oak.spi.commit.EmptyHook; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; import org.apache.jackrabbit.oak.spi.state.NodeState; import org.apache.jackrabbit.oak.spi.state.NodeStore; -import org.apache.jackrabbit.oak.stats.Clock; import org.junit.Assert; -import org.junit.BeforeClass; +import org.junit.Ignore; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; -public class LongNameTest { +import javax.jcr.Node; +import javax.jcr.RepositoryException; +import javax.jcr.Session; +import javax.jcr.SimpleCredentials; +import java.io.File; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Random; - public static final Credentials CREDENTIALS = new SimpleCredentials("admin", "admin".toCharArray()); +import static org.junit.Assert.fail; - private static final String TOO_LONG_NAME = "this string is an example of a very long node name which is approximately one hundred fifty eight bytes long so too long for the document node store to handle"; +@RunWith(Parameterized.class) +public class LongNameTest { - private static final String NOT_TOO_LONG_NAME = "this string despite it is very long as well is not too long for the document node store to handle so it may be migrated succesfully without troubles"; + private static final Logger LOG = LoggerFactory.getLogger(LongNameTest.class); - private static RepositoryConfig sourceRepositoryConfig; + @Parameterized.Parameters(name = "{0}") + public static Collection<Object[]> data() { + List<Object[]> params = new ArrayList<Object[]>(); - private static File crx2RepoDir; + params.add(new Object[] { "Short parent, short name", 349, 150, false }); + params.add(new Object[] { "Short parent, long name", 349, 151, false }); + params.add(new Object[] { "Long parent, short name", 350, 150, false }); + params.add(new Object[] { "Long parent, long name", 350, 151, true }); - @BeforeClass - public static void prepareSourceRepository() throws RepositoryException, IOException, InterruptedException { - crx2RepoDir = new File("target", "upgrade-" + Clock.SIMPLE.getTimeIncreasing()); - FileUtils.deleteQuietly(crx2RepoDir); + return params; + } - sourceRepositoryConfig = createCrx2Config(crx2RepoDir); - RepositoryContext ctx = RepositoryContext.create(sourceRepositoryConfig); - RepositoryImpl sourceRepository = ctx.getRepository(); - Session session = sourceRepository.login(CREDENTIALS); - try { - Assert.assertTrue(TOO_LONG_NAME.getBytes().length > 150); - Assert.assertTrue(NOT_TOO_LONG_NAME.getBytes().length < 150); + private final String parentPath; - Node longNameParent = createParent(session.getRootNode()); - Assert.assertTrue(longNameParent.getPath().length() >= 350); + private final String nodeName; - longNameParent.addNode(TOO_LONG_NAME); - longNameParent.addNode(NOT_TOO_LONG_NAME); - session.save(); + private final boolean shouldBeSkipped; - Assert.assertTrue(longNameParent.hasNode(TOO_LONG_NAME)); - Assert.assertTrue(longNameParent.hasNode(NOT_TOO_LONG_NAME)); + @Rule + public final TemporaryFolder crxRepo = new TemporaryFolder(new File("target")); - } finally { - session.logout(); - } - sourceRepository.shutdown(); - } - - private static RepositoryConfig createCrx2Config(File crx2RepoDir) throws RepositoryException, IOException { - File source = new File(crx2RepoDir, "source"); - source.mkdirs(); - return RepositoryConfig.install(source); + public LongNameTest(String name, int parentPathLength, int nodeNameLength, boolean shouldBeSkipped) { + this.parentPath = generatePath(parentPathLength); + this.nodeName = generateNodeName(nodeNameLength); + this.shouldBeSkipped = shouldBeSkipped; } @Test - public void longNameShouldBeSkipped() throws RepositoryException, IOException { - DocumentNodeStore nodeStore = new DocumentMK.Builder().getNodeStore(); - try { - upgrade(nodeStore, false, true); - - NodeState parent = getParent(nodeStore.getRoot()); - Assert.assertTrue(parent.hasChildNode(NOT_TOO_LONG_NAME)); - Assert.assertEquals(1, parent.getChildNodeCount(10)); - - // The following throws an DocumentStoreException: - // Assert.assertFalse(parent.hasChildNode(TOO_LONG_NAME)); - } finally { - nodeStore.dispose(); + public void testMigrationToDocStore() throws IOException, CommitFailedException, RepositoryException { + SegmentNodeStore src = SegmentNodeStoreBuilders.builder(new MemoryStore()).build(); + createNodes(src); + + DocumentNodeStore dst = new DocumentMK.Builder().getNodeStore(); + + RepositorySidegrade sidegrade = new RepositorySidegrade(src, dst); + sidegrade.setFilterLongNames(true); + sidegrade.copy(); + + NodeState parent = getParent(dst); + Assert.assertTrue("Parent should exists", parent.exists()); + if (shouldBeSkipped) { + Assert.assertFalse("Node shouldn't exists", parent.hasChildNode(nodeName)); + } else { + Assert.assertTrue("Node should exists", parent.hasChildNode(nodeName)); } } @Test - public void assertNoLongNamesTest() throws IOException, RepositoryException { - RepositoryConfig config = createCrx2Config(crx2RepoDir); - RepositoryContext context = RepositoryContext.create(config); + public void testNodeOnDocStore() throws CommitFailedException { + DocumentNodeStore docStore = new DocumentMK.Builder().getNodeStore(); + try { - RepositoryUpgrade upgrade = new RepositoryUpgrade(context, new MemoryNodeStore()); - try { - upgrade.assertNoLongNames(); - fail("Exception should be thrown"); - } catch (RepositoryException e) { - // that's fine + createNodes(docStore); + if (shouldBeSkipped) { + fail("It shouldn't be possible to create a node"); + } + } catch (CommitFailedException e) { + if (!shouldBeSkipped) { + LOG.warn("Unexpected exception", e); + fail("It should be possible to create a node"); } - } finally { - context.getRepository().shutdown(); } } - @Test(expected = RepositoryException.class) - public void longNameOnDocumentStoreThrowsAnException() throws RepositoryException, IOException { - DocumentNodeStore nodeStore = new DocumentMK.Builder().getNodeStore(); + @Test + @Ignore + public void testUpgradeToDocStore() throws IOException, CommitFailedException, RepositoryException { + File root = crxRepo.newFolder(); + File source = new File(root, "source"); + source.mkdirs(); + RepositoryImpl sourceRepository = RepositoryContext.create(RepositoryConfig.install(source)).getRepository(); + Session session = sourceRepository.login(new SimpleCredentials("admin", "admin".toCharArray())); try { - upgrade(nodeStore, false, false); + Node node = session.getRootNode(); + for (String s : PathUtils.elements(parentPath)) { + node = node.addNode(s); + } + node.addNode(nodeName); + session.save(); } finally { - nodeStore.dispose(); + session.logout(); } - } - - @Test - public void longNameOnSegmentStoreWorksFine() throws RepositoryException, IOException { - SegmentNodeStore nodeStore = SegmentNodeStoreBuilders.builder(new MemoryStore()).build(); - upgrade(nodeStore, false, false); - - NodeState parent = getParent(nodeStore.getRoot()); - Assert.assertTrue(parent.hasChildNode(NOT_TOO_LONG_NAME)); - Assert.assertTrue(parent.hasChildNode(TOO_LONG_NAME)); - } + sourceRepository.shutdown(); - private static void upgrade(NodeStore target, boolean skipNameCheck, boolean filterLongNames) throws RepositoryException, IOException { - RepositoryConfig config = createCrx2Config(crx2RepoDir); - RepositoryContext context = RepositoryContext.create(config); + DocumentNodeStore dst = new DocumentMK.Builder().getNodeStore(); + RepositoryContext ctx = RepositoryContext.create(RepositoryConfig.install(source)); + RepositoryUpgrade upgrade = new RepositoryUpgrade(ctx, dst); + upgrade.setCheckLongNames(true); try { - RepositoryUpgrade upgrade = new RepositoryUpgrade(context, target); - upgrade.setCheckLongNames(skipNameCheck); - upgrade.setFilterLongNames(filterLongNames); upgrade.copy(null); - } finally { - context.getRepository().shutdown(); + if (shouldBeSkipped) { + fail("Jackrabbit2 Lucene index should be used to inform about the node with long name"); + } + } catch (Exception e) { + if (!shouldBeSkipped) { + LOG.warn("Unexpected exception", e); + fail("Upgrade should be successful"); + } + } + + ctx.getRepository().shutdown(); + ctx = RepositoryContext.create(RepositoryConfig.install(source)); + upgrade = new RepositoryUpgrade(ctx, dst); + upgrade.setCheckLongNames(false); + upgrade.copy(null); + + NodeState parent = getParent(dst); + Assert.assertTrue("Parent should exists", parent.exists()); + if (shouldBeSkipped) { + Assert.assertFalse("Node shouldn't exists", parent.hasChildNode(nodeName)); + } else { + Assert.assertTrue("Node should exists", parent.hasChildNode(nodeName)); } } - private static Node createParent(Node root) throws RepositoryException { - Node current = root; - for (String segment : getParentSegments()) { - current = current.addNode(segment); + private void createNodes(NodeStore ns) throws CommitFailedException { + NodeBuilder root = ns.getRoot().builder(); + NodeBuilder nb = root; + for (String s : PathUtils.elements(parentPath)) { + nb = nb.child(s); + } + nb.child(nodeName); + ns.merge(root, EmptyHook.INSTANCE, CommitInfo.EMPTY); + } + + private static String generatePath(int length) { + if (length == 1) { + return "/"; + } + + Random random = new Random(); + StringBuilder path = new StringBuilder(); + while (path.length() < length) { + int remaining = length - path.length(); + if (remaining == 1) { + path.append(generateNodeName(1)); + } else { + path.append('/'); + remaining--; + path.append(generateNodeName(1 + random.nextInt(Math.min(remaining, 150)))); + } } - return current; + return path.toString(); } - private static NodeState getParent(NodeState root) throws RepositoryException { - NodeState current = root; - for (String segment : getParentSegments()) { - current = current.getChildNode(segment); + private static String generateNodeName(int length) { + Random random = new Random(); + StringBuilder nodeName = new StringBuilder(); + for (int i = 0; i < length; i++) { + nodeName.append((char) ('a' + random.nextInt('z' - 'a'))); } - return current; + return nodeName.toString(); } - private static Iterable<String> getParentSegments() { - return limit(cycle("this", "is", "a", "path"), 100); // total path - // length - // = 350 + private NodeState getParent(NodeStore ns) { + NodeState node = ns.getRoot(); + for (String s : PathUtils.elements(parentPath)) { + node = node.getChildNode(s); + } + return node; } -} +} \ No newline at end of file