From 36ffdeeca0ac5290727b3bf5d2a7267664e9638a Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 16 Jun 2017 09:49:44 +0100 Subject: [PATCH 01/12] remove REST reference to removed method --- .../brooklyn/rest/transform/EntityTransformer.java | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/EntityTransformer.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/EntityTransformer.java index fcbe671e72..8a15020d36 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/EntityTransformer.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/EntityTransformer.java @@ -32,6 +32,7 @@ import org.apache.brooklyn.api.objs.SpecParameter; import org.apache.brooklyn.config.ConfigKey; import org.apache.brooklyn.core.config.render.RendererHints; +import org.apache.brooklyn.core.typereg.RegisteredTypes; import org.apache.brooklyn.rest.domain.EnricherConfigSummary; import org.apache.brooklyn.rest.domain.EntityConfigSummary; import org.apache.brooklyn.rest.domain.EntitySummary; @@ -90,19 +91,15 @@ public static EntitySummary entitySummary(Entity entity, UriBuilder ub) { .put("spec", URI.create(entityUri + "/spec")) ; - if (entity.getIconUrl()!=null) + if (RegisteredTypes.getIconUrl(entity)!=null) lb.put("iconUrl", URI.create(entityUri + "/icon")); if (entity.getCatalogItemId() != null) { String versionedId = entity.getCatalogItemId(); URI catalogUri; - if (CatalogUtils.looksLikeVersionedId(versionedId)) { - String symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(versionedId); - String version = CatalogUtils.getVersionFromVersionedId(versionedId); - catalogUri = serviceUriBuilder(ub, CatalogApi.class, "getEntity").build(symbolicName, version); - } else { - catalogUri = serviceUriBuilder(ub, CatalogApi.class, "getEntity_0_7_0").build(versionedId); - } + String symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(versionedId); + String version = CatalogUtils.getVersionFromVersionedId(versionedId); + catalogUri = serviceUriBuilder(ub, CatalogApi.class, "getEntity").build(symbolicName, version); lb.put("catalog", catalogUri); } From 3860c3498d63a9a9d880480363c7947599d40a7c Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 16 Jun 2017 11:53:33 +0100 Subject: [PATCH 02/12] new utilities for registered type (catalog item id) naming syntax --- .../core/typereg/RegisteredTypeNaming.java | 110 ++++++++++++++++++ .../typereg/RegisteredTypeNamingTest.java | 87 ++++++++++++++ 2 files changed, 197 insertions(+) create mode 100644 core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java create mode 100644 core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java b/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java new file mode 100644 index 0000000000..dc9dfe56c6 --- /dev/null +++ b/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java @@ -0,0 +1,110 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.brooklyn.core.typereg; + +/** + * Methods for testing validity of names and decomposing them. + * Mostly based on OSGi, specifically sections 1.3.2 and 3.2.5 of + * osgi.core-4.3.0 spec (https://www.osgi.org/release-4-version-4-3-download/). */ +public class RegisteredTypeNaming { + + private static final String USABLE_REGEX = "[^:\\s]+"; + + private final static String DOT = "\\."; + + public final static String OSGI_TOKEN_CHARS = "A-Za-z0-9_-"; + public final static String OSGI_TOKEN_REGEX = "[" + OSGI_TOKEN_CHARS + "]+"; + public final static String OSGI_SYMBOLIC_NAME_REGEX = OSGI_TOKEN_REGEX + "(" + DOT + OSGI_TOKEN_REGEX + ")*"; + public final static String NUMBER = "[0-9]+"; + public final static String QUALIFIER = OSGI_TOKEN_REGEX; + public final static String VERSION_REGEX = + NUMBER + + "(" + "\\." + NUMBER + + "(" + "\\." + NUMBER + + "(" + "\\." + QUALIFIER + + ")?" + + ")?" + + ")?"; + + private static boolean isUsable(String candidate) { + return candidate!=null && candidate.matches(USABLE_REGEX); + } + + /** + * For type names we currently work with any non-empty string that does not contain a ':' or whitespace. + * However we discourage things that are not OSGi symbolic names; see {@link #isValidTypeName(String)}. + * In some places (eg bundles) the use of OSGi symbolic names may be enforced. + */ + public static boolean isUsableTypeName(String candidate) { + return isUsable(candidate); + } + + /** + * We recommend type names be OSGi symbolic names, such as: + * + * com.acme-group.1-Virtual-Machine + * + * Note that this is more permissive than Java, allowing hyphens and + * allowing segments to start with numbers. + * However it is also more restrictive: OSGi does not allow + * accented characters or most punctuation. Only hyphens and underscores are allowed + * in segment names, and periods are allowed only as segment separators. + */ + public static boolean isGoodTypeName(String candidate) { + return isUsable(candidate) && candidate.matches(OSGI_SYMBOLIC_NAME_REGEX); + } + + /** + * For versions we currently work with any non-empty string that does not contain a ':' or whitespace. + * However we discourage things that are not OSGi versions; see {@link #isOsgiLegalVersion(String)}. + * In some places (eg bundles) the use of OSGi version syntax may be enforced. + */ + public static boolean isUsableVersion(String candidate) { + return isUsable(candidate); + } + + /** True if the argument matches the OSGi version spec, of the form + * MAJOR.MINOR.POINT.QUALIFIER or part thereof (not ending in a period though), + * where the first three are whole numbers and the final piece is any valid OSGi token + * (something satisfying {@link #isGoodTypeName(String)} with no periods). + */ + public static boolean isOsgiLegalVersion(String candidate) { + return candidate!=null && candidate.matches(VERSION_REGEX); + } + + /** True if the argument has exactly one colon, and the part before + * satisfies {@link #isUsableTypeName(String)} and the part after {@link #isUsableVersion(String)}. */ + public static boolean isUsableTypeColonVersion(String candidate) { + // simplify regex, rather than decomposing and calling the methods referenced in the javadoc + return candidate!=null && candidate.matches(USABLE_REGEX + ":" + USABLE_REGEX); + } + + /** True if the argument has exactly one colon, and the part before + * satisfies {@link #isGoodTypeName(String)} and the part after + * {@link #isOsgiLegalVersion(String)}. */ + public static boolean isGoodTypeColonVersion(String candidate) { + if (candidate==null) return false; + int idx = candidate.indexOf(':'); + if (idx<=0) return false; + if (!isGoodTypeName(candidate.substring(0, idx))) return false; + if (!isOsgiLegalVersion(candidate.substring(idx+1))) return false; + return true; + } + +} diff --git a/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java b/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java new file mode 100644 index 0000000000..81b69bf029 --- /dev/null +++ b/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.brooklyn.core.typereg; + +import org.testng.Assert; +import org.testng.annotations.Test; + +@Test +public class RegisteredTypeNamingTest { + + public void testNames() { + assertName("foo", true, true); + assertName("foo-bar", true, true); + assertName("a-package.1-foo-bar", true, true); + + assertName("", false, false); + assertName(null, false, false); + assertName("foo:1", false, false); + assertName("foo bar", false, false); + + assertName(".foo", true, false); + assertName("package..foo", true, false); + assertName("package..foo", true, false); + assertName("!$&", true, false); + } + + public void testVersions() { + assertVersion("1", true, true); + assertVersion("1.0.0", true, true); + assertVersion("1.0.0.SNAPSHOT", true, true); + + assertVersion("", false, false); + assertVersion(null, false, false); + assertVersion("1:1", false, false); + + assertVersion("1.SNAPSHOT", true, false); + assertVersion("1.0.0_SNAPSHOT", true, false); + assertVersion(".1", true, false); + assertVersion("v1", true, false); + assertVersion("!$&", true, false); + } + + public void testNameColonVersion() { + assertNameColonVersion("foo:1", true, true); + assertNameColonVersion("1:1", true, true); + assertNameColonVersion("a-package.1-foo-bar:1.0.0.SNAPSHOT_dev", true, true); + + assertNameColonVersion("", false, false); + assertNameColonVersion(null, false, false); + assertNameColonVersion("foo:", false, false); + assertNameColonVersion(":1", false, false); + + assertNameColonVersion("foo:1.SNAPSHOT", true, false); + assertNameColonVersion("foo:v1", true, false); + assertNameColonVersion("foo...bar!:1", true, false); + } + + private void assertName(String candidate, boolean isUsable, boolean isGood) { + Assert.assertEquals(RegisteredTypeNaming.isUsableTypeName(candidate), isUsable, "usable name '"+candidate+"'"); + Assert.assertEquals(RegisteredTypeNaming.isGoodTypeName(candidate), isGood, "good name '"+candidate+"'"); + } + private void assertVersion(String candidate, boolean isUsable, boolean isGood) { + Assert.assertEquals(RegisteredTypeNaming.isUsableVersion(candidate), isUsable, "usable version '"+candidate+"'"); + Assert.assertEquals(RegisteredTypeNaming.isOsgiLegalVersion(candidate), isGood, "good version '"+candidate+"'"); + } + private void assertNameColonVersion(String candidate, boolean isUsable, boolean isGood) { + Assert.assertEquals(RegisteredTypeNaming.isUsableTypeColonVersion(candidate), isUsable, "usable name:version '"+candidate+"'"); + Assert.assertEquals(RegisteredTypeNaming.isGoodTypeColonVersion(candidate), isGood, "good name:version '"+candidate+"'"); + } + +} From 607fc853aa395915e15d527405b7a7a8a6e97b48 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 16 Jun 2017 13:24:28 +0100 Subject: [PATCH 03/12] move many things to use RegisteredTypeNaming rather than hokey `looksLikeVersionedId` --- .../brooklyn/api/objs/BrooklynObject.java | 3 + .../internal/BasicBrooklynCatalog.java | 98 +++++++++++++++---- .../internal/CatalogItemDtoAbstract.java | 25 ++++- .../core/catalog/internal/CatalogUtils.java | 17 ++-- .../core/mgmt/rebind/RebindIteration.java | 5 +- .../typereg/BasicBrooklynTypeRegistry.java | 4 +- .../core/typereg/RegisteredTypeNaming.java | 5 +- .../internal/CatalogVersioningTest.java | 23 +++-- .../rest/util/BrooklynRestResourceUtils.java | 66 ------------- .../brooklyn/rest/util/WebResourceUtils.java | 3 +- 10 files changed, 143 insertions(+), 106 deletions(-) diff --git a/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java b/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java index 265813a370..1d3498258b 100644 --- a/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java +++ b/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java @@ -56,6 +56,9 @@ public interface BrooklynObject extends Identifiable, Configurable { *

* In some cases this may be set heuristically from context and so may not be accurate. * Callers can set an explicit catalog item ID if inferencing is not correct. + *

+ * This should conform to OSGi specs for symbolic_name:version + * but weaker semantics are usually allowed so long as neither segment contains a : or whitespace. */ String getCatalogItemId(); diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java index a3a4e7f1dd..6333e774f5 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java @@ -46,6 +46,7 @@ import org.apache.brooklyn.core.mgmt.internal.CampYamlParser; import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal; import org.apache.brooklyn.core.typereg.BrooklynTypePlanTransformer; +import org.apache.brooklyn.core.typereg.RegisteredTypeNaming; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.collections.MutableSet; @@ -600,14 +601,34 @@ private void collectCatalogItems(String sourceYaml, Map itemMetadata, List< // if symname not set, infer from: id, then name, then item id, then item name if (Strings.isBlank(symbolicName)) { if (Strings.isNonBlank(id)) { - if (CatalogUtils.looksLikeVersionedId(id)) { + if (RegisteredTypeNaming.isGoodTypeColonVersion(id)) { symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(id); + } else if (CatalogUtils.looksLikeVersionedId(id)) { + // use of above method is deprecated in 0.12; this block can be removed in 0.13 + log.warn("Discouraged version syntax in id '"+id+"'; version should comply with OSGi specs (#.#.#.qualifier or portion) or specify symbolic name and version explicitly"); + symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(id); + } else if (RegisteredTypeNaming.isUsableTypeColonVersion(id)) { + log.warn("Deprecated type naming syntax in id '"+id+"'; colons not allowed in type name as it is used to indicate version"); + // deprecated in 0.12; from 0.13 this can change to treat part after the colon as version, also see line to set version below + // (may optionally warn or disallow if we want to require OSGi versions) + // symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(id); + symbolicName = id; } else { symbolicName = id; } } else if (Strings.isNonBlank(name)) { - if (CatalogUtils.looksLikeVersionedId(name)) { + if (RegisteredTypeNaming.isGoodTypeColonVersion(name)) { + log.warn("Deprecated use of 'name' key to define '"+name+"'; version should be specified within 'id' key or with 'version' key, not this tag"); + // deprecated in 0.12; remove in 0.13 + symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(name); + } else if (CatalogUtils.looksLikeVersionedId(name)) { + log.warn("Deprecated use of 'name' key to define '"+name+"'; version should be specified within 'id' key or with 'version' key, not this tag"); + // deprecated in 0.12; remove in 0.13 symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(name); + } else if (RegisteredTypeNaming.isUsableTypeColonVersion(name)) { + log.warn("Deprecated type naming syntax in id '"+id+"'; colons not allowed in type name as it is used to indicate version"); + // deprecated in 0.12; throw error if we want in 0.13 + symbolicName = name; } else { symbolicName = name; } @@ -623,18 +644,35 @@ private void collectCatalogItems(String sourceYaml, Map itemMetadata, List< } } + String versionFromId = null; + if (RegisteredTypeNaming.isGoodTypeColonVersion(id)) { + versionFromId = CatalogUtils.getVersionFromVersionedId(id); + } else if (CatalogUtils.looksLikeVersionedId(id)) { + log.warn("Discouraged version syntax in id '"+id+"'; version should comply with OSGi specs (#.#.#.qualifier or portion) or specify symbolic name and version explicitly"); + // remove in 0.13 + versionFromId = CatalogUtils.getVersionFromVersionedId(id); + } else if (RegisteredTypeNaming.isUsableTypeColonVersion(id)) { + // deprecated in 0.12, with warning above; from 0.13 this can be uncommented to treat part after the colon as version + // (may optionally warn or disallow if we want to require OSGi versions) + // if comparable section above is changed, change this to: + // versionFromId = CatalogUtils.getVersionFromVersionedId(id); + } + // if version not set, infer from: id, then from name, then item version - if (CatalogUtils.looksLikeVersionedId(id)) { - String versionFromId = CatalogUtils.getVersionFromVersionedId(id); - if (versionFromId != null && Strings.isNonBlank(version) && !versionFromId.equals(version)) { + if (versionFromId!=null) { + if (Strings.isNonBlank(version) && !versionFromId.equals(version)) { throw new IllegalArgumentException("Discrepency between version set in id " + versionFromId + " and version property " + version); } version = versionFromId; } + if (Strings.isBlank(version)) { if (CatalogUtils.looksLikeVersionedId(name)) { + // deprecated in 0.12, remove in 0.13 + log.warn("Deprecated use of 'name' key to define '"+name+"'; version should be specified within 'id' key or with 'version' key, not this tag"); version = CatalogUtils.getVersionFromVersionedId(name); - } else if (Strings.isBlank(version)) { + } + if (Strings.isBlank(version)) { version = setFromItemIfUnset(version, itemAsMap, "version"); version = setFromItemIfUnset(version, itemAsMap, "template_version"); if (version==null) { @@ -854,24 +892,50 @@ private boolean attemptType(String key, CatalogItemType candidateCiType) { } // first look in collected items, if a key is given String type = (String) item.get("type"); - String version = null; - if (CatalogUtils.looksLikeVersionedId(type)) { - version = CatalogUtils.getVersionFromVersionedId(type); - type = CatalogUtils.getSymbolicNameFromVersionedId(type); - } + if (type!=null && key!=null) { for (CatalogItemDtoAbstract candidate: itemsDefinedSoFar) { if (candidateCiType == candidate.getCatalogItemType() && (type.equals(candidate.getSymbolicName()) || type.equals(candidate.getId()))) { - if (version==null || version.equals(candidate.getVersion())) { - // matched - exit - catalogItemType = candidateCiType; - planYaml = candidateYaml; - resolved = true; - return true; + // matched - exit + catalogItemType = candidateCiType; + planYaml = candidateYaml; + resolved = true; + return true; + } + } + } + { + // legacy routine; should be the same as above code added in 0.12 because: + // if type is symbolic_name, the type will match below, and version will be null so any version allowed to match + // if type is symbolic_name:version, the id will match, and the version will also have to match + // SHOULD NEVER NEED THIS - remove during or after 0.13 + String typeWithId = type; + String version = null; + if (CatalogUtils.looksLikeVersionedId(type)) { + version = CatalogUtils.getVersionFromVersionedId(type); + type = CatalogUtils.getSymbolicNameFromVersionedId(type); + } + if (type!=null && key!=null) { + for (CatalogItemDtoAbstract candidate: itemsDefinedSoFar) { + if (candidateCiType == candidate.getCatalogItemType() && + (type.equals(candidate.getSymbolicName()) || type.equals(candidate.getId()))) { + if (version==null || version.equals(candidate.getVersion())) { + log.warn("Lookup of '"+type+"' version '"+version+"' only worked using legacy routines; please advise Brooklyn community so they understand why"); + // matched - exit + catalogItemType = candidateCiType; + planYaml = candidateYaml; + resolved = true; + return true; + } } } } + + type = typeWithId; + // above line is a change to behaviour; previously we proceeded below with the version dropped in code above; + // but that seems like a bug as the code below will have ignored version. + // likely this means we are now stricter about loading things that reference new versions, but correctly so. } // then try parsing plan - this will use loader diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java index 4644d8d901..3f69e677ad 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java @@ -33,6 +33,7 @@ import org.apache.brooklyn.core.mgmt.rebind.BasicCatalogItemRebindSupport; import org.apache.brooklyn.core.objs.AbstractBrooklynObject; import org.apache.brooklyn.core.relations.EmptyRelationSupport; +import org.apache.brooklyn.core.typereg.RegisteredTypeNaming; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.core.flags.FlagUtils; import org.apache.brooklyn.util.core.flags.SetFromFlag; @@ -386,6 +387,16 @@ protected void setTags(Set tags) { /** * Parses an instance of CatalogLibrariesDto from the given List. Expects the list entries * to be either Strings or Maps of String -> String. Will skip items that are not. + *

+ * If a string is supplied, this tries heuristically to identify whether a reference is a bundle or a URL, as follows: + * - if the string contains a slash, it is treated as a URL (or classpath reference), e.g. /file.txt; + * - if the string is {@link RegisteredTypeNaming#isGoodTypeColonVersion(String)} with an OSGi version it is treated as a bundle, e.g. file:1; + * - if the string is ambiguous (has a single colon) a warning is given, + * and typically it is treated as a URL because OSGi versions are needed here, e.g. file:v1 is a URL, + * but for a transitional period (possibly ending in 0.13 as warning is introduced in 0.12) for compatibility with previous versions, + * versions starting with a number trigger bundle resolution, e.g. file:1.txt is a bundle for now + * (but in all these cases warnings are logged) + * - otherwise (multiple colons, or no colons) it is treated like a URL */ public static Collection parseLibraries(Collection possibleLibraries) { Collection dto = MutableList.of(); @@ -405,16 +416,28 @@ public static Collection parseLibraries(Collection possibleLib //Infer reference type (heuristically) if (inlineRef.contains("/") || inlineRef.contains("\\")) { - //looks like an url/file path + //looks like an url/file path (note these chars now formally disallowed in type names) name = null; version = null; url = inlineRef; + } else if (RegisteredTypeNaming.isGoodTypeColonVersion(inlineRef)) { + //looks like a name+version ref + name = CatalogUtils.getSymbolicNameFromVersionedId(inlineRef); + version = CatalogUtils.getVersionFromVersionedId(inlineRef); + url = null; } else if (CatalogUtils.looksLikeVersionedId(inlineRef)) { + LOG.warn("Reference to library "+inlineRef+" is being treated as type but deprecated version syntax " + + "means in subsequent versions it will be treated as a URL."); //looks like a name+version ref name = CatalogUtils.getSymbolicNameFromVersionedId(inlineRef); version = CatalogUtils.getVersionFromVersionedId(inlineRef); url = null; } else { + if (RegisteredTypeNaming.isUsableTypeColonVersion(inlineRef)) { + LOG.warn("Ambiguous library reference "+inlineRef+" is being treated as a URL even though" + + " it looks a bit like a bundle with a non-osgi-version. Use strict OSGi versions to treat as a bundle. " + + "To suppress this message and force URL use a slash in the reference "); + } //assume it to be relative url name = null; version = null; diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java index 47dab6d5f7..1d8198e9c1 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java @@ -46,6 +46,7 @@ import org.apache.brooklyn.core.objs.BrooklynObjectInternal; import org.apache.brooklyn.core.typereg.BasicManagedBundle; import org.apache.brooklyn.core.typereg.RegisteredTypeLoadingContexts; +import org.apache.brooklyn.core.typereg.RegisteredTypeNaming; import org.apache.brooklyn.core.typereg.RegisteredTypePredicates; import org.apache.brooklyn.core.typereg.RegisteredTypes; import org.apache.brooklyn.util.collections.MutableList; @@ -232,6 +233,10 @@ public static void logDebugOrTraceIfRebinding(Logger log, String message, Object log.debug(message, args); } + /** @deprecated since 0.12.0 - the "version starts with number" test this does is hokey; use + * either {@link RegisteredTypeNaming#isUsableTypeColonVersion(String)} for weak enforcement + * or {@link RegisteredTypeNaming#isGoodTypeColonVersion(String)} for OSGi enforcement. */ + @Deprecated public static boolean looksLikeVersionedId(String versionedId) { if (versionedId==null) return false; int fi = versionedId.indexOf(VERSION_DELIMITER); @@ -248,16 +253,14 @@ public static boolean looksLikeVersionedId(String versionedId) { // e.g. foo:1 or foo:1.1 or foo:1_SNAPSHOT all supported, but not e.g. foo:bar (or chef:cookbook or docker:my/image) return false; } + if (!RegisteredTypeNaming.isUsableTypeColonVersion(versionedId)) { + // arguments that contain / or whitespace will pass here but calling code will likely be changed not to support it + log.warn("Reference '"+versionedId+"' is being treated as a versioned type but it " + + "contains deprecated characters (slashes or whitespace); likely to be unsupported in future versions."); + } return true; } - /** @deprecated since 0.9.0 use {@link #getSymbolicNameFromVersionedId(String)} */ - // all uses removed - @Deprecated - public static String getIdFromVersionedId(String versionedId) { - return getSymbolicNameFromVersionedId(versionedId); - } - public static String getSymbolicNameFromVersionedId(String versionedId) { if (versionedId == null) return null; int versionDelimiterPos = versionedId.lastIndexOf(VERSION_DELIMITER); diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java index e92a774dbe..a80b14a00e 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java @@ -99,6 +99,7 @@ import org.apache.brooklyn.core.objs.proxy.InternalPolicyFactory; import org.apache.brooklyn.core.policy.AbstractPolicy; import org.apache.brooklyn.core.typereg.BasicManagedBundle; +import org.apache.brooklyn.core.typereg.RegisteredTypeNaming; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.core.ClassLoaderUtils; @@ -1086,7 +1087,9 @@ protected LoadedClass load(Class bTyp // See https://issues.apache.org/jira/browse/BROOKLYN-149 // This is a dangling reference to the catalog item (which will have been logged by lookupCatalogItem). // Try loading as any version. - if (CatalogUtils.looksLikeVersionedId(catalogItemId)) { + if (RegisteredTypeNaming.isUsableTypeColonVersion(catalogItemId) || + // included through 0.12 so legacy type names are accepted (with warning) + CatalogUtils.looksLikeVersionedId(catalogItemId)) { String symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(catalogItemId); catalogItem = rebindContext.lookup().lookupCatalogItem(symbolicName); diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java index 0c2c87ba2a..468664b842 100644 --- a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java +++ b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java @@ -153,7 +153,9 @@ public RegisteredType get(String symbolicNameWithOptionalVersion, RegisteredType @Override public Maybe getMaybe(String symbolicNameWithOptionalVersion, RegisteredTypeLoadingContext context) { Maybe r1 = null; - if (CatalogUtils.looksLikeVersionedId(symbolicNameWithOptionalVersion)) { + if (RegisteredTypeNaming.isUsableTypeColonVersion(symbolicNameWithOptionalVersion) || + // included through 0.12 so legacy type names are accepted (with warning) + CatalogUtils.looksLikeVersionedId(symbolicNameWithOptionalVersion)) { String symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(symbolicNameWithOptionalVersion); String version = CatalogUtils.getVersionFromVersionedId(symbolicNameWithOptionalVersion); r1 = getSingle(symbolicName, version, context); diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java b/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java index dc9dfe56c6..22e9e7885a 100644 --- a/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java +++ b/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java @@ -24,7 +24,7 @@ * osgi.core-4.3.0 spec (https://www.osgi.org/release-4-version-4-3-download/). */ public class RegisteredTypeNaming { - private static final String USABLE_REGEX = "[^:\\s]+"; + private static final String USABLE_REGEX = "[^:\\s/\\\\]+"; private final static String DOT = "\\."; @@ -47,7 +47,8 @@ private static boolean isUsable(String candidate) { } /** - * For type names we currently work with any non-empty string that does not contain a ':' or whitespace. + * For type names we currently work with any non-empty string that does not contain + * a ':' or whitespace or forward slash or backslash. * However we discourage things that are not OSGi symbolic names; see {@link #isValidTypeName(String)}. * In some places (eg bundles) the use of OSGi symbolic names may be enforced. */ diff --git a/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogVersioningTest.java b/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogVersioningTest.java index 0ca2171bed..3c09bb9e09 100644 --- a/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogVersioningTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogVersioningTest.java @@ -35,6 +35,7 @@ import org.apache.brooklyn.core.entity.Entities; import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext; import org.apache.brooklyn.core.test.entity.LocalManagementContextForTests; +import org.apache.brooklyn.core.typereg.RegisteredTypeNaming; import com.google.common.base.Predicates; import com.google.common.collect.Iterables; @@ -54,19 +55,21 @@ public void tearDown() throws Exception { if (managementContext != null) Entities.destroyAll(managementContext); } + /** @deprecated since 0.12.0; replaced with {@link RegisteredTypeNaming} */ @Test - public void testParsingVersion() { - assertVersionParsesAs("foo:1", "foo", "1"); - assertVersionParsesAs("foo", null, null); - assertVersionParsesAs("foo:1.1", "foo", "1.1"); - assertVersionParsesAs("foo:1_SNAPSHOT", "foo", "1_SNAPSHOT"); - assertVersionParsesAs("foo:10.9.8_SNAPSHOT", "foo", "10.9.8_SNAPSHOT"); - assertVersionParsesAs("foo:bar", null, null); - assertVersionParsesAs("chef:cookbook", null, null); - assertVersionParsesAs("http://foo:8080", null, null); + @Deprecated + public void testLegacyParsingVersion() { + assertLegacyVersionParsesAs("foo:1", "foo", "1"); + assertLegacyVersionParsesAs("foo", null, null); + assertLegacyVersionParsesAs("foo:1.1", "foo", "1.1"); + assertLegacyVersionParsesAs("foo:1_SNAPSHOT", "foo", "1_SNAPSHOT"); + assertLegacyVersionParsesAs("foo:10.9.8_SNAPSHOT", "foo", "10.9.8_SNAPSHOT"); + assertLegacyVersionParsesAs("foo:bar", null, null); + assertLegacyVersionParsesAs("chef:cookbook", null, null); + assertLegacyVersionParsesAs("http://foo:8080", null, null); } - private static void assertVersionParsesAs(String versionedId, String id, String version) { + private static void assertLegacyVersionParsesAs(String versionedId, String id, String version) { if (version==null) { Assert.assertFalse(CatalogUtils.looksLikeVersionedId(versionedId)); } else { diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/BrooklynRestResourceUtils.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/BrooklynRestResourceUtils.java index d9eb7ee70d..47920696ca 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/BrooklynRestResourceUtils.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/BrooklynRestResourceUtils.java @@ -23,7 +23,6 @@ import java.lang.reflect.Constructor; import java.util.ArrayList; -import java.util.Collection; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -34,7 +33,6 @@ import javax.ws.rs.core.MediaType; import org.apache.brooklyn.api.catalog.BrooklynCatalog; -import org.apache.brooklyn.api.catalog.CatalogItem; import org.apache.brooklyn.api.entity.Application; import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.location.Location; @@ -47,9 +45,6 @@ import org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.DslComponent; import org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.DslComponent.Scope; import org.apache.brooklyn.config.ConfigKey; -import org.apache.brooklyn.core.catalog.CatalogPredicates; -import org.apache.brooklyn.core.catalog.internal.CatalogItemComparator; -import org.apache.brooklyn.core.catalog.internal.CatalogUtils; import org.apache.brooklyn.core.entity.Attributes; import org.apache.brooklyn.core.entity.Entities; import org.apache.brooklyn.core.entity.EntityInternal; @@ -58,7 +53,6 @@ import org.apache.brooklyn.core.mgmt.entitlement.Entitlements; import org.apache.brooklyn.core.mgmt.entitlement.Entitlements.StringAndArgument; import org.apache.brooklyn.core.objs.BrooklynTypes; -import org.apache.brooklyn.core.typereg.RegisteredTypes; import org.apache.brooklyn.enricher.stock.Enrichers; import org.apache.brooklyn.entity.stock.BasicApplication; import org.apache.brooklyn.rest.domain.ApplicationSpec; @@ -76,9 +70,7 @@ import com.google.common.base.Function; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; -import com.google.common.base.Predicates; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -216,20 +208,6 @@ private class FindItemAndClass { @SuppressWarnings({ "unchecked" }) private FindItemAndClass inferFrom(String type) { RegisteredType item = mgmt.getTypeRegistry().get(type); - if (item==null) { - // deprecated attempt to load an item not in the type registry - - // although the method called was deprecated in 0.7.0, its use here was not warned until 0.9.0; - // therefore this behaviour should not be changed until after 0.9.0; - // at which point it should try a pojo load (see below) - item = getCatalogItemForType(type); - if (item!=null) { - log.warn("Creating application for requested type `"+type+" using item "+item+"; " - + "the registered type name ("+item.getSymbolicName()+") should be used from the spec instead, " - + "or the type registered under its own name. " - + "Future versions will likely change semantics to attempt a POJO load of the type instead."); - } - } if (item != null) { return setAs( @@ -254,50 +232,6 @@ private FindItemAndClass setAs(Class clazz, String catalogItem this.catalogItemId = catalogItemId; return this; } - - @Deprecated // see caller - private RegisteredType getCatalogItemForType(String typeName) { - final RegisteredType resultI; - if (CatalogUtils.looksLikeVersionedId(typeName)) { - //All catalog identifiers of the form aaaa:bbbb are composed of symbolicName+version. - //No javaType is allowed as part of the identifier. - resultI = mgmt.getTypeRegistry().get(typeName); - } else { - //Usually for catalog items with javaType (that is items from catalog.xml) - //the symbolicName and javaType match because symbolicName (was ID) - //is not specified explicitly. But could be the case that there is an item - //whose symbolicName is explicitly set to be different from the javaType. - //Note that in the XML the attribute is called registeredTypeName. - Iterable> resultL = mgmt.getCatalog().getCatalogItems(CatalogPredicates.javaType(Predicates.equalTo(typeName))); - if (!Iterables.isEmpty(resultL)) { - //Push newer versions in front of the list (not that there should - //be more than one considering the items are coming from catalog.xml). - resultI = RegisteredTypes.of(sortVersionsDesc(resultL).iterator().next()); - if (log.isDebugEnabled() && Iterables.size(resultL)>1) { - log.debug("Found "+Iterables.size(resultL)+" matches in catalog for type "+typeName+"; returning the result with preferred version, "+resultI); - } - } else { - //As a last resort try searching for items with the same symbolicName supposedly - //different from the javaType. - resultI = mgmt.getTypeRegistry().get(typeName, BrooklynCatalog.DEFAULT_VERSION); - if (resultI != null) { - if (resultI.getSuperTypes().isEmpty()) { - //Catalog items scanned from the classpath (using reflection and annotations) now - //get yaml spec rather than a java type. Can't use those when creating apps from - //the legacy app spec format. - log.warn("Unable to find catalog item for type "+typeName + - ". There is an existing catalog item with ID " + resultI.getId() + - " but it doesn't define a class type."); - return null; - } - } - } - } - return resultI; - } - private Collection> sortVersionsDesc(Iterable> versions) { - return ImmutableSortedSet.orderedBy(CatalogItemComparator.getInstance()).addAll(versions).build(); - } } @SuppressWarnings({ "deprecation" }) diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/WebResourceUtils.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/WebResourceUtils.java index 5e8b7f9555..bd7a7405e6 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/WebResourceUtils.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/WebResourceUtils.java @@ -30,6 +30,7 @@ import org.apache.brooklyn.api.mgmt.ManagementContext; import org.apache.brooklyn.core.catalog.internal.CatalogUtils; +import org.apache.brooklyn.core.typereg.RegisteredTypeNaming; import org.apache.brooklyn.rest.domain.ApiError; import org.apache.brooklyn.rest.util.json.BrooklynJacksonJsonProvider; import org.apache.brooklyn.util.exceptions.Exceptions; @@ -175,7 +176,7 @@ public static Object getValueForDisplay(ObjectMapper mapper, Object value, boole } public static String getPathFromVersionedId(String versionedId) { - if (CatalogUtils.looksLikeVersionedId(versionedId)) { + if (RegisteredTypeNaming.isUsableTypeColonVersion(versionedId)) { String symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(versionedId); String version = CatalogUtils.getVersionFromVersionedId(versionedId); return Urls.encode(symbolicName) + "/" + Urls.encode(version); From eb08f8ec0d65b59bb2c4201a98632aa070ddac97 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 16 Jun 2017 13:38:45 +0100 Subject: [PATCH 04/12] notes on related methods for looking up catalog items flexibly --- .../creation/service/TestServiceTypeResolver.java | 4 ++-- .../brooklyn/core/catalog/internal/CatalogUtils.java | 5 +++-- .../apache/brooklyn/util/core/ClassLoaderUtils.java | 12 ++++++++++++ 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/TestServiceTypeResolver.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/TestServiceTypeResolver.java index 7fd6d8a91e..1c13211c47 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/TestServiceTypeResolver.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/TestServiceTypeResolver.java @@ -22,7 +22,6 @@ import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.entity.EntitySpec; import org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynComponentTemplateResolver; -import org.apache.brooklyn.core.catalog.internal.CatalogUtils; import org.apache.brooklyn.util.text.Strings; @SuppressWarnings("deprecation") @@ -43,7 +42,8 @@ public String getBrooklynType(String serviceType) { @SuppressWarnings("unchecked") @Override public CatalogItem> getCatalogItem(BrooklynComponentTemplateResolver resolver, String serviceType) { - return (CatalogItem>) CatalogUtils.getCatalogItemOptionalVersion(resolver.getManagementContext(), getBrooklynType(serviceType)); + return (CatalogItem>) + resolver.getManagementContext().getCatalog().getCatalogItem(getBrooklynType(serviceType), null); } @Override diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java index 1d8198e9c1..bc744a0980 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java @@ -236,6 +236,7 @@ public static void logDebugOrTraceIfRebinding(Logger log, String message, Object /** @deprecated since 0.12.0 - the "version starts with number" test this does is hokey; use * either {@link RegisteredTypeNaming#isUsableTypeColonVersion(String)} for weak enforcement * or {@link RegisteredTypeNaming#isGoodTypeColonVersion(String)} for OSGi enforcement. */ + // several references, but all deprecated, most safe to remove after a cycle or two and bad verisons have been warned @Deprecated public static boolean looksLikeVersionedId(String versionedId) { if (versionedId==null) return false; @@ -287,7 +288,7 @@ public static String getVersionedId(String id, String version) { } /** @deprecated since 0.9.0 use {@link BrooklynTypeRegistry#get(String, org.apache.brooklyn.api.typereg.BrooklynTypeRegistry.RegisteredTypeKind, Class)} */ - // only a handful of items remaining, and those require a CatalogItem + // only a handful of items remaining, requiring a CatalogItem: two in REST, one in rebind, and one in ClassLoaderUtils @Deprecated public static CatalogItem getCatalogItemOptionalVersion(ManagementContext mgmt, String versionedId) { if (versionedId == null) return null; @@ -308,7 +309,7 @@ public static boolean isBestVersion(ManagementContext mgmt, CatalogItem ite } /** @deprecated since 0.9.0 use {@link BrooklynTypeRegistry#get(String, org.apache.brooklyn.api.typereg.BrooklynTypeRegistry.RegisteredTypeKind, Class)} */ - // only a handful of items remaining, and those require a CatalogItem + // only one item left, in deprecated service resolver @Deprecated public static CatalogItem getCatalogItemOptionalVersion(ManagementContext mgmt, Class type, String versionedId) { if (looksLikeVersionedId(versionedId)) { diff --git a/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java b/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java index f6079398c0..a6be53d081 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java @@ -253,6 +253,17 @@ private Maybe loadClass(String name, LoaderDispatcher dispatcher, Stri if (entity != null && mgmt != null) { String catalogItemId = entity.getCatalogItemId(); if (catalogItemId != null) { +// RegisteredType type = mgmt.getTypeRegistry().get(catalogItemId); +// if (type != null) { +// BrooklynClassLoadingContextSequential loader = new BrooklynClassLoadingContextSequential(mgmt); +// loader.add(newClassLoadingContextForCatalogItems(mgmt, type.getId(), +// type.getContainingBundle() + type.getLibraries() ?); +// cls = dispatcher.tryLoadFrom(loader, className); +// if (cls.isPresent()) { +// return cls; +// } + // TODO prefer above to below, but need to reconcile item.searchPath with RegisteredType? + // or use entity search path ? CatalogItem item = CatalogUtils.getCatalogItemOptionalVersion(mgmt, catalogItemId); if (item != null) { BrooklynClassLoadingContextSequential loader = new BrooklynClassLoadingContextSequential(mgmt); @@ -262,6 +273,7 @@ private Maybe loadClass(String name, LoaderDispatcher dispatcher, Stri if (cls.isPresent()) { return cls; } + } else { log.warn("Entity " + entity + " refers to non-existent catalog item " + catalogItemId + ". Trying to load class " + name); From c9d03c93e35eb5417ca6168207f4b0ddc3de9e7c Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 16 Jun 2017 13:46:20 +0100 Subject: [PATCH 05/12] remove deprecated CAMP ServiceTypeResolver changed a while ago to EntitySpecResolver; removes references to catalog items and hokey lookup --- .../BrooklynComponentTemplateResolver.java | 9 +-- .../service/BrooklynServiceTypeResolver.java | 78 ------------------- .../creation/service/ServiceTypeResolver.java | 77 ------------------ .../service/ServiceTypeResolverAdaptor.java | 70 ----------------- .../service/ServiceTypeResolverTest.java | 39 ---------- .../service/TestServiceTypeResolver.java | 54 ------------- 6 files changed, 1 insertion(+), 326 deletions(-) delete mode 100644 camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/BrooklynServiceTypeResolver.java delete mode 100644 camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolver.java delete mode 100644 camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolverAdaptor.java delete mode 100644 camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolverTest.java delete mode 100644 camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/TestServiceTypeResolver.java diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java index 7e9b56279b..ae92c922ae 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java @@ -30,7 +30,6 @@ import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.entity.EntitySpec; -import org.apache.brooklyn.api.framework.FrameworkLookup; import org.apache.brooklyn.api.location.LocationSpec; import org.apache.brooklyn.api.mgmt.ManagementContext; import org.apache.brooklyn.api.mgmt.classloading.BrooklynClassLoadingContext; @@ -39,8 +38,6 @@ import org.apache.brooklyn.camp.brooklyn.BrooklynCampConstants; import org.apache.brooklyn.camp.brooklyn.BrooklynCampReservedKeys; import org.apache.brooklyn.camp.brooklyn.spi.creation.service.CampServiceSpecResolver; -import org.apache.brooklyn.camp.brooklyn.spi.creation.service.ServiceTypeResolver; -import org.apache.brooklyn.camp.brooklyn.spi.creation.service.ServiceTypeResolverAdaptor; import org.apache.brooklyn.camp.spi.AbstractResource; import org.apache.brooklyn.camp.spi.ApplicationComponentTemplate; import org.apache.brooklyn.camp.spi.AssemblyTemplate; @@ -82,7 +79,6 @@ * This generates instances of a template resolver that use a {@link ServiceTypeResolver} * to parse the {@code serviceType} line in the template. */ -@SuppressWarnings("deprecation") // Because of ServiceTypeResolver public class BrooklynComponentTemplateResolver { private static final Logger log = LoggerFactory.getLogger(BrooklynComponentTemplateResolver.class); @@ -197,10 +193,7 @@ public EntitySpec resolveSpec(Set encounteredRegis private List getServiceTypeResolverOverrides() { List overrides = new ArrayList<>(); - Iterable loader = FrameworkLookup.lookupAll(ServiceTypeResolver.class, mgmt.getCatalogClassLoader()); - for (ServiceTypeResolver resolver : loader) { - overrides.add(new ServiceTypeResolverAdaptor(this, resolver)); - } + // none for now -- previously supported ServiceTypeResolver service return overrides; } diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/BrooklynServiceTypeResolver.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/BrooklynServiceTypeResolver.java deleted file mode 100644 index f9c07e4e2a..0000000000 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/BrooklynServiceTypeResolver.java +++ /dev/null @@ -1,78 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.brooklyn.camp.brooklyn.spi.creation.service; - -import javax.annotation.Nullable; - -import org.apache.brooklyn.api.catalog.CatalogItem; -import org.apache.brooklyn.api.entity.Entity; -import org.apache.brooklyn.api.entity.EntitySpec; -import org.apache.brooklyn.api.mgmt.ManagementContext; -import org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynComponentTemplateResolver; -import org.apache.brooklyn.camp.spi.PlatformComponentTemplate; -import org.apache.brooklyn.core.catalog.internal.CatalogUtils; -import org.apache.brooklyn.core.mgmt.persist.DeserializingClassRenamesProvider; -import org.apache.brooklyn.core.resolve.entity.AbstractEntitySpecResolver; -import org.apache.brooklyn.util.text.Strings; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -/** - * This converts {@link PlatformComponentTemplate} instances whose type is prefixed {@code brooklyn:} - * to Brooklyn {@link EntitySpec} instances. - * - * @deprecated since 0.9.0, use {@link AbstractEntitySpecResolver} instead - */ -@Deprecated -public class BrooklynServiceTypeResolver implements ServiceTypeResolver { - - @SuppressWarnings("unused") - private static final Logger LOG = LoggerFactory.getLogger(ServiceTypeResolver.class); - - public BrooklynServiceTypeResolver() { - } - - @Override - public String getTypePrefix() { return DEFAULT_TYPE_PREFIX; } - - @Override - public String getBrooklynType(String serviceType) { - return Strings.removeFromStart(serviceType, getTypePrefix() + ":").trim(); - } - - @Nullable - @Override - public CatalogItem> getCatalogItem(BrooklynComponentTemplateResolver resolver, String serviceType) { - String type = getBrooklynType(serviceType); - if (type != null) { - return getCatalogItemImpl(resolver.getManagementContext(), type); - } else { - return null; - } - } - - @Override - public void decorateSpec(BrooklynComponentTemplateResolver resolver, EntitySpec spec) { - } - - protected CatalogItem> getCatalogItemImpl(ManagementContext mgmt, String brooklynType) { - brooklynType = DeserializingClassRenamesProvider.INSTANCE.findMappedName(brooklynType); - return CatalogUtils.getCatalogItemOptionalVersion(mgmt, Entity.class, brooklynType); - } -} diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolver.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolver.java deleted file mode 100644 index 14f855a605..0000000000 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolver.java +++ /dev/null @@ -1,77 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.brooklyn.camp.brooklyn.spi.creation.service; - -import java.util.ServiceLoader; - -import org.apache.brooklyn.api.catalog.CatalogItem; -import org.apache.brooklyn.api.entity.Entity; -import org.apache.brooklyn.api.entity.EntitySpec; -import org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynComponentTemplateResolver; -import org.apache.brooklyn.core.resolve.entity.EntitySpecResolver; - -/** - * Resolves and decorates {@link EntitySpec entity specifications} based on the {@code serviceType} in a template. - *

- * The {@link #getTypePrefix()} method returns a string that should match the beginning of the - * service type. The resolver implementation will use the rest of the service type information - * to create and decorate an approprate {@link EntitySpec entity}. - *

- * The resolvers are loaded using the {@link ServiceLoader} mechanism, allowing external libraries - * to add extra service type implementations that will be picked up at runtime. - * - * @see BrooklynServiceTypeResolver - * @see ChefServiceTypeResolver - * - * @deprecated since 0.9.0, {@link EntitySpecResolver} instead. - */ -@Deprecated -public interface ServiceTypeResolver { - - String DEFAULT_TYPE_PREFIX = "brooklyn"; - - /** - * The service type prefix the resolver is responsible for. - */ - String getTypePrefix(); - - /** - * The name of the Java type that Brooklyn will instantiate to create the - * service. This can be generated from parts of the service type information - * or may be a fixed value. - */ - String getBrooklynType(String serviceType); - - /** - * Returns the {@link CatalogItem} if there is one for the given type. - *

- * If no type, callers should fall back to default classloading. - */ - CatalogItem> getCatalogItem(BrooklynComponentTemplateResolver resolver, String serviceType); - - /** - * Takes the provided {@link EntitySpec} and decorates it appropriately for the service type. - *

- * This includes setting configuration and adding policies, enrichers and initializers. - * - * @see BrooklynServiceTypeResolver#decorateSpec(BrooklynComponentTemplateResolver, EntitySpec) - */ - void decorateSpec(BrooklynComponentTemplateResolver resolver, EntitySpec spec); - -} diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolverAdaptor.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolverAdaptor.java deleted file mode 100644 index d4cf6e955c..0000000000 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolverAdaptor.java +++ /dev/null @@ -1,70 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.brooklyn.camp.brooklyn.spi.creation.service; - -import java.util.Set; - -import org.apache.brooklyn.api.entity.Entity; -import org.apache.brooklyn.api.entity.EntitySpec; -import org.apache.brooklyn.api.mgmt.classloading.BrooklynClassLoadingContext; -import org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynComponentTemplateResolver; -import org.apache.brooklyn.core.resolve.entity.AbstractEntitySpecResolver; -import org.apache.brooklyn.core.resolve.entity.EntitySpecResolver; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import com.google.common.base.Splitter; - -@SuppressWarnings("deprecation") -public class ServiceTypeResolverAdaptor extends AbstractEntitySpecResolver { - private static final Logger log = LoggerFactory.getLogger(ServiceTypeResolverAdaptor.class); - private ServiceTypeResolver serviceTypeResolver; - private BrooklynComponentTemplateResolver resolver; - - public ServiceTypeResolverAdaptor(BrooklynComponentTemplateResolver resolver, ServiceTypeResolver serviceTypeResolver) { - super(serviceTypeResolver.getTypePrefix()); - this.serviceTypeResolver = serviceTypeResolver; - this.resolver = resolver; - } - - @Override - public boolean accepts(String type, BrooklynClassLoadingContext loader) { - if (type.indexOf(':') != -1) { - String prefix = Splitter.on(":").splitToList(type).get(0); - return prefix.equals(serviceTypeResolver.getTypePrefix()); - } else { - return false; - } - } - - @SuppressWarnings({ "unchecked", "rawtypes" }) - @Override - public EntitySpec resolve(String type, BrooklynClassLoadingContext loader, Set encounteredTypes) { - // Assume this is interface! Only known implementation is DockerServiceTypeResolver. - String brooklynType = serviceTypeResolver.getBrooklynType(type); - Class javaType = loader.loadClass(brooklynType, Entity.class); - if (!javaType.isInterface()) { - log.warn("Using " + ServiceTypeResolver.class.getSimpleName() + " with a non-interface type - this usage is not supported. Use " + EntitySpecResolver.class.getSimpleName() + " instead."); - } - EntitySpec spec = EntitySpec.create((Class)javaType); - serviceTypeResolver.decorateSpec(resolver, spec); - return spec; - } - -} diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolverTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolverTest.java deleted file mode 100644 index cbaccf050e..0000000000 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/ServiceTypeResolverTest.java +++ /dev/null @@ -1,39 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.brooklyn.camp.brooklyn.spi.creation.service; - -import static org.testng.Assert.assertEquals; - -import org.apache.brooklyn.api.entity.EntitySpec; -import org.apache.brooklyn.camp.brooklyn.AbstractYamlTest; -import org.apache.brooklyn.entity.stock.BasicEntity; -import org.testng.annotations.Test; - - -public class ServiceTypeResolverTest extends AbstractYamlTest { - - @Test - public void testAddCatalogItemVerySimple() throws Exception { - EntitySpec spec = createAppEntitySpec( - "services:", - "- type: \"test-resolver:" + BasicEntity.class.getName() + "\""); - assertEquals(spec.getChildren().get(0).getFlags().get("resolver"), TestServiceTypeResolver.class); - } - -} diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/TestServiceTypeResolver.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/TestServiceTypeResolver.java deleted file mode 100644 index 1c13211c47..0000000000 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/TestServiceTypeResolver.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.brooklyn.camp.brooklyn.spi.creation.service; - -import org.apache.brooklyn.api.catalog.CatalogItem; -import org.apache.brooklyn.api.entity.Entity; -import org.apache.brooklyn.api.entity.EntitySpec; -import org.apache.brooklyn.camp.brooklyn.spi.creation.BrooklynComponentTemplateResolver; -import org.apache.brooklyn.util.text.Strings; - -@SuppressWarnings("deprecation") -public class TestServiceTypeResolver implements ServiceTypeResolver { - - private static final String PREFIX = "test-resolver"; - - @Override - public String getTypePrefix() { - return PREFIX; - } - - @Override - public String getBrooklynType(String serviceType) { - return Strings.removeFromStart(serviceType, PREFIX + ":"); - } - - @SuppressWarnings("unchecked") - @Override - public CatalogItem> getCatalogItem(BrooklynComponentTemplateResolver resolver, String serviceType) { - return (CatalogItem>) - resolver.getManagementContext().getCatalog().getCatalogItem(getBrooklynType(serviceType), null); - } - - @Override - public void decorateSpec(BrooklynComponentTemplateResolver resolver, EntitySpec spec) { - spec.configure("resolver", TestServiceTypeResolver.class); - } - -} From db77f4e14d07fe9e23fb464fb23302afc1fba2f0 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Tue, 20 Jun 2017 09:37:29 +0100 Subject: [PATCH 06/12] fix tests that relied on removed deprecated behaviour --- .../util/BrooklynRestResourceUtilsTest.java | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/BrooklynRestResourceUtilsTest.java b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/BrooklynRestResourceUtilsTest.java index 48908e39bc..9aa4803c23 100644 --- a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/BrooklynRestResourceUtilsTest.java +++ b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/BrooklynRestResourceUtilsTest.java @@ -41,6 +41,7 @@ import org.apache.brooklyn.entity.stock.BasicEntity; import org.apache.brooklyn.rest.domain.ApplicationSpec; import org.apache.brooklyn.rest.domain.EntitySpec; +import org.testng.Assert; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -83,17 +84,17 @@ public void testCreateAppFromImplClass() { @Test public void testCreateAppFromCatalogByType() { - createAppFromCatalog(SampleNoOpApplication.class.getName()); + createAppFromCatalog(SampleNoOpApplication.class.getName(), false); } @Test public void testCreateAppFromCatalogByName() { - createAppFromCatalog("app.noop"); + createAppFromCatalog("app.noop", true); } @Test public void testCreateAppFromCatalogById() { - createAppFromCatalog("app.noop:0.0.1"); + createAppFromCatalog("app.noop:0.0.1", true); } @Test @@ -103,11 +104,12 @@ public void testCreateAppFromCatalogByTypeMultipleItems() { .javaType(SampleNoOpApplication.class.getName()) .build(); managementContext.getCatalog().addItem(item); - createAppFromCatalog(SampleNoOpApplication.class.getName()); + createAppFromCatalog(SampleNoOpApplication.class.getName(), false); + createAppFromCatalog("app.noop", true); } @SuppressWarnings("deprecation") - private void createAppFromCatalog(String type) { + private void createAppFromCatalog(String type, boolean expectCatalogItemIdSet) { CatalogTemplateItemDto item = CatalogItemBuilder.newTemplate("app.noop", "0.0.1") .javaType(SampleNoOpApplication.class.getName()) .build(); @@ -120,22 +122,28 @@ private void createAppFromCatalog(String type) { .build(); Application app = util.create(spec); - assertEquals(app.getCatalogItemId(), "app.noop:0.0.1"); + if (expectCatalogItemIdSet) { + assertEquals(app.getCatalogItemId(), "app.noop:0.0.1"); + } else { + // since 0.12.0 we no longer reverse-lookup java types to try to find a registered type + // (as per warnings in several previous versions) + Assert.assertNull(app.getCatalogItemId()); + } } @Test public void testEntityAppFromCatalogByType() { - createEntityFromCatalog(BasicEntity.class.getName()); + createEntityFromCatalog(BasicEntity.class.getName(), false); } @Test public void testEntityAppFromCatalogByName() { - createEntityFromCatalog("app.basic"); + createEntityFromCatalog("app.basic", true); } @Test public void testEntityAppFromCatalogById() { - createEntityFromCatalog("app.basic:0.0.1"); + createEntityFromCatalog("app.basic:0.0.1", true); } @Test @@ -145,11 +153,12 @@ public void testEntityAppFromCatalogByTypeMultipleItems() { .javaType(SampleNoOpApplication.class.getName()) .build(); managementContext.getCatalog().addItem(item); - createEntityFromCatalog(BasicEntity.class.getName()); + createEntityFromCatalog(BasicEntity.class.getName(), false); + createEntityFromCatalog("app.basic", true); } @SuppressWarnings("deprecation") - private void createEntityFromCatalog(String type) { + private void createEntityFromCatalog(String type, boolean expectCatalogItemIdSet) { String symbolicName = "app.basic"; String version = "0.0.1"; CatalogTemplateItemDto item = CatalogItemBuilder.newTemplate(symbolicName, version) @@ -165,7 +174,13 @@ private void createEntityFromCatalog(String type) { Application app = util.create(spec); Entity entity = Iterables.getOnlyElement(app.getChildren()); - assertEquals(entity.getCatalogItemId(), CatalogUtils.getVersionedId(symbolicName, version)); + if (expectCatalogItemIdSet) { + assertEquals(entity.getCatalogItemId(), CatalogUtils.getVersionedId(symbolicName, version)); + } else { + // since 0.12.0 we no longer reverse-lookup java types to try to find a registered type + // (as per warnings in several previous versions) + Assert.assertNull(entity.getCatalogItemId()); + } } @Test From 5b2cc48685ee606207257e74fd8a61f13f876a00 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Tue, 20 Jun 2017 16:48:25 +0100 Subject: [PATCH 07/12] prefer versions which comply with recommendations, heuristics elsewhere expand tests. some obscure ordering items are different now. --- .../util/text/NaturalOrderComparator.java | 39 +++++++- .../brooklyn/util/text/VersionComparator.java | 92 +++++++++++++++---- .../util/text/NaturalOrderComparatorTest.java | 63 +++++++++---- .../util/text/VersionComparatorTest.java | 44 ++++++--- 4 files changed, 185 insertions(+), 53 deletions(-) diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/text/NaturalOrderComparator.java b/utils/common/src/main/java/org/apache/brooklyn/util/text/NaturalOrderComparator.java index 7cdf42181e..82f4e86ced 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/text/NaturalOrderComparator.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/text/NaturalOrderComparator.java @@ -53,17 +53,24 @@ /** comparator which takes two strings and puts them in an order * with special rules for numbers (whole number) to be placed in numeric order; * e.g. "10">"9", including when those numbers occur in the midst of equal text; e.g. "a10" > "a9"; - * but not if the text differs; e.g. "a10" < "b9" + * but not if the text differs; e.g. "a10" < "b9". + * leading zeroes are slightly lower than the same number without the leading zero, so "8" < "09" < "9", + * but after considering the remainder of the string, so "09.b" > "9.a". + * decimals are treated like a word-split, not a decimal point, i.e. "1.9" < "1.10". *

- * class is thread-safe. nulls not supported. (to support nulls, wrap in guava: + * class is thread-safe. + *

+ * nulls not supported. to support nulls, wrap in guava: * Ordering.from(NaturalOrderComparator.INSTANCE).nullsFirst()) *

- * NOTE: decimals are treated like a word-split, not a decimal point, i.e. 1.9 < 1.10 + *

+ * Based on https://github.com/paour/natorder */ public class NaturalOrderComparator implements Comparator { public static final NaturalOrderComparator INSTANCE = new NaturalOrderComparator(); + /** compares a string of digits once 0's have been skipped, stopping when the digits finish */ int compareRight(String a, String b) { int bias = 0; @@ -140,6 +147,32 @@ public int compare(String a, String b) { if ((result = compareRight(a.substring(ia), b.substring(ib))) != 0) { return result; } + // numeric portion is the same; previously we incremented and checked again + // which is inefficient (we keep checking remaining numbers here); + // also if we want to compare on 0's after comparing remainder of string + // we need to recurse here after skipping the digits we just checked above + do { + ia++; ib++; + boolean aDigitsDone = ia >= a.length() || !Character.isDigit(charAt(a, ia)); + boolean bDigitsDone = ib >= b.length() || !Character.isDigit(charAt(b, ib)); + if (aDigitsDone != bDigitsDone) { + // sanity check + throw new IllegalStateException("Digit sequence lengths should have matched, '"+a+"' v '"+b+"'"); + } + if (aDigitsDone) break; + } while (true); + if (ia < a.length() || ib < b.length()) { + if (ia >= a.length()) return -1; // only b has more chars + if (ib >= b.length()) return 1; // only a has more chars + // both have remaining chars; neither is numeric due to compareRight; recurse into remaining + if ((result = compare(a.substring(ia), b.substring(ib))) != 0) { + return result; + } + } + // numbers are equal value, remaining string compares identical; + // comes down to whether there are any leading zeroes here + return nzb-nza; + } else if ((Character.isDigit(ca) || nza>0) && (Character.isDigit(cb) || nzb>0)) { // both sides are numbers, but at least one is a sequence of zeros if (nza==0) { diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java b/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java index 071f591363..bac19293bd 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java @@ -22,23 +22,23 @@ import java.util.Arrays; import java.util.Collection; import java.util.Comparator; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.apache.brooklyn.util.text.NaturalOrderComparator; import org.apache.brooklyn.util.text.Strings; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Objects; /** * {@link Comparator} for version strings. *

- * SNAPSHOT items always lowest rated, - * then splitting on dots, - * using natural order comparator (so "9" < "10" and "4u8" < "4u20"), - * and preferring segments without qualifiers ("4" > "4beta"). + * SNAPSHOT items always lowest rated, then looking for no qualifier, then doing natural order comparison. + * This gives the desired semantics for our recommended version syntax. *

* Impossible to follow semantics for all versioning schemes but - * does the obvious right thing for normal schemes - * and pretty well in fringe cases. + * does the obvious right thing for normal schemes and pretty well in fringe cases. *

* See test case for lots of examples. */ @@ -56,24 +56,80 @@ public static boolean isSnapshot(String version) { if (version==null) return false; return version.toUpperCase().contains(SNAPSHOT); } + + @SuppressWarnings("unused") + private static class TwoBooleans { + private final boolean b1, b2; + public TwoBooleans(boolean b1, boolean b2) { this.b1 = b1; this.b2 = b2; } + boolean bothTrue() { return b1 && b2; } + boolean eitherTrue() { return b1 || b2; } + boolean bothFalse() { return !eitherTrue(); } + boolean same() { return b1==b2; } + boolean different() { return b1!=b2; } + int compare(boolean trueIsLess) { return same() ? 0 : b1==trueIsLess ? -1 : 1; } + public static TwoBooleans of(boolean v1, boolean v2) { + return new TwoBooleans(v1, v2); + } + } @Override public int compare(String v1, String v2) { - if (v1==null && v2==null) return 0; - if (v1==null) return -1; - if (v2==null) return 1; + if (Objects.equal(v1, v2)) return 0; - boolean isV1Snapshot = isSnapshot(v1); - boolean isV2Snapshot = isSnapshot(v2); - if (isV1Snapshot == isV2Snapshot) { - // if snapshot status is the same, look at dot-split parts first - return compareDotSplitParts(splitOnDot(v1), splitOnDot(v2)); - } else { - // snapshot goes first - return isV1Snapshot ? -1 : 1; - } + TwoBooleans nulls = TwoBooleans.of(v1==null, v2==null); + if (nulls.eitherTrue()) return nulls.compare(true); + + TwoBooleans snapshots = TwoBooleans.of(isSnapshot(v1), isSnapshot(v2)); + if (snapshots.different()) return snapshots.compare(true); + + String u1 = versionWithQualifier(v1); + String u2 = versionWithQualifier(v2); + int uq = NaturalOrderComparator.INSTANCE.compare(u1, u2); + if (uq!=0) return uq; + + // qualified items are lower than unqualified items + TwoBooleans no_qualifier = TwoBooleans.of(u1.equals(v1), u2.equals(v2)); + if (no_qualifier.different()) return no_qualifier.compare(false); + + // now compare qualifiers + + // allow -, ., and _ as qualifier offsets; if used, compare qualifer without that + String q1 = v1.substring(u1.length()); + String q2 = v2.substring(u2.length()); + + String qq1 = q1, qq2 = q2; + if (q1.startsWith("-") || q1.startsWith(".") || q1.startsWith("_")) q1 = q1.substring(1); + if (q2.startsWith("-") || q2.startsWith(".") || q2.startsWith("_")) q2 = q2.substring(1); + uq = NaturalOrderComparator.INSTANCE.compare(q1, q2); + if (uq!=0) return uq; + + // if qualifiers the same, look at qualifier separator char + int sep1 = qq1.startsWith("-") ? 3 : qq1.startsWith(".") ? 2 : qq1.startsWith("_") ? 1 : 0; + int sep2 = qq2.startsWith("-") ? 3 : qq2.startsWith(".") ? 2 : qq2.startsWith("_") ? 1 : 0; + uq = Integer.compare(sep1, sep2); + if (uq!=0) return uq; + + // finally, do normal comparison if both have qualifier or both unqualified (in case leading 0's are used) + return NaturalOrderComparator.INSTANCE.compare(v1, v2); + + // (previously we did this but don't think we need any of that complexity) + //return compareDotSplitParts(splitOnDot(v1), splitOnDot(v2)); + } + + private String versionWithQualifier(String v1) { + Matcher q = Pattern.compile("([0-9]+(\\.[0-9]+(\\.[0-9])?)?)(.*)").matcher(v1); + return q.matches() ? q.group(1) : ""; } + private boolean isQualifiedAndUnqualifiedVariantOfSameVersion(String v1, String v2) { + Matcher q = Pattern.compile("([0-9]+(\\.[0-9]+(\\.[0-9])?)?)(.*)").matcher(v1); + return (q.matches() && q.group(1).equals(v2)); + } + + private static boolean hasQualifier(String v) { + return !v.matches("[0-9]+(\\.[0-9]+(\\.[0-9])?)?"); + } + @VisibleForTesting static String[] splitOnDot(String v) { return v.split("(?<=\\.)|(?=\\.)"); diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/text/NaturalOrderComparatorTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/text/NaturalOrderComparatorTest.java index 3113c8e0e5..d38c30a36e 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/text/NaturalOrderComparatorTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/text/NaturalOrderComparatorTest.java @@ -18,8 +18,6 @@ */ package org.apache.brooklyn.util.text; -import org.apache.brooklyn.util.text.ComparableVersion; -import org.apache.brooklyn.util.text.NaturalOrderComparator; import org.testng.Assert; import org.testng.annotations.Test; @@ -27,22 +25,32 @@ public class NaturalOrderComparatorTest { public static final NaturalOrderComparator noc = new NaturalOrderComparator(); - ComparableVersion v = new ComparableVersion("10.5.8"); - ComparableVersion v_rc2 = new ComparableVersion("10.5.8-rc2"); - @Test public void testNoc() { - Assert.assertEquals(noc.compare("0a", "1"), -1); - Assert.assertEquals(noc.compare("0", "1"), -1); Assert.assertEquals(noc.compare("1", "10"), -1); Assert.assertEquals(noc.compare("9", "10"), -1); Assert.assertEquals(noc.compare("a", "b"), -1); Assert.assertEquals(noc.compare("a9", "a10"), -1); + Assert.assertEquals(noc.compare("1", "02"), -1); + Assert.assertEquals(noc.compare("02", "3"), -1); + Assert.assertEquals(noc.compare("02", "2"), -1); + + Assert.assertEquals(noc.compare("1.b", "02.a"), -1); + Assert.assertEquals(noc.compare("02.b", "3.a"), -1); + // leading zero considered _after_ remainder of string + Assert.assertEquals(noc.compare("02.a", "2.b"), -1); + Assert.assertEquals(noc.compare("0.9", "0.91"), -1); Assert.assertEquals(noc.compare("0.90", "0.91"), -1); Assert.assertEquals(noc.compare("1.2.x", "1.09.x"), -1); + Assert.assertEquals(noc.compare("2", "2.1"), -1); + Assert.assertEquals(noc.compare("2", "2.01"), -1); + Assert.assertEquals(noc.compare("2.01", "2.1"), -1); + Assert.assertEquals(noc.compare("2.1", "2.02"), -1); + Assert.assertEquals(noc.compare("2", "02.1"), -1); + Assert.assertEquals(noc.compare("2", "02.01"), -1); Assert.assertEquals(noc.compare("0", "1a"), -1); Assert.assertEquals(noc.compare("0a", "1"), -1); @@ -66,25 +74,40 @@ public void testBasicOnes() { @Test public void testVersionNumbers() { - Assert.assertEquals(0, noc.compare("10.5.8", "10.5.8")); + Assert.assertEquals(noc.compare("10.5.8", "10.5.8"), 0); Assert.assertTrue(noc.compare("10.5", "9.9") > 0); Assert.assertTrue(noc.compare("10.5.1", "10.5") > 0); Assert.assertTrue(noc.compare("10.5.1", "10.6") < 0); Assert.assertTrue(noc.compare("10.5.1-1", "10.5.1-0") > 0); } - @Test(groups="WIP", enabled=false) - public void testUnderscoreDoesNotChangeMeaningOfNumberInNoc() { - // why?? - Assert.assertTrue(noc.compare("0.0.0_SNAPSHOT", "0.0.1-SNAPSHOT-20141111114709760") < 0); - - Assert.assertTrue(v.isGreaterThanAndNotEqualTo(v_rc2.version)); - Assert.assertTrue(v_rc2.isLessThanAndNotEqualTo(v.version)); - } - - @Test(groups="WIP", enabled=false) - public void testUnderscoreDoesNotChangeMeaningOfNumberInOurWorld() { - Assert.assertTrue(new ComparableVersion("0.0.0_SNAPSHOT").isLessThanAndNotEqualTo("0.0.1-SNAPSHOT-20141111114709760")); + @Test + public void testWordsNumsPunctuation() { + // it's basically ascii except for where we're comparing numbers in the same place + + Assert.assertTrue(noc.compare("1", "") > 0); + Assert.assertTrue(noc.compare("one", "1") > 0); + Assert.assertTrue(noc.compare("some1", "some") > 0); + Assert.assertTrue(noc.compare("someone", "some") > 0); + Assert.assertTrue(noc.compare("someone", "some1") > 0); + Assert.assertTrue(noc.compare("someone", "some0") > 0); + Assert.assertTrue(noc.compare("some-one", "some-1") > 0); + + Assert.assertTrue(noc.compare("a1", "aa1") < 0); + Assert.assertTrue(noc.compare("a", "aa") < 0); + Assert.assertTrue(noc.compare("a", "a-") < 0); + Assert.assertTrue(noc.compare("a-", "a.") < 0); + Assert.assertTrue(noc.compare("a-", "a1") < 0); + Assert.assertTrue(noc.compare("a-", "aa") < 0); + Assert.assertTrue(noc.compare("a1", "aa") < 0); + Assert.assertTrue(noc.compare("aA", "a_") < 0); + Assert.assertTrue(noc.compare("a_", "aa") < 0); + Assert.assertTrue(noc.compare("a-1", "a1") < 0); + Assert.assertTrue(noc.compare("a0", "a-1") < 0); + Assert.assertTrue(noc.compare("a0", "aa0") < 0); + Assert.assertTrue(noc.compare("a-9", "a-10") < 0); + Assert.assertTrue(noc.compare("a-0", "a-01") < 0); + Assert.assertTrue(noc.compare("a-01", "a-1") < 0); } } \ No newline at end of file diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/text/VersionComparatorTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/text/VersionComparatorTest.java index 6c65a8a92e..082126739f 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/text/VersionComparatorTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/text/VersionComparatorTest.java @@ -56,40 +56,51 @@ public void testSnapshotSuffixComparison() { @Test public void testComparison() { + assertVersionOrder("1beta", "1"); + assertVersionOrder("0", "1"); assertVersionOrder("0", "0.0", "0.9", "0.10", "0.10.0", "1"); assertVersionOrder("a", "b"); assertVersionOrder("1beta", "1", "2beta", "11beta"); - assertVersionOrder("beta", "0", "1beta", "1-alpha", "1", "11beta", "11-alpha", "11"); + assertVersionOrder("beta", "0", "1alpha", "1-alpha", "1beta", "1", "11-alpha", "11beta", "11_beta", "11.beta", "11-beta", "11"); assertVersionOrder("1.0-a", "1.0-b", "1.0"); assertVersionOrder("qualifier", "0qualifier", "0-qualifier", "0", "1-qualifier", "1"); - assertVersionOrder("2.0.qualifier", "2.0", "2.0.0qualifier", "2.0.0-qualifier", "2.0.0.qualifier", "2.0.0"); - assertVersionOrder("2.0.qualifier.0", "2.0", "2.0.0qualifier.0", "2.0.0-qualifier.0", "2.0.0.qualifier.0", "2.0.0", "2.0.0.0"); + assertVersionOrder("2.0.qualifier", "2.0", "2.0.0qualifier", "2.0.0.qualifier", "2.0.0-qualifier", "2.0.0"); + assertVersionOrder("2.0-0", "2.0-2", "2.0-10", "2.0.qualifier.0", "2.0", + "2.0.0.0", "2.0.0-0", "2.0.0.1", "2.0.0-1", "2.0.0-q", "2.0.0qualifier.0", "2.0.0.qualifier.0", "2.0.0-qualifier.0", "2.0.0"); assertVersionOrder("0", "0.0", "0.1", "0.1.0", "0.1.1", "0.2", "0.2.1", "1", "1.0", "2"); + assertVersionOrder("0", "0.0", "0.1", "0.1.0", "0.1.1", "0.2", "0.2.1", "1", "1.0", "02", "2", "02.01", "2.01", "02.1", "2.1", "02.02", "02.2", "2.2"); // case sensitive assertVersionOrder("AA", "Aa", "aa"); - // letters in order, ignoring case, and using natural order on numbers, splitting at word boundaries - assertVersionOrder("A", "B-2", "B-10", "B", "B0", "C", "b", "b1", "b9", "b10", "c", "0"); - // and non-letter symbols are compared, in alpha order (e.g. - less than _) with dots even higher - assertVersionOrder("0-qual", "0", "0.1", "1-qualC", "1_qualB", "1.qualA", "1", "1.0"); + // non-version-strings sorted as per NaturalOrderComparator, all < 0 + assertVersionOrder("A", "B", "B0", "B-2", "B-10", "C", "b", "b-9", "b1", "b9", "b10", "ba1", "c", "0"); + // and non-letter symbols are compared as follows + assertVersionOrder("0-qual", "0", "0.1", + // _after_ qualifier (after leading zeroes) if it's the qualifier separator + "01.qualA", "01-qualC", "01", + "1_qualA", "1.qualA", "1-qualA", "1.qualB", "1_qualC", "1-qualC", + // in ascii order (-, ., _) otherwise (always after leading zeroes) + "1-x-qualC", "1-x.qualB", "1-x_qualA", + // with unqualified always preferred + "1"); // numeric comparison works with qualifiers, preferring unqualified - assertVersionOrder("0--qual", "0-qual", "0-qualB", "0-qualB2", "0-qualB10", "0-qualC", "0.qualA", "0", "0.1.qual", "0.1", "1"); + assertVersionOrder("0--qual", "0-qual", "0.qualA", "0-qualA", "0-qualB", "0-qualB2", "0-qualB10", "0-qualC", "0", "0.1.qual", "0.1", "1"); // all snapshots rated lower assertVersionOrder( - "0_SNAPSHOT", "0.1.SNAPSHOT", "1-SNAPSHOT-X-X", "1-SNAPSHOT-X", "1-SNAPSHOT-XX-X", "1-SNAPSHOT-XX", "1-SNAPSHOT", - "1.0-SNAPSHOT-B", "1.0.SNAPSHOT-A", - "1.2-SNAPSHOT", "1.10-SNAPSHOT", + "0_SNAPSHOT", "0.1.SNAPSHOT", "1-SNAPSHOT", "1-SNAPSHOT-X", "1-SNAPSHOT-X-X", "1-SNAPSHOT-XX", "1-SNAPSHOT-XX-X", + "1.0.SNAPSHOT-A", "1.0-SNAPSHOT-B", + "1.2-SNAPSHOT", "1.10.SNAPSHOT", "qualifer", "0", "0.1", "1"); - assertVersionOrder("0.10.0-SNAPSHOT", "0.10.0.SNAPSHOT", "0.10.0-GA", "0.10.0.GA", "0.10.0"); + assertVersionOrder("0.10.0.SNAPSHOT", "0.10.0-SNAPSHOT", "0.10.0.GA", "0.10.0-GA", "0.10.0"); } @Test @@ -111,4 +122,13 @@ private static void assertVersionOrder(String v1, String v2, String ...otherVers } } + @Test + public void testComparableVersion() { + ComparableVersion v = new ComparableVersion("10.5.8"); + ComparableVersion v_rc2 = new ComparableVersion("10.5.8-rc2"); + + Assert.assertTrue(v.isGreaterThanAndNotEqualTo(v_rc2.version)); + Assert.assertTrue(v_rc2.isLessThanAndNotEqualTo(v.version)); + } + } From cc703928e1a2bd9c97795acab9a747a3733eb4a2 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Tue, 20 Jun 2017 16:49:57 +0100 Subject: [PATCH 08/12] tidy version comparator, removing old code --- .../brooklyn/util/text/VersionComparator.java | 143 ------------------ .../util/text/VersionComparatorTest.java | 21 --- 2 files changed, 164 deletions(-) diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java b/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java index bac19293bd..e5d1fbfaf8 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java @@ -18,17 +18,10 @@ */ package org.apache.brooklyn.util.text; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; import java.util.Comparator; import java.util.regex.Matcher; import java.util.regex.Pattern; -import org.apache.brooklyn.util.text.NaturalOrderComparator; -import org.apache.brooklyn.util.text.Strings; - -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Objects; /** @@ -121,140 +114,4 @@ private String versionWithQualifier(String v1) { return q.matches() ? q.group(1) : ""; } - private boolean isQualifiedAndUnqualifiedVariantOfSameVersion(String v1, String v2) { - Matcher q = Pattern.compile("([0-9]+(\\.[0-9]+(\\.[0-9])?)?)(.*)").matcher(v1); - return (q.matches() && q.group(1).equals(v2)); - } - - private static boolean hasQualifier(String v) { - return !v.matches("[0-9]+(\\.[0-9]+(\\.[0-9])?)?"); - } - - @VisibleForTesting - static String[] splitOnDot(String v) { - return v.split("(?<=\\.)|(?=\\.)"); - } - - private int compareDotSplitParts(String[] v1Parts, String[] v2Parts) { - for (int i = 0; ; i++) { - if (i >= v1Parts.length && i >= v2Parts.length) { - // end of both - return 0; - } - if (i == v1Parts.length) { - // sequence depends whether the extra part *starts with* a number - // ie - // 2.0 < 2.0.0 - // and - // 2.0.qualifier < 2.0 < 2.0.0qualifier < 2.0.0-qualifier < 2.0.0.qualifier < 2.0.0 < 2.0.9-qualifier - return isNumberInFirstCharPossiblyAfterADot(v2Parts, i) ? -1 : 1; - } - if (i == v2Parts.length) { - // as above but inverted - return isNumberInFirstCharPossiblyAfterADot(v1Parts, i) ? 1 : -1; - } - // not at end; compare this dot split part - - int result = compareDotSplitPart(v1Parts[i], v2Parts[i]); - if (result!=0) return result; - } - } - - private int compareDotSplitPart(String v1, String v2) { - String[] v1Parts = splitOnNonWordChar(v1); - String[] v2Parts = splitOnNonWordChar(v2); - - for (int i = 0; ; i++) { - if (i >= v1Parts.length && i >= v2Parts.length) { - // end of both - return 0; - } - if (i == v1Parts.length) { - // shorter set always wins here; i.e. - // 1-qualifier < 1 - return 1; - } - if (i == v2Parts.length) { - // as above but inverted - return -1; - } - // not at end; compare this dot split part - - String v1p = v1Parts[i]; - String v2p = v2Parts[i]; - - if (v1p.equals(v2p)) continue; - - if (isNumberInFirstChar(v1p) || isNumberInFirstChar(v2p)) { - // something starting with a number is higher than something not - if (!isNumberInFirstChar(v1p)) return -1; - if (!isNumberInFirstChar(v2p)) return 1; - - // both start with numbers; can use natural order comparison *unless* - // one is purely a number AND the other *begins* with that number, - // followed by non-digit chars, in which case prefer the pure number - // ie: - // 1beta < 1 - // but note - // 1 < 2beta < 11beta - if (isNumber(v1p) || isNumber(v2p)) { - if (!isNumber(v1p)) { - if (v1p.startsWith(v2p)) { - if (!isNumberInFirstChar(Strings.removeFromStart(v1p, v2p))) { - // v2 is a number, and v1 is the same followed by non-numbers - return -1; - } - } - } - if (!isNumber(v2p)) { - // as above but inverted - if (v2p.startsWith(v1p)) { - if (!isNumberInFirstChar(Strings.removeFromStart(v2p, v1p))) { - return 1; - } - } - } - // both numbers, skip to natural order comparison - } - } - - // otherwise it is in-order - int result = NaturalOrderComparator.INSTANCE.compare(v1p, v2p); - if (result!=0) return result; - } - } - - @VisibleForTesting - static String[] splitOnNonWordChar(String v) { - Collection parts = new ArrayList(); - String remaining = v; - - // use lookahead to split on all non-letter non-numbers, putting them into their own buckets - parts.addAll(Arrays.asList(remaining.split("(?<=[^0-9\\p{L}])|(?=[^0-9\\p{L}])"))); - return parts.toArray(new String[parts.size()]); - } - - @VisibleForTesting - static boolean isNumberInFirstCharPossiblyAfterADot(String[] parts, int i) { - if (parts==null || parts.length<=i) return false; - if (isNumberInFirstChar(parts[i])) return true; - if (".".equals(parts[i])) { - if (parts.length>i+1) - if (isNumberInFirstChar(parts[i+1])) - return true; - } - return false; - } - - @VisibleForTesting - static boolean isNumberInFirstChar(String v) { - if (v==null || v.length()==0) return false; - return Character.isDigit(v.charAt(0)); - } - - @VisibleForTesting - static boolean isNumber(String v) { - if (v==null || v.length()==0) return false; - return v.matches("[\\d]+"); - } } diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/text/VersionComparatorTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/text/VersionComparatorTest.java index 082126739f..aa01c79918 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/text/VersionComparatorTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/text/VersionComparatorTest.java @@ -27,27 +27,6 @@ import org.testng.annotations.Test; public class VersionComparatorTest { - - @Test - public void testStaticHelpers() { - Assert.assertEquals(VersionComparator.splitOnDot("a.b.cc"), new String[] { "a", ".", "b", ".", "cc" }); - Assert.assertEquals(VersionComparator.splitOnDot("a..b-c"), new String[] { "a", ".", ".", "b-c" }); - - Assert.assertEquals(VersionComparator.splitOnNonWordChar("a1-b__cc9c"), new String[] { - "a1", "-", "b", "_", "_", "cc9c" }); - - Assert.assertEquals(VersionComparator.isNumberInFirstChar("1a"), true); - Assert.assertEquals(VersionComparator.isNumberInFirstChar("a1"), false); - Assert.assertEquals(VersionComparator.isNumberInFirstChar(""), false); - Assert.assertEquals(VersionComparator.isNumberInFirstChar(null), false); - - Assert.assertEquals(VersionComparator.isNumber("1"), true); - Assert.assertEquals(VersionComparator.isNumber("1111"), true); - Assert.assertEquals(VersionComparator.isNumber("1a"), false); - Assert.assertEquals(VersionComparator.isNumber("a1"), false); - Assert.assertEquals(VersionComparator.isNumber(""), false); - Assert.assertEquals(VersionComparator.isNumber(null), false); - } @Test public void testSnapshotSuffixComparison() { From 45cb10a57af9e4aba0e7d624f1af6bfb25e5cee2 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Wed, 21 Jun 2017 17:04:04 +0100 Subject: [PATCH 09/12] add lots of text explaining BrooklynVersionSyntax, update comparator, tests, and usages --- .../internal/BasicBrooklynCatalog.java | 16 ++-- .../internal/CatalogItemDtoAbstract.java | 4 +- .../core/typereg/RegisteredTypeNaming.java | 52 +++++------ .../internal/CatalogItemComparatorTest.java | 9 +- .../internal/CatalogVersioningTest.java | 1 + .../typereg/RegisteredTypeNamingTest.java | 59 +++++++------ .../util/text/BrooklynVersionSyntax.java | 87 +++++++++++++++++++ .../brooklyn/util/text/VersionComparator.java | 11 ++- .../util/text/ComparableVersionTest.java | 5 +- 9 files changed, 180 insertions(+), 64 deletions(-) create mode 100644 utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java index e4c98824dd..006d3e84ca 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java @@ -602,11 +602,14 @@ private void collectCatalogItems(String sourceYaml, Map itemMetadata, List< // if symname not set, infer from: id, then name, then item id, then item name if (Strings.isBlank(symbolicName)) { if (Strings.isNonBlank(id)) { - if (RegisteredTypeNaming.isGoodTypeColonVersion(id)) { + if (RegisteredTypeNaming.isGoodBrooklynTypeColonVersion(id)) { symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(id); + } else if (RegisteredTypeNaming.isValidOsgiTypeColonVersion(id)) { + symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(id); + log.warn("Discouraged version syntax in id '"+id+"'; version should comply with brooklyn recommendation (#.#.#-qualifier or portion) or specify symbolic name and version explicitly, not OSGi version syntax"); } else if (CatalogUtils.looksLikeVersionedId(id)) { // use of above method is deprecated in 0.12; this block can be removed in 0.13 - log.warn("Discouraged version syntax in id '"+id+"'; version should comply with OSGi specs (#.#.#.qualifier or portion) or specify symbolic name and version explicitly"); + log.warn("Discouraged version syntax in id '"+id+"'; version should comply with brooklyn recommendation (#.#.#-qualifier or portion) or specify symbolic name and version explicitly"); symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(id); } else if (RegisteredTypeNaming.isUsableTypeColonVersion(id)) { log.warn("Deprecated type naming syntax in id '"+id+"'; colons not allowed in type name as it is used to indicate version"); @@ -618,7 +621,7 @@ private void collectCatalogItems(String sourceYaml, Map itemMetadata, List< symbolicName = id; } } else if (Strings.isNonBlank(name)) { - if (RegisteredTypeNaming.isGoodTypeColonVersion(name)) { + if (RegisteredTypeNaming.isGoodBrooklynTypeColonVersion(name) || RegisteredTypeNaming.isValidOsgiTypeColonVersion(name)) { log.warn("Deprecated use of 'name' key to define '"+name+"'; version should be specified within 'id' key or with 'version' key, not this tag"); // deprecated in 0.12; remove in 0.13 symbolicName = CatalogUtils.getSymbolicNameFromVersionedId(name); @@ -646,10 +649,13 @@ private void collectCatalogItems(String sourceYaml, Map itemMetadata, List< } String versionFromId = null; - if (RegisteredTypeNaming.isGoodTypeColonVersion(id)) { + if (RegisteredTypeNaming.isGoodBrooklynTypeColonVersion(id)) { + versionFromId = CatalogUtils.getVersionFromVersionedId(id); + } else if (RegisteredTypeNaming.isValidOsgiTypeColonVersion(id)) { versionFromId = CatalogUtils.getVersionFromVersionedId(id); + log.warn("Discouraged version syntax in id '"+id+"'; version should comply with Brooklyn recommended version syntax (#.#.#-qualifier or portion) or specify symbolic name and version explicitly, not OSGi"); } else if (CatalogUtils.looksLikeVersionedId(id)) { - log.warn("Discouraged version syntax in id '"+id+"'; version should comply with OSGi specs (#.#.#.qualifier or portion) or specify symbolic name and version explicitly"); + log.warn("Discouraged version syntax in id '"+id+"'; version should comply with Brooklyn recommended version syntax (#.#.#-qualifier or portion) or specify symbolic name and version explicitly"); // remove in 0.13 versionFromId = CatalogUtils.getVersionFromVersionedId(id); } else if (RegisteredTypeNaming.isUsableTypeColonVersion(id)) { diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java index 3f69e677ad..d098cddf25 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java @@ -420,7 +420,7 @@ public static Collection parseLibraries(Collection possibleLib name = null; version = null; url = inlineRef; - } else if (RegisteredTypeNaming.isGoodTypeColonVersion(inlineRef)) { + } else if (RegisteredTypeNaming.isGoodBrooklynTypeColonVersion(inlineRef) || RegisteredTypeNaming.isValidOsgiTypeColonVersion(inlineRef)) { //looks like a name+version ref name = CatalogUtils.getSymbolicNameFromVersionedId(inlineRef); version = CatalogUtils.getVersionFromVersionedId(inlineRef); @@ -444,6 +444,8 @@ public static Collection parseLibraries(Collection possibleLib url = inlineRef; } + // TODO +// version = RegisteredTypeNaming.toOsgiVersion(version, "library reference "+name+":"+version); dto.add(new CatalogBundleDto(name, version, url)); } else { LOG.debug("Unexpected entry in libraries list neither string nor map: " + object); diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java b/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java index 22e9e7885a..61b999e0ad 100644 --- a/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java +++ b/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java @@ -18,6 +18,8 @@ */ package org.apache.brooklyn.core.typereg; +import org.apache.brooklyn.util.text.BrooklynVersionSyntax; + /** * Methods for testing validity of names and decomposing them. * Mostly based on OSGi, specifically sections 1.3.2 and 3.2.5 of @@ -31,16 +33,6 @@ public class RegisteredTypeNaming { public final static String OSGI_TOKEN_CHARS = "A-Za-z0-9_-"; public final static String OSGI_TOKEN_REGEX = "[" + OSGI_TOKEN_CHARS + "]+"; public final static String OSGI_SYMBOLIC_NAME_REGEX = OSGI_TOKEN_REGEX + "(" + DOT + OSGI_TOKEN_REGEX + ")*"; - public final static String NUMBER = "[0-9]+"; - public final static String QUALIFIER = OSGI_TOKEN_REGEX; - public final static String VERSION_REGEX = - NUMBER + - "(" + "\\." + NUMBER + - "(" + "\\." + NUMBER + - "(" + "\\." + QUALIFIER + - ")?" + - ")?" + - ")?"; private static boolean isUsable(String candidate) { return candidate!=null && candidate.matches(USABLE_REGEX); @@ -71,22 +63,19 @@ public static boolean isGoodTypeName(String candidate) { return isUsable(candidate) && candidate.matches(OSGI_SYMBOLIC_NAME_REGEX); } - /** - * For versions we currently work with any non-empty string that does not contain a ':' or whitespace. - * However we discourage things that are not OSGi versions; see {@link #isOsgiLegalVersion(String)}. - * In some places (eg bundles) the use of OSGi version syntax may be enforced. - */ + /** @see BrooklynVersionSyntax#isUsableVersion(String) */ public static boolean isUsableVersion(String candidate) { - return isUsable(candidate); + return BrooklynVersionSyntax.isUsableVersion(candidate); } - /** True if the argument matches the OSGi version spec, of the form - * MAJOR.MINOR.POINT.QUALIFIER or part thereof (not ending in a period though), - * where the first three are whole numbers and the final piece is any valid OSGi token - * (something satisfying {@link #isGoodTypeName(String)} with no periods). - */ - public static boolean isOsgiLegalVersion(String candidate) { - return candidate!=null && candidate.matches(VERSION_REGEX); + /** @see BrooklynVersionSyntax#isGoodBrooklynVersion(String) */ + public static boolean isGoodBrooklynVersion(String candidate) { + return BrooklynVersionSyntax.isGoodBrooklynVersion(candidate); + } + + /** @see BrooklynVersionSyntax#isValidOsgiVersion(String) */ + public static boolean isValidOsgiVersion(String candidate) { + return BrooklynVersionSyntax.isValidOsgiVersion(candidate); } /** True if the argument has exactly one colon, and the part before @@ -98,14 +87,25 @@ public static boolean isUsableTypeColonVersion(String candidate) { /** True if the argument has exactly one colon, and the part before * satisfies {@link #isGoodTypeName(String)} and the part after - * {@link #isOsgiLegalVersion(String)}. */ - public static boolean isGoodTypeColonVersion(String candidate) { + * {@link #isGoodBrooklynVersion(String)}. */ + public static boolean isGoodBrooklynTypeColonVersion(String candidate) { if (candidate==null) return false; int idx = candidate.indexOf(':'); if (idx<=0) return false; if (!isGoodTypeName(candidate.substring(0, idx))) return false; - if (!isOsgiLegalVersion(candidate.substring(idx+1))) return false; + if (!isGoodBrooklynVersion(candidate.substring(idx+1))) return false; return true; } + /** True if the argument has exactly one colon, and the part before + * satisfies {@link #isGoodTypeName(String)} and the part after + * {@link #isValidOsgiVersion(String)}. */ + public static boolean isValidOsgiTypeColonVersion(String candidate) { + if (candidate==null) return false; + int idx = candidate.indexOf(':'); + if (idx<=0) return false; + if (!isGoodTypeName(candidate.substring(0, idx))) return false; + if (!isValidOsgiVersion(candidate.substring(idx+1))) return false; + return true; + } } diff --git a/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogItemComparatorTest.java b/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogItemComparatorTest.java index 3c8ce8395e..9f2aa2c432 100644 --- a/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogItemComparatorTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogItemComparatorTest.java @@ -62,12 +62,15 @@ public void testComparison() { compare(STABLE, STABLE, 0); compare(STABLE, "10.6", 1); - compare(STABLE, "10.5.8.1", 1); - compare("10.5.8-rc2", "10.5.8-rc3", 1) ; - compare("10.5.8-rc2", "10.5.8-rc1", -1); + compare(RC2, "10.5.8-rc3", 1) ; + compare(RC2, "10.5.8-rc1", -1); compare(STABLE, RC2, -1); + // surprising one, remember .1 is qualifier + compare(STABLE, "10.5.8.1", -1); + compare(STABLE, "10.5.8-1", -1); + compare(RC2, "10.5.8-1", -1); CatalogItemComparator cmp = CatalogItemComparator.INSTANCE; assertTrue(cmp.compare(v(RC2), v("10.5.8-beta1")) == cmp.compare(v(RC2), v("10.5.8-beta3"))); diff --git a/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogVersioningTest.java b/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogVersioningTest.java index 3c09bb9e09..6748fdf6c4 100644 --- a/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogVersioningTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/catalog/internal/CatalogVersioningTest.java @@ -69,6 +69,7 @@ public void testLegacyParsingVersion() { assertLegacyVersionParsesAs("http://foo:8080", null, null); } + @SuppressWarnings("deprecation") private static void assertLegacyVersionParsesAs(String versionedId, String id, String version) { if (version==null) { Assert.assertFalse(CatalogUtils.looksLikeVersionedId(versionedId)); diff --git a/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java b/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java index 81b69bf029..ec07c8ab6f 100644 --- a/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java @@ -18,6 +18,7 @@ */ package org.apache.brooklyn.core.typereg; +import org.apache.brooklyn.util.osgi.OsgiUtils; import org.testng.Assert; import org.testng.annotations.Test; @@ -41,47 +42,55 @@ public void testNames() { } public void testVersions() { - assertVersion("1", true, true); - assertVersion("1.0.0", true, true); - assertVersion("1.0.0.SNAPSHOT", true, true); + assertVersion("1", true, true, true); + assertVersion("1.0.0", true, true, true); + assertVersion("1.0.0.SNAPSHOT", true, true, false); + assertVersion("1.0.0-SNAPSHOT", true, false, true); - assertVersion("", false, false); - assertVersion(null, false, false); - assertVersion("1:1", false, false); + assertVersion("", false, false, false); + assertVersion(null, false, false, false); + assertVersion("1:1", false, false, false); - assertVersion("1.SNAPSHOT", true, false); - assertVersion("1.0.0_SNAPSHOT", true, false); - assertVersion(".1", true, false); - assertVersion("v1", true, false); - assertVersion("!$&", true, false); + assertVersion("1.SNAPSHOT", true, false, false); + assertVersion("1.0.0_SNAPSHOT", true, false, false); + assertVersion(".1", true, false, false); + assertVersion("v1", true, false, false); + assertVersion("!$&", true, false, false); } public void testNameColonVersion() { - assertNameColonVersion("foo:1", true, true); - assertNameColonVersion("1:1", true, true); - assertNameColonVersion("a-package.1-foo-bar:1.0.0.SNAPSHOT_dev", true, true); + assertNameColonVersion("foo:1", true, true, true); + assertNameColonVersion("1:1", true, true, true); + assertNameColonVersion("a-package.1-foo-bar:1.0.0.SNAPSHOT_dev", true, true, false); + assertNameColonVersion("a-package.1-foo-bar:1.0.0-SNAPSHOT_dev", true, false, true); - assertNameColonVersion("", false, false); - assertNameColonVersion(null, false, false); - assertNameColonVersion("foo:", false, false); - assertNameColonVersion(":1", false, false); + assertNameColonVersion("", false, false, false); + assertNameColonVersion(null, false, false, false); + assertNameColonVersion("foo:", false, false, false); + assertNameColonVersion(":1", false, false, false); - assertNameColonVersion("foo:1.SNAPSHOT", true, false); - assertNameColonVersion("foo:v1", true, false); - assertNameColonVersion("foo...bar!:1", true, false); + assertNameColonVersion("foo:1.SNAPSHOT", true, false, false); + assertNameColonVersion("foo:v1", true, false, false); + assertNameColonVersion("foo...bar!:1", true, false, false); } private void assertName(String candidate, boolean isUsable, boolean isGood) { Assert.assertEquals(RegisteredTypeNaming.isUsableTypeName(candidate), isUsable, "usable name '"+candidate+"'"); Assert.assertEquals(RegisteredTypeNaming.isGoodTypeName(candidate), isGood, "good name '"+candidate+"'"); } - private void assertVersion(String candidate, boolean isUsable, boolean isGood) { + private void assertVersion(String candidate, boolean isUsable, boolean isOsgi, boolean isGood) { Assert.assertEquals(RegisteredTypeNaming.isUsableVersion(candidate), isUsable, "usable version '"+candidate+"'"); - Assert.assertEquals(RegisteredTypeNaming.isOsgiLegalVersion(candidate), isGood, "good version '"+candidate+"'"); + Assert.assertEquals(RegisteredTypeNaming.isValidOsgiVersion(candidate), isOsgi, "osgi version '"+candidate+"'"); + Assert.assertEquals(RegisteredTypeNaming.isGoodBrooklynVersion(candidate), isGood, "good version '"+candidate+"'"); } - private void assertNameColonVersion(String candidate, boolean isUsable, boolean isGood) { + private void assertNameColonVersion(String candidate, boolean isUsable, boolean isOsgi, boolean isGood) { Assert.assertEquals(RegisteredTypeNaming.isUsableTypeColonVersion(candidate), isUsable, "usable name:version '"+candidate+"'"); - Assert.assertEquals(RegisteredTypeNaming.isGoodTypeColonVersion(candidate), isGood, "good name:version '"+candidate+"'"); + Assert.assertEquals(RegisteredTypeNaming.isValidOsgiTypeColonVersion(candidate), isOsgi, "osgi name:version '"+candidate+"'"); + Assert.assertEquals(RegisteredTypeNaming.isGoodBrooklynTypeColonVersion(candidate), isGood, "good name:version '"+candidate+"'"); } + public void testConvertToOsgiVersion() { + Assert.assertEquals(OsgiUtils.toOsgiVersion("1-foo"), "1.0.0.foo"); + Assert.assertEquals(OsgiUtils.toOsgiVersion("1"), "1.0.0"); + } } diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java b/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java new file mode 100644 index 0000000000..9a59d11813 --- /dev/null +++ b/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.brooklyn.util.text; + +/** Utilities for parsing and working with versions following the recommended Brooklyn scheme, + * following major.minor.patch-qualifier syntax, + * with support for mapping to OSGi. + *

+ * See {@link VersionComparator} and its tests for examples. + */ +public class BrooklynVersionSyntax { + + private static final String USABLE_REGEX = "[^:\\s/\\\\]+"; + + private final static String OSGI_TOKEN_CHARS = "A-Za-z0-9_-"; + private final static String OSGI_TOKEN_REGEX = "[" + OSGI_TOKEN_CHARS + "]+"; + private final static String NUMBER = "[0-9]+"; + private final static String QUALIFIER = OSGI_TOKEN_REGEX; + + public final static String VALID_OSGI_VERSION_REGEX = + NUMBER + + "(" + "\\." + NUMBER + + "(" + "\\." + NUMBER + + "(" + "\\." + QUALIFIER + + ")?" + + ")?" + + ")?"; + + public final static String GOOD_BROOKLYN_VERSION_REGEX = + NUMBER + + "(" + "\\." + NUMBER + + "(" + "\\." + NUMBER + + "(" + "-" + QUALIFIER + + ")?" + + ")?" + + ")?"; + + private static boolean isUsable(String candidate) { + return candidate!=null && candidate.matches(USABLE_REGEX); + } + + /** + * For versions we currently work with any non-empty string that does not contain a ':' or whitespace. + * However we discourage things that are not OSGi versions; see {@link #isValidOsgiVersion(String)}. + * In some places (eg bundles) the use of OSGi version syntax may be enforced. + */ + public static boolean isUsableVersion(String candidate) { + return isUsable(candidate); + } + + /** True if the argument matches the Brooklyn version syntax, + * MAJOR.MINOR.POINT-QUALIFIER or part thereof (not ending in a period though), + * where the first three are whole numbers and the final piece is any valid OSGi token + * (something satisfying {@link #isGoodTypeName(String)} with no periods). + * See also {@link #isValidOsgiVersion(String)} and note this _requires_ a different separator to OSGi. + */ + public static boolean isGoodBrooklynVersion(String candidate) { + return candidate!=null && candidate.matches(GOOD_BROOKLYN_VERSION_REGEX); + } + + /** True if the argument matches the OSGi version spec, of the form + * MAJOR.MINOR.POINT.QUALIFIER or part thereof (not ending in a period though), + * where the first three are whole numbers and the final piece is any valid OSGi token + * (something satisfying {@link #isGoodTypeName(String)} with no periods). + * See also {@link #isGoodVersion(String)}. + */ + public static boolean isValidOsgiVersion(String candidate) { + return candidate!=null && candidate.matches(VALID_OSGI_VERSION_REGEX); + } + +} diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java b/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java index e5d1fbfaf8..09f430d425 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java @@ -27,11 +27,18 @@ /** * {@link Comparator} for version strings. *

- * SNAPSHOT items always lowest rated, then looking for no qualifier, then doing natural order comparison. - * This gives the desired semantics for our recommended version syntax. + * This gives the desired semantics for our recommended version syntax, + * following major.minor.patch-qualifier syntax, + * doing numeric order of major/minor/patch (1.10 > 1.9), + * treating anything with a qualifier lower than the corresponding without but higher than items before + * (1.2 > 1.2-rc > 1.1), + * SNAPSHOT items always lowest rated (1.0 > 2-SNAPSHOT), and + * qualifier sorting follows {@link NaturalOrderComparator} (1-M10 > 1-M9). *

* Impossible to follow semantics for all versioning schemes but * does the obvious right thing for normal schemes and pretty well in fringe cases. + * Probably the most surprising fringe behaviours will be + * 1.2.3.4 < 1.2.3 (the ".4" is considered a qualifier). *

* See test case for lots of examples. */ diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/text/ComparableVersionTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/text/ComparableVersionTest.java index a730ade4e1..059e2e372a 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/text/ComparableVersionTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/text/ComparableVersionTest.java @@ -35,8 +35,9 @@ public void testBasicOnes() { Assert.assertTrue(v.isLessThanAndNotEqualTo("10.6")); Assert.assertTrue(v.isLessThanOrEqualTo("10.5.8")); Assert.assertFalse(v.isLessThanAndNotEqualTo("10.5.8")); - - Assert.assertTrue(v.isLessThanAndNotEqualTo("10.5.8.1")); + + // this one is surprising -- but we don't support sub-patch numbers, the ".1" is a qualifier + Assert.assertTrue(v.isGreaterThanAndNotEqualTo("10.5.8.1")); Assert.assertTrue(v_rc2.isLessThanAndNotEqualTo("10.5.8-rc3")) ; Assert.assertTrue(v_rc2.isGreaterThanAndNotEqualTo("10.5.8-rc1")); From ed98e27e934ddf54d6c1c80b4a61afaab1225e47 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Wed, 21 Jun 2017 18:26:43 +0100 Subject: [PATCH 10/12] add routines to convert brooklyn/osgi versions and bundle finder supports either, and does the conversion for you --- .../apache/brooklyn/core/BrooklynVersion.java | 5 + .../internal/BasicBrooklynCatalog.java | 4 +- .../brooklyn/util/core/ClassLoaderUtils.java | 5 +- .../apache/brooklyn/util/core/osgi/Osgis.java | 18 ++- .../typereg/RegisteredTypeNamingTest.java | 6 +- .../util/core/ClassLoaderUtilsTest.java | 2 +- .../core/xstream/OsgiClassPrefixerTest.java | 2 +- .../apache/brooklyn/util/osgi/OsgiUtils.java | 4 + .../util/text/BrooklynVersionSyntax.java | 103 +++++++++++++++++- .../brooklyn/util/osgi/OsgiUtilsTest.java | 3 + .../brooklyn/util/osgi/VersionedNameTest.java | 3 +- .../util/text/BrooklynVersionSyntaxTest.java | 98 +++++++++++++++++ 12 files changed, 239 insertions(+), 14 deletions(-) create mode 100644 utils/common/src/test/java/org/apache/brooklyn/util/text/BrooklynVersionSyntaxTest.java diff --git a/core/src/main/java/org/apache/brooklyn/core/BrooklynVersion.java b/core/src/main/java/org/apache/brooklyn/core/BrooklynVersion.java index 161eff0903..f28d3fbde2 100644 --- a/core/src/main/java/org/apache/brooklyn/core/BrooklynVersion.java +++ b/core/src/main/java/org/apache/brooklyn/core/BrooklynVersion.java @@ -45,6 +45,7 @@ import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.osgi.OsgiUtil; import org.apache.brooklyn.util.stream.Streams; +import org.apache.brooklyn.util.text.BrooklynVersionSyntax; import org.apache.brooklyn.util.text.Strings; import org.apache.brooklyn.util.text.VersionComparator; import org.osgi.framework.Bundle; @@ -458,4 +459,8 @@ public boolean equals(Object other) { return true; } } + + public static String getOsgiVersion() { + return BrooklynVersionSyntax.toValidOsgiVersion(get()); + } } diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java index 006d3e84ca..b0d028f282 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java @@ -59,8 +59,8 @@ import org.apache.brooklyn.util.javalang.AggregateClassLoader; import org.apache.brooklyn.util.javalang.JavaClassNames; import org.apache.brooklyn.util.javalang.LoadedClassLoader; -import org.apache.brooklyn.util.osgi.OsgiUtils; import org.apache.brooklyn.util.osgi.VersionedName; +import org.apache.brooklyn.util.text.BrooklynVersionSyntax; import org.apache.brooklyn.util.text.Strings; import org.apache.brooklyn.util.time.Duration; import org.apache.brooklyn.util.time.Time; @@ -448,7 +448,7 @@ public static VersionedName getVersionedName(Map catalogMetadata, boolean r if (Strings.isBlank(version)) { throw new IllegalStateException("Catalog BOM must define version if bundle is defined"); } - return new VersionedName(bundle, Version.valueOf(OsgiUtils.toOsgiVersion(version))); + return new VersionedName(bundle, Version.valueOf(BrooklynVersionSyntax.toValidOsgiVersion(version))); } private void collectCatalogItems(String yaml, List> result, Map parentMeta) { diff --git a/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java b/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java index a6be53d081..8d5cf0f5fd 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java @@ -40,6 +40,7 @@ import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.osgi.OsgiUtils; +import org.apache.brooklyn.util.text.BrooklynVersionSyntax; import org.apache.brooklyn.util.text.Strings; import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; @@ -65,7 +66,7 @@ public class ClassLoaderUtils { static final String WHITE_LIST_KEY = "org.apache.brooklyn.classloader.fallback.bundles"; static final String CLASS_NAME_DELIMITER = ":"; private static final String WHITE_LIST_DEFAULT = - "org\\.apache\\.brooklyn\\..*:" + OsgiUtils.toOsgiVersion(BrooklynVersion.get()); + "org\\.apache\\.brooklyn\\..*:" + BrooklynVersion.getOsgiVersion(); // Class.forName gets the class loader from the calling class. // We don't have access to the same reflection API so need to pass it explicitly. @@ -307,7 +308,7 @@ protected Maybe tryLoadFromBundle(LoaderDispatcher dispatcher, String if (framework != null) { Maybe bundle = Osgis.bundleFinder(framework) .symbolicName(symbolicName) - .version(OsgiUtils.toOsgiVersion(version)) + .version(version) .find(); if (bundle.isAbsent()) { throw new IllegalStateException("Bundle " + toBundleString(symbolicName, version) diff --git a/core/src/main/java/org/apache/brooklyn/util/core/osgi/Osgis.java b/core/src/main/java/org/apache/brooklyn/util/core/osgi/Osgis.java index a33b4467e8..d46f098d48 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/osgi/Osgis.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/osgi/Osgis.java @@ -39,6 +39,7 @@ import org.apache.brooklyn.util.os.Os; import org.apache.brooklyn.util.osgi.OsgiUtils; import org.apache.brooklyn.util.stream.Streams; +import org.apache.brooklyn.util.text.BrooklynVersionSyntax; import org.apache.brooklyn.util.text.Strings; import org.osgi.framework.Bundle; import org.osgi.framework.BundleException; @@ -98,6 +99,7 @@ public BundleFinder symbolicName(String symbolicName) { return this; } + /** Accepts non-osgi version syntax, converting to OSGi version syntax */ public BundleFinder version(String version) { this.version = version; return this; @@ -174,12 +176,26 @@ protected Maybe findOne(boolean requireExactlyOne) { } /** Finds all matching bundles, in decreasing version order. */ + @SuppressWarnings("deprecation") public List findAll() { boolean urlMatched = false; List result = MutableList.of(); + String v=null, vDep = null; + if (version!=null) { + v = BrooklynVersionSyntax.toValidOsgiVersion(version); + vDep = OsgiUtils.toOsgiVersion(version); + } for (Bundle b: framework.getBundleContext().getBundles()) { if (symbolicName!=null && !symbolicName.equals(b.getSymbolicName())) continue; - if (version!=null && !Version.parseVersion(version).equals(b.getVersion())) continue; + if (version!=null) { + String bv = b.getVersion().toString(); + if (!v.equals(bv)) { + if (!vDep.equals(bv)) { + continue; + } + LOG.warn("Legacy inferred OSGi version string '"+vDep+"' found to match "+symbolicName+":"+version+"; switch to '"+v+"' format to avoid issues with deprecated version syntax"); + } + } if (!Predicates.and(predicates).apply(b)) continue; // check url last, because if it isn't mandatory we should only clear if we find a url diff --git a/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java b/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java index ec07c8ab6f..3fc8dc3d24 100644 --- a/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java @@ -88,9 +88,5 @@ private void assertNameColonVersion(String candidate, boolean isUsable, boolean Assert.assertEquals(RegisteredTypeNaming.isValidOsgiTypeColonVersion(candidate), isOsgi, "osgi name:version '"+candidate+"'"); Assert.assertEquals(RegisteredTypeNaming.isGoodBrooklynTypeColonVersion(candidate), isGood, "good name:version '"+candidate+"'"); } - - public void testConvertToOsgiVersion() { - Assert.assertEquals(OsgiUtils.toOsgiVersion("1-foo"), "1.0.0.foo"); - Assert.assertEquals(OsgiUtils.toOsgiVersion("1"), "1.0.0"); - } + } diff --git a/core/src/test/java/org/apache/brooklyn/util/core/ClassLoaderUtilsTest.java b/core/src/test/java/org/apache/brooklyn/util/core/ClassLoaderUtilsTest.java index a9ea938ec7..1477798a0a 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/ClassLoaderUtilsTest.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/ClassLoaderUtilsTest.java @@ -283,7 +283,7 @@ public void testLoadBrooklynClass() throws Exception { Entity.class.getName()); new ClassLoaderUtils(this, mgmt).loadClass( "org.apache.brooklyn.api", - OsgiUtils.toOsgiVersion(BrooklynVersion.get()), + BrooklynVersion.getOsgiVersion(), Entity.class.getName()); try { new ClassLoaderUtils(this, mgmt).loadClass( diff --git a/core/src/test/java/org/apache/brooklyn/util/core/xstream/OsgiClassPrefixerTest.java b/core/src/test/java/org/apache/brooklyn/util/core/xstream/OsgiClassPrefixerTest.java index aa2e659f73..b1d4278c18 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/xstream/OsgiClassPrefixerTest.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/xstream/OsgiClassPrefixerTest.java @@ -62,7 +62,7 @@ public void testGetPrefixWithBundle() throws Exception { public void testGetPrefixWithWhitelistedBundle() throws Exception { final Bundle bundle = Mockito.mock(Bundle.class); Mockito.when(bundle.getSymbolicName()).thenReturn("org.apache.brooklyn.my-bundle"); - Mockito.when(bundle.getVersion()).thenReturn(Version.valueOf(OsgiUtils.toOsgiVersion(BrooklynVersion.get()))); + Mockito.when(bundle.getVersion()).thenReturn(Version.valueOf(BrooklynVersion.getOsgiVersion())); Function, Optional> bundleRetriever = new Function, Optional>() { @Override public Optional apply(Class input) { diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/osgi/OsgiUtils.java b/utils/common/src/main/java/org/apache/brooklyn/util/osgi/OsgiUtils.java index df416864cc..1fe07f5b8a 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/osgi/OsgiUtils.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/osgi/OsgiUtils.java @@ -25,6 +25,7 @@ import java.util.regex.Pattern; import org.apache.brooklyn.util.guava.Maybe; +import org.apache.brooklyn.util.text.BrooklynVersionSyntax; import org.apache.brooklyn.util.text.Strings; import org.osgi.framework.Bundle; import org.osgi.framework.Constants; @@ -101,6 +102,9 @@ public static Maybe parseOsgiIdentifier(String symbolicNameOption return Maybe.of(new VersionedName(parts[0], v)); } + /** @deprecated since 0.12.0 use {@link BrooklynVersionSyntax#toValidOsgiVersion(String)}; + * but note it has slightly different semantics in some odd cases, e.g. this maps "1x" to "0.0.0.1x" whereas + * the new method maps to "1.0.0.x" */ public static String toOsgiVersion(String version) { if (version != null) { return DefaultMaven2OsgiConverter.cleanupVersion(version); diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java b/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java index 9a59d11813..63c69e17de 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java @@ -18,11 +18,16 @@ */ package org.apache.brooklyn.util.text; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import com.google.common.base.Preconditions; + /** Utilities for parsing and working with versions following the recommended Brooklyn scheme, * following major.minor.patch-qualifier syntax, * with support for mapping to OSGi. *

- * See {@link VersionComparator} and its tests for examples. + * See tests for examples, {@link VersionComparator} for more notes, and its tests for more examples. */ public class BrooklynVersionSyntax { @@ -84,4 +89,100 @@ public static boolean isValidOsgiVersion(String candidate) { return candidate!=null && candidate.matches(VALID_OSGI_VERSION_REGEX); } + /** Creates a string satisfying {@link #isValidOsgiVersion(String)} based on the input. + * For input satisfying {@link #isGoodBrooklynVersion(String)} the only change will be in the qualifer separator + * (from "-" to ".") and making any "0" minor/patch token explicit (so "1-x" becomes "1.0.0.x"), + * and the change can be reversed using {@link #toGoodBrooklynVersion(String)} (modulo insertion of "0"'s for minor/patch numbers if missing). + * For input satisfying {@link #isValidOsgiVersion(String)}, the only change will be insertions of 0 for minor/patch. + * Precise behaviour for other input is not guaranteed but callers can expect output which resembles the input, + * with any major/minor/patch string at the front preserved and internal contiguous alphanumeric sequences preserved. */ + public static String toValidOsgiVersion(String input) { + Preconditions.checkNotNull(input); + return toGoodVersion(input, ".", true); + /* Note Maven has and used: DefaultMaven2OsgiConverter + * from https://github.com/apache/felix/blob/trunk/tools/maven-bundle-plugin/src/main/java/org/apache/maven/shared/osgi/DefaultMaven2OsgiConverter.java + * but it (a) is more complicated, and (b) doesn't aggressively find numbers e.g. "1beta" goes to "0.0.0.1beta" instead of "1.0.0.beta" + */ + } + + /** Creates a string satisfying {@link #isGoodBrooklynVersion(String)} based on the input. + * For input satisfying {@link #isGoodBrooklynVersion(String)} the input will be returned unchanged. + * For input satisfying {@link #isValidOsgiVersion(String)} the qualifier separator will be changed to "-", + * and {@link #toValidOsgiVersion(String)} can be used to reverse the input (modulo insertion of "0"'s for minor/patch numbers if missing). + * Precise behaviour for other input is not guaranteed but callers can expect output which resembles the input, + * with any major/minor/patch string at the front preserved and internal contiguous alphanumeric sequences preserved. */ + public static String toGoodBrooklynVersion(String input) { + return toGoodVersion(input, "-", false); + } + + private static String toGoodVersion(String input, String qualifierSeparator, boolean requireMinorAndPatch) { + Preconditions.checkNotNull(input); + final String FUZZY_REGEX = + "(" + NUMBER + "(" + "\\." + NUMBER + "(" + "\\." + NUMBER + ")?)?)?" + + "(" + ".*)"; + Matcher m = Pattern.compile(FUZZY_REGEX).matcher(input); + if (!m.matches()) { + throw new IllegalStateException("fuzzy matcher should match anything: '"+input+"'"); // sanity check - shouldn't happen + } + StringBuilder result = new StringBuilder(); + if (Strings.isEmpty(m.group(1))) { + result.append("0.0.0"); + } else { + result.append(m.group(1)); + if (requireMinorAndPatch) { + if (Strings.isEmpty(m.group(2))) { + result.append(".0"); + } + if (Strings.isEmpty(m.group(3))) { + result.append(".0"); + } + } + } + String q = m.group(4); + if (Strings.isNonEmpty(q)) { + boolean collapsedUnsupported = false; + boolean starting = true; + result.append(qualifierSeparator); + for (int i=0; i= c) || ('a' <= c && 'z' >= c) || ('0' <= c && '9' >= c); + starting = false; + if (!include) { + if (c=='-' || c=='_' || c=='.') { + // treat these as separator chars, and drop them + if (q.length()==1) { + // unless there are no other chars (e.g. version "1." becomes "1-_" + unsupported = true; + } + } else { + // treat other chars as unsupported + unsupported = true; + } + } + } else { + include = ('A' <= c && 'Z' >= c) || ('a' <= c && 'z' >= c) || ('0' <= c && '9' >= c) || c=='-' || c=='_'; + if (!include) { + // treat as unsupported, unless we've collapsed + if (!collapsedUnsupported) { + unsupported = true; + } + } + } + if (include) { + result.append(c); + collapsedUnsupported = false; + } else if (unsupported) { + // stick a "_" in for unsupported chars + result.append('_'); + collapsedUnsupported = true; + } + } + } + return result.toString(); + } + } diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/osgi/OsgiUtilsTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/osgi/OsgiUtilsTest.java index e41b00aac0..7a8550e0b0 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/osgi/OsgiUtilsTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/osgi/OsgiUtilsTest.java @@ -21,6 +21,8 @@ public class OsgiUtilsTest { + // TODO other utils tests ... or maybe other tests in this package are sufficient? + @Test public void testToOsgiVersion() { assertVersion("0.10.0-20160713.1653", "0.10.0.20160713_1653"); @@ -43,6 +45,7 @@ public void testToOsgiVersion() { assertVersion("4aug2000r7-dev", "0.0.0.4aug2000r7-dev"); } + @Deprecated private void assertVersion(String ver, String expected) { assertEquals(OsgiUtils.toOsgiVersion(ver), expected); } diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/osgi/VersionedNameTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/osgi/VersionedNameTest.java index c838258115..f1626736eb 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/osgi/VersionedNameTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/osgi/VersionedNameTest.java @@ -16,6 +16,7 @@ package org.apache.brooklyn.util.osgi; import org.apache.brooklyn.util.javalang.coerce.TypeCoercerExtensible; +import org.apache.brooklyn.util.text.BrooklynVersionSyntax; import org.osgi.framework.Version; import org.testng.Assert; import org.testng.annotations.Test; @@ -37,7 +38,7 @@ public void testDoesNotAcceptInvalidVersions() { @Test public void testManuallyCorrectingVersion() { Assert.assertEquals(new VersionedName("foo", new Version("1.0.0.alpha")), VersionedName.fromString("foo:"+ - OsgiUtils.toOsgiVersion("1.0-alpha"))); + BrooklynVersionSyntax.toValidOsgiVersion("1.0-alpha"))); } } diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/text/BrooklynVersionSyntaxTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/text/BrooklynVersionSyntaxTest.java new file mode 100644 index 0000000000..b938935245 --- /dev/null +++ b/utils/common/src/test/java/org/apache/brooklyn/util/text/BrooklynVersionSyntaxTest.java @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.brooklyn.util.text; + +import org.testng.Assert; +import org.testng.annotations.Test; + +@Test +public class BrooklynVersionSyntaxTest { + + public void testVersions() { + assertVersion("1", true, true, true); + assertVersion("1.0.0", true, true, true); + assertVersion("1.0.0.SNAPSHOT", true, true, false); + assertVersion("1.0.0-SNAPSHOT", true, false, true); + + assertVersion("", false, false, false); + assertVersion(null, false, false, false); + assertVersion("1:1", false, false, false); + + assertVersion("1.SNAPSHOT", true, false, false); + assertVersion("1.0.0_SNAPSHOT", true, false, false); + assertVersion(".1", true, false, false); + assertVersion("v1", true, false, false); + assertVersion("!$&", true, false, false); + } + + public void testConvert() { + assertConverts("0", "0", "0.0.0"); + assertConverts("1.1", "1.1", "1.1.0"); + assertConverts("x", "0.0.0-x", "0.0.0.x"); + assertConverts("0.x", "0-x", "0.0.0.x"); + assertConverts("1.1.x", "1.1-x", "1.1.0.x"); + assertConverts("1.1.1.1.x", "1.1.1-1_x", "1.1.1.1_x"); + assertConverts("1x", "1-x", "1.0.0.x"); + assertConverts("1$", "1-_", "1.0.0._"); + assertConverts("1-", "1-_", "1.0.0._"); + assertConverts("1.1.", "1.1-_", "1.1.0._"); + assertConverts("1$$$", "1-_", "1.0.0._"); + assertConverts("1._$", "1-__", "1.0.0.__"); + assertConverts("1a$$$", "1-a_", "1.0.0.a_"); + assertConverts("1a$$$SNAPSHOT", "1-a_SNAPSHOT", "1.0.0.a_SNAPSHOT"); + } + + private void assertConverts(String input, String bklyn, String osgi) { + Assert.assertEquals(BrooklynVersionSyntax.toGoodBrooklynVersion(input), bklyn, "conversion to good brooklyn"); + Assert.assertEquals(BrooklynVersionSyntax.toValidOsgiVersion(input), osgi, "conversion to valid osgi"); + } + + private void assertVersion(String candidate, boolean isUsable, boolean isOsgi, boolean isGood) { + Assert.assertEquals(BrooklynVersionSyntax.isUsableVersion(candidate), isUsable, "usable version '"+candidate+"'"); + Assert.assertEquals(BrooklynVersionSyntax.isValidOsgiVersion(candidate), isOsgi, "osgi version '"+candidate+"'"); + Assert.assertEquals(BrooklynVersionSyntax.isGoodBrooklynVersion(candidate), isGood, "good version '"+candidate+"'"); + } + + private void assertOsgiVersion(String input, String osgi) { + Assert.assertEquals(BrooklynVersionSyntax.toValidOsgiVersion(input), osgi, "conversion to valid osgi"); + } + + public void testOsgiVersions() { + assertOsgiVersion("0.10.0-20160713.1653", "0.10.0.20160713_1653"); + + assertOsgiVersion("2.1.0-SNAPSHOT", "2.1.0.SNAPSHOT"); + assertOsgiVersion("2.1-SNAPSHOT", "2.1.0.SNAPSHOT"); + assertOsgiVersion("0.1-SNAPSHOT", "0.1.0.SNAPSHOT"); + assertOsgiVersion("2-SNAPSHOT", "2.0.0.SNAPSHOT"); + assertOsgiVersion("2", "2.0.0"); + assertOsgiVersion("2.1", "2.1.0"); + assertOsgiVersion("2.1.3", "2.1.3"); + assertOsgiVersion("2.1.3.4", "2.1.3.4"); + assertOsgiVersion("1.1-alpha-2", "1.1.0.alpha-2"); + assertOsgiVersion("1.0-alpha-16-20070122.203121-13", "1.0.0.alpha-16-20070122_203121-13"); + assertOsgiVersion("1.0-20070119.021432-1", "1.0.0.20070119_021432-1"); + assertOsgiVersion("1-20070119.021432-1", "1.0.0.20070119_021432-1"); + assertOsgiVersion("1.4.1-20070217.082013-7", "1.4.1.20070217_082013-7"); + assertOsgiVersion("0.0.0.4aug2000r7-dev", "0.0.0.4aug2000r7-dev"); + assertOsgiVersion("0-4aug2000r7-dev", "0.0.0.4aug2000r7-dev"); + assertOsgiVersion("-4aug2000r7-dev", "0.0.0.4aug2000r7-dev"); + // potentially surprising, and different to maven (0.0.0.4aug..) + assertOsgiVersion("4aug2000r7-dev", "4.0.0.aug2000r7-dev"); + } +} From 0236dbcd5809827c0f2548561c1ddfa5cc26d7a3 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Wed, 28 Jun 2017 11:45:28 +0100 Subject: [PATCH 11/12] address comments on PR in #737 --- .../BrooklynComponentTemplateResolver.java | 2 +- ...n.spi.creation.service.ServiceTypeResolver | 19 ------------------- .../internal/BasicBrooklynCatalog.java | 4 ++-- .../core/typereg/RegisteredTypeNaming.java | 9 +++++---- 4 files changed, 8 insertions(+), 26 deletions(-) delete mode 100644 camp/camp-brooklyn/src/test/resources/META-INF/services/org.apache.brooklyn.camp.brooklyn.spi.creation.service.ServiceTypeResolver diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java index ae92c922ae..abc2e8ea87 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java @@ -76,7 +76,7 @@ import com.google.common.collect.Maps; /** - * This generates instances of a template resolver that use a {@link ServiceTypeResolver} + * This generates instances of a template resolver that use a {@link EntitySpecResolver} * to parse the {@code serviceType} line in the template. */ public class BrooklynComponentTemplateResolver { diff --git a/camp/camp-brooklyn/src/test/resources/META-INF/services/org.apache.brooklyn.camp.brooklyn.spi.creation.service.ServiceTypeResolver b/camp/camp-brooklyn/src/test/resources/META-INF/services/org.apache.brooklyn.camp.brooklyn.spi.creation.service.ServiceTypeResolver deleted file mode 100644 index 3bba7fb58b..0000000000 --- a/camp/camp-brooklyn/src/test/resources/META-INF/services/org.apache.brooklyn.camp.brooklyn.spi.creation.service.ServiceTypeResolver +++ /dev/null @@ -1,19 +0,0 @@ -# -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. -# -org.apache.brooklyn.camp.brooklyn.spi.creation.service.TestServiceTypeResolver \ No newline at end of file diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java index e4c98824dd..3817665c44 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java @@ -908,7 +908,7 @@ private boolean attemptType(String key, CatalogItemType candidateCiType) { } { // legacy routine; should be the same as above code added in 0.12 because: - // if type is symbolic_name, the type will match below, and version will be null so any version allowed to match + // if type is symbolic_name, the type will match above, and version will be null so any version allowed to match // if type is symbolic_name:version, the id will match, and the version will also have to match // SHOULD NEVER NEED THIS - remove during or after 0.13 String typeWithId = type; @@ -922,7 +922,7 @@ private boolean attemptType(String key, CatalogItemType candidateCiType) { if (candidateCiType == candidate.getCatalogItemType() && (type.equals(candidate.getSymbolicName()) || type.equals(candidate.getId()))) { if (version==null || version.equals(candidate.getVersion())) { - log.warn("Lookup of '"+type+"' version '"+version+"' only worked using legacy routines; please advise Brooklyn community so they understand why"); + log.error("Lookup of '"+type+"' version '"+version+"' only worked using legacy routines; please advise Brooklyn community so they understand why"); // matched - exit catalogItemType = candidateCiType; planYaml = candidateYaml; diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java b/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java index 22e9e7885a..8e11ae976f 100644 --- a/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java +++ b/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java @@ -35,9 +35,9 @@ public class RegisteredTypeNaming { public final static String QUALIFIER = OSGI_TOKEN_REGEX; public final static String VERSION_REGEX = NUMBER + - "(" + "\\." + NUMBER + - "(" + "\\." + NUMBER + - "(" + "\\." + QUALIFIER + + "(" + DOT + NUMBER + + "(" + DOT + NUMBER + + "(" + DOT + QUALIFIER + ")?" + ")?" + ")?"; @@ -49,7 +49,8 @@ private static boolean isUsable(String candidate) { /** * For type names we currently work with any non-empty string that does not contain * a ':' or whitespace or forward slash or backslash. - * However we discourage things that are not OSGi symbolic names; see {@link #isValidTypeName(String)}. + * However we discourage things that are not OSGi symbolic names; + * see {@link #isGoodTypeName(String)}. * In some places (eg bundles) the use of OSGi symbolic names may be enforced. */ public static boolean isUsableTypeName(String candidate) { From ee853f5806f3d2d89f2c76521ec236695f61f80d Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Wed, 28 Jun 2017 12:24:36 +0100 Subject: [PATCH 12/12] tidy esp javadoc and regexes, as per PR comments --- .../util/text/BrooklynVersionSyntax.java | 29 ++++++++++--------- .../util/text/NaturalOrderComparator.java | 2 ++ .../brooklyn/util/text/VersionComparator.java | 13 ++++----- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java b/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java index 63c69e17de..e16dcf887c 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java @@ -31,26 +31,27 @@ */ public class BrooklynVersionSyntax { - private static final String USABLE_REGEX = "[^:\\s/\\\\]+"; + public static final String USABLE_REGEX = "[^:\\s/\\\\]+"; + public static final String DOT = "\\."; - private final static String OSGI_TOKEN_CHARS = "A-Za-z0-9_-"; - private final static String OSGI_TOKEN_REGEX = "[" + OSGI_TOKEN_CHARS + "]+"; - private final static String NUMBER = "[0-9]+"; - private final static String QUALIFIER = OSGI_TOKEN_REGEX; + public final static String OSGI_TOKEN_CHARS = "A-Za-z0-9_-"; + public final static String OSGI_TOKEN_REGEX = "[" + OSGI_TOKEN_CHARS + "]+"; + public final static String NUMBER = "[0-9]+"; + public final static String QUALIFIER = OSGI_TOKEN_REGEX; public final static String VALID_OSGI_VERSION_REGEX = NUMBER + - "(" + "\\." + NUMBER + - "(" + "\\." + NUMBER + - "(" + "\\." + QUALIFIER + + "(" + DOT + NUMBER + + "(" + DOT + NUMBER + + "(" + DOT + QUALIFIER + ")?" + ")?" + ")?"; public final static String GOOD_BROOKLYN_VERSION_REGEX = NUMBER + - "(" + "\\." + NUMBER + - "(" + "\\." + NUMBER + + "(" + DOT + NUMBER + + "(" + DOT + NUMBER + "(" + "-" + QUALIFIER + ")?" + ")?" + @@ -72,7 +73,7 @@ public static boolean isUsableVersion(String candidate) { /** True if the argument matches the Brooklyn version syntax, * MAJOR.MINOR.POINT-QUALIFIER or part thereof (not ending in a period though), * where the first three are whole numbers and the final piece is any valid OSGi token - * (something satisfying {@link #isGoodTypeName(String)} with no periods). + * (containing letters, numbers, _ and -; no full stops). * See also {@link #isValidOsgiVersion(String)} and note this _requires_ a different separator to OSGi. */ public static boolean isGoodBrooklynVersion(String candidate) { @@ -82,8 +83,8 @@ public static boolean isGoodBrooklynVersion(String candidate) { /** True if the argument matches the OSGi version spec, of the form * MAJOR.MINOR.POINT.QUALIFIER or part thereof (not ending in a period though), * where the first three are whole numbers and the final piece is any valid OSGi token - * (something satisfying {@link #isGoodTypeName(String)} with no periods). - * See also {@link #isGoodVersion(String)}. + * (containing letters, numbers, _ and -; no full stops). + * See also {@link #isGoodBrooklynVersion(String)}. */ public static boolean isValidOsgiVersion(String candidate) { return candidate!=null && candidate.matches(VALID_OSGI_VERSION_REGEX); @@ -118,7 +119,7 @@ public static String toGoodBrooklynVersion(String input) { private static String toGoodVersion(String input, String qualifierSeparator, boolean requireMinorAndPatch) { Preconditions.checkNotNull(input); final String FUZZY_REGEX = - "(" + NUMBER + "(" + "\\." + NUMBER + "(" + "\\." + NUMBER + ")?)?)?" + + "(" + NUMBER + "(" + DOT + NUMBER + "(" + DOT + NUMBER + ")?)?)?" + "(" + ".*)"; Matcher m = Pattern.compile(FUZZY_REGEX).matcher(input); if (!m.matches()) { diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/text/NaturalOrderComparator.java b/utils/common/src/main/java/org/apache/brooklyn/util/text/NaturalOrderComparator.java index 82f4e86ced..bade244942 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/text/NaturalOrderComparator.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/text/NaturalOrderComparator.java @@ -58,6 +58,8 @@ * but after considering the remainder of the string, so "09.b" > "9.a". * decimals are treated like a word-split, not a decimal point, i.e. "1.9" < "1.10". *

+ * this does not return anything as equal unless they are the same string. + *

* class is thread-safe. *

* nulls not supported. to support nulls, wrap in guava: diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java b/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java index 09f430d425..120a210b69 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/text/VersionComparator.java @@ -82,8 +82,8 @@ public int compare(String v1, String v2) { TwoBooleans snapshots = TwoBooleans.of(isSnapshot(v1), isSnapshot(v2)); if (snapshots.different()) return snapshots.compare(true); - String u1 = versionWithQualifier(v1); - String u2 = versionWithQualifier(v2); + String u1 = versionWithoutQualifier(v1); + String u2 = versionWithoutQualifier(v2); int uq = NaturalOrderComparator.INSTANCE.compare(u1, u2); if (uq!=0) return uq; @@ -111,13 +111,12 @@ public int compare(String v1, String v2) { // finally, do normal comparison if both have qualifier or both unqualified (in case leading 0's are used) return NaturalOrderComparator.INSTANCE.compare(v1, v2); - - // (previously we did this but don't think we need any of that complexity) - //return compareDotSplitParts(splitOnDot(v1), splitOnDot(v2)); } - private String versionWithQualifier(String v1) { - Matcher q = Pattern.compile("([0-9]+(\\.[0-9]+(\\.[0-9])?)?)(.*)").matcher(v1); + private String versionWithoutQualifier(String v1) { + Matcher q = Pattern.compile("("+BrooklynVersionSyntax.NUMBER+ + "("+BrooklynVersionSyntax.DOT+BrooklynVersionSyntax.NUMBER+ + "("+BrooklynVersionSyntax.DOT+BrooklynVersionSyntax.NUMBER+")?)?)(.*)").matcher(v1); return q.matches() ? q.group(1) : ""; }