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/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..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 @@ -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; @@ -79,10 +76,9 @@ 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. */ -@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 7fd6d8a91e..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.core.catalog.internal.CatalogUtils; -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>) CatalogUtils.getCatalogItemOptionalVersion(resolver.getManagementContext(), getBrooklynType(serviceType)); - } - - @Override - public void decorateSpec(BrooklynComponentTemplateResolver resolver, EntitySpec spec) { - spec.configure("resolver", TestServiceTypeResolver.class); - } - -} 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/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 5715b66def..3ccd0fc9d9 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; @@ -58,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; @@ -447,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) { @@ -601,14 +602,37 @@ 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.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 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"); + // 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.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); + } 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; } @@ -624,18 +648,38 @@ private void collectCatalogItems(String sourceYaml, Map itemMetadata, List< } } + String versionFromId = null; + 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 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)) { + // 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) { @@ -855,24 +899,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 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; + 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.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; + 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..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 @@ -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,22 +416,36 @@ 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.isGoodBrooklynTypeColonVersion(inlineRef) || RegisteredTypeNaming.isValidOsgiTypeColonVersion(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; 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/catalog/internal/CatalogUtils.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java index 47dab6d5f7..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 @@ -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,11 @@ 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. */ + // 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; int fi = versionedId.indexOf(VERSION_DELIMITER); @@ -248,16 +254,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); @@ -284,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; @@ -305,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/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 new file mode 100644 index 0000000000..1e54dcfa29 --- /dev/null +++ b/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java @@ -0,0 +1,112 @@ +/* + * 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.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 + * 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 + ")*"; + + 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 or forward slash or backslash. + * 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) { + 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); + } + + /** @see BrooklynVersionSyntax#isUsableVersion(String) */ + public static boolean isUsableVersion(String candidate) { + return BrooklynVersionSyntax.isUsableVersion(candidate); + } + + /** @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 + * 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 #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 (!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/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java b/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java index f6079398c0..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. @@ -253,6 +254,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 +274,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); @@ -295,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/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 0ca2171bed..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 @@ -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,22 @@ 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) { + @SuppressWarnings("deprecation") + private static void assertLegacyVersionParsesAs(String versionedId, String id, String version) { if (version==null) { Assert.assertFalse(CatalogUtils.looksLikeVersionedId(versionedId)); } else { 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..3fc8dc3d24 --- /dev/null +++ b/core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypeNamingTest.java @@ -0,0 +1,92 @@ +/* + * 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.apache.brooklyn.util.osgi.OsgiUtils; +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, 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 testNameColonVersion() { + 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, false); + assertNameColonVersion(null, false, false, false); + assertNameColonVersion("foo:", false, false, false); + assertNameColonVersion(":1", false, false, 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 isOsgi, boolean isGood) { + Assert.assertEquals(RegisteredTypeNaming.isUsableVersion(candidate), isUsable, "usable 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 isOsgi, boolean isGood) { + Assert.assertEquals(RegisteredTypeNaming.isUsableTypeColonVersion(candidate), isUsable, "usable name:version '"+candidate+"'"); + Assert.assertEquals(RegisteredTypeNaming.isValidOsgiTypeColonVersion(candidate), isOsgi, "osgi name:version '"+candidate+"'"); + Assert.assertEquals(RegisteredTypeNaming.isGoodBrooklynTypeColonVersion(candidate), isGood, "good name:version '"+candidate+"'"); + } + +} 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/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); } 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 600c90f968..66467a10cd 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 @@ -22,7 +22,6 @@ import static org.apache.brooklyn.rest.util.WebResourceUtils.notFound; import java.util.ArrayList; -import java.util.Collection; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -33,7 +32,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; @@ -46,9 +44,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; @@ -57,7 +52,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; @@ -75,9 +69,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; @@ -215,20 +207,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( @@ -253,50 +231,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(); - } } public Application create(ApplicationSpec spec) { 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); 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 9b176712de..57a18ab4c6 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 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 new file mode 100644 index 0000000000..e16dcf887c --- /dev/null +++ b/utils/common/src/main/java/org/apache/brooklyn/util/text/BrooklynVersionSyntax.java @@ -0,0 +1,189 @@ +/* + * 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 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 tests for examples, {@link VersionComparator} for more notes, and its tests for more examples. + */ +public class BrooklynVersionSyntax { + + public static final String USABLE_REGEX = "[^:\\s/\\\\]+"; + public static final 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 NUMBER = "[0-9]+"; + public final static String QUALIFIER = OSGI_TOKEN_REGEX; + + public final static String VALID_OSGI_VERSION_REGEX = + NUMBER + + "(" + DOT + NUMBER + + "(" + DOT + NUMBER + + "(" + DOT + QUALIFIER + + ")?" + + ")?" + + ")?"; + + public final static String GOOD_BROOKLYN_VERSION_REGEX = + NUMBER + + "(" + DOT + NUMBER + + "(" + DOT + 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 + * (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) { + 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 + * (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); + } + + /** 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 + "(" + DOT + NUMBER + "(" + DOT + 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/main/java/org/apache/brooklyn/util/text/NaturalOrderComparator.java b/utils/common/src/main/java/org/apache/brooklyn/util/text/NaturalOrderComparator.java index 7cdf42181e..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 @@ -53,17 +53,26 @@ /** 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: + * 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: * 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 +149,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..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 @@ -18,27 +18,27 @@ */ 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; /** * {@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"). + * 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. + * 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. */ @@ -56,149 +56,68 @@ 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); - @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); + TwoBooleans snapshots = TwoBooleans.of(isSnapshot(v1), isSnapshot(v2)); + if (snapshots.different()) return snapshots.compare(true); + + String u1 = versionWithoutQualifier(v1); + String u2 = versionWithoutQualifier(v2); + int uq = NaturalOrderComparator.INSTANCE.compare(u1, u2); + if (uq!=0) return uq; - 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; - } - } + // 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); - @VisibleForTesting - static String[] splitOnNonWordChar(String v) { - Collection parts = new ArrayList(); - String remaining = v; + // 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; - // 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()]); + // 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); } - @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; + 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) : ""; } - @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/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"); + } +} 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")); 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..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() { @@ -56,40 +35,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 +101,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)); + } + }