make LocationRegistry be a facade to TypeRegistry for most things

    instead of the messy old way where catalog added/removed in the location 
registry;
    means it works for things added directly to TypeRegistry now (and those 
tests pass)

    one minor change to backwards compatibility, the internal 
LocationDefinition ID now contains the version.
    it isn't used and in other cases is a random string anyway. this will fix 
bugs where mulitple versions
    of the same location would result in just one being returned.


Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo
Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/c5af5495
Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/c5af5495
Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/c5af5495

Branch: refs/heads/master
Commit: c5af5495549829462f8658468897a5d064398838
Parents: b016e67
Author: Alex Heneveld <[email protected]>
Authored: Mon Jul 3 14:03:29 2017 +0100
Committer: Alex Heneveld <[email protected]>
Committed: Mon Jul 3 17:30:26 2017 +0100

----------------------------------------------------------------------
 .../brooklyn/api/location/LocationRegistry.java |  20 ++-
 .../catalog/CatalogOsgiYamlLocationTest.java    |  10 +-
 .../catalog/CatalogYamlLocationTest.java        |  21 ++-
 .../catalog/internal/BasicBrooklynCatalog.java  |  11 +-
 .../core/location/BasicLocationRegistry.java    | 168 ++++++++++---------
 .../core/typereg/BasicBrooklynTypeRegistry.java |  14 +-
 .../core/location/LocationManagementTest.java   |  10 +-
 .../core/location/LocationRegistryTest.java     |  13 +-
 .../command/support/CloudExplorerSupport.java   |   2 +-
 .../rest/resources/LocationResource.java        |   2 +-
 .../rest/resources/LocationResourceTest.java    |   4 +-
 11 files changed, 154 insertions(+), 121 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c5af5495/api/src/main/java/org/apache/brooklyn/api/location/LocationRegistry.java
----------------------------------------------------------------------
diff --git 
a/api/src/main/java/org/apache/brooklyn/api/location/LocationRegistry.java 
b/api/src/main/java/org/apache/brooklyn/api/location/LocationRegistry.java
index 7804afc..bbf6f52 100644
--- a/api/src/main/java/org/apache/brooklyn/api/location/LocationRegistry.java
+++ b/api/src/main/java/org/apache/brooklyn/api/location/LocationRegistry.java
@@ -27,6 +27,7 @@ import javax.annotation.Nullable;
 import org.apache.brooklyn.api.entity.Entity;
 import org.apache.brooklyn.api.entity.EntitySpec;
 import org.apache.brooklyn.api.mgmt.LocationManager;
+import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry;
 import org.apache.brooklyn.util.guava.Maybe;
 
 /**
@@ -37,19 +38,32 @@ import org.apache.brooklyn.util.guava.Maybe;
 public interface LocationRegistry {
 
     /** map of ID (possibly randomly generated) to the definition (spec, name, 
id, and props; 
-     * where spec is the spec as defined, for instance possibly another 
named:xxx location) */
+     * where spec is the spec as defined, for instance possibly another 
named:xxx location).
+     * optionally will also return all things from other sources where this 
can look up. */
+    public Map<String,LocationDefinition> getDefinedLocations(boolean 
includeThingsWeAreFacadeFor);
+    
+    /** @deprecated since 0.12.0 use {@link #getDefinedLocations(boolean)} 
passing <code>true</code>.
+     * some clients might only want the things actually defined here, which is 
cheaper. */
+    @Deprecated
     public Map<String,LocationDefinition> getDefinedLocations();
     
     /** returns a LocationDefinition given its ID (usually a random string), 
or null if none */
     public LocationDefinition getDefinedLocationById(String id);
     
     /** returns a LocationDefinition given its name (e.g. for named locations, 
supply the bit after the "named:" prefix), 
-     * or null if none */
+     * looking inthe {@link BrooklynTypeRegistry} for registered items, then 
in this list, or null if none */
     public LocationDefinition getDefinedLocationByName(String name);
 
-    /** adds or updates the given defined location */
+    /** as {@link #updateDefinedLocationNonPersisted(LocationDefinition)}
+     * @deprecated since 0.12.0 use {@link 
#updateDefinedLocationNonPersisted(LocationDefinition)};
+     * it's exactly the same, just the name makes it clear */
+    @Deprecated
     public void updateDefinedLocation(LocationDefinition l);
 
+    /** adds or updates the given defined location in this registry; note it 
is not persisted;
+     * callers should consider adding to the {@link BrooklynTypeRegistry} 
instead */
+    public void updateDefinedLocationNonPersisted(LocationDefinition l);
+
     /** removes the defined location from the registry (applications running 
there are unaffected) */
     public void removeDefinedLocation(String id);
 

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c5af5495/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlLocationTest.java
----------------------------------------------------------------------
diff --git 
a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlLocationTest.java
 
b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlLocationTest.java
index 94b2c50..d986a51 100644
--- 
a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlLocationTest.java
+++ 
b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlLocationTest.java
@@ -75,6 +75,14 @@ public class CatalogOsgiYamlLocationTest extends 
AbstractYamlTest {
         assertAdded(symbolicName, SIMPLE_LOCATION_TYPE);
         assertOsgi(symbolicName);
         removeAndAssert(symbolicName);
+        
+        // and do it again; the OSGi registry doesn't add it;
+        // it only works because the location registry initializes itself on 
first call
+        // by reading from the catalog
+        symbolicName = "my.catalog.location.id.load.2";
+        addCatalogLocation(symbolicName, SIMPLE_LOCATION_TYPE, 
getOsgiLibraries());
+        assertAdded(symbolicName, SIMPLE_LOCATION_TYPE);
+        assertOsgi(symbolicName);
     }
 
     @Test
@@ -103,7 +111,7 @@ public class CatalogOsgiYamlLocationTest extends 
AbstractYamlTest {
 
         // Item added to catalog should automatically be available in location 
registry
         LocationDefinition def = 
mgmt().getLocationRegistry().getDefinedLocationByName(symbolicName);
-        assertEquals(def.getId(), symbolicName);
+        assertEquals(def.getId(), symbolicName+":"+TEST_VERSION);
         assertEquals(def.getName(), symbolicName);
         
         LocationSpec<?> spec = mgmt().getTypeRegistry().createSpec(item, null, 
LocationSpec.class);

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c5af5495/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java
----------------------------------------------------------------------
diff --git 
a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java
 
b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java
index 026b2e8..80db3f3 100644
--- 
a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java
+++ 
b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java
@@ -95,7 +95,6 @@ public class CatalogYamlLocationTest extends AbstractYamlTest 
{
 
         // Item added to catalog should automatically be available in location 
registry
         LocationDefinition def = 
mgmt().getLocationRegistry().getDefinedLocationByName(symbolicName);
-        assertEquals(def.getId(), symbolicName);
         assertEquals(def.getName(), symbolicName);
         
         LocationSpec spec = mgmt().getTypeRegistry().createSpec(item, null, 
LocationSpec.class);
@@ -171,9 +170,9 @@ public class CatalogYamlLocationTest extends 
AbstractYamlTest {
         
         addCatalogItems(yaml);
 
-        Map<String, LocationDefinition> defs = 
mgmt().getLocationRegistry().getDefinedLocations();
-        LocationDefinition def1 = checkNotNull(defs.get("loc1"), "loc1 
missing; has %s", defs.keySet());
-        LocationDefinition def2 = checkNotNull(defs.get("loc2"), "loc2 
missing; has %s", defs.keySet());
+        Map<String, LocationDefinition> defs = 
mgmt().getLocationRegistry().getDefinedLocations(true);
+        LocationDefinition def1 = checkNotNull(defs.get("loc1:0.1.2"), "loc1 
missing; has %s", defs.keySet());
+        LocationDefinition def2 = checkNotNull(defs.get("loc2:0.1.2"), "loc2 
missing; has %s", defs.keySet());
         LocationSpec<? extends Location> spec1 = 
mgmt().getLocationRegistry().getLocationSpec(def1).get();
         LocationSpec<? extends Location> spec2 = 
mgmt().getLocationRegistry().getLocationSpec(def2).get();
         
@@ -234,15 +233,15 @@ public class CatalogYamlLocationTest extends 
AbstractYamlTest {
         addCatalogItems(yaml2);
         addCatalogItems(yaml3);
 
-        LocationDefinition def1 = 
mgmt().getLocationRegistry().getDefinedLocations().get("loc1");
+        LocationDefinition def1 = 
mgmt().getLocationRegistry().getDefinedLocationByName("loc1");
         LocationSpec<? extends Location> spec1 = 
mgmt().getLocationRegistry().getLocationSpec(def1).get();
         assertEquals(spec1.getDisplayName(), "My name in item metadata");
         
-        LocationDefinition def2 = 
mgmt().getLocationRegistry().getDefinedLocations().get("loc2");
+        LocationDefinition def2 = 
mgmt().getLocationRegistry().getDefinedLocationByName("loc2");
         LocationSpec<? extends Location> spec2 = 
mgmt().getLocationRegistry().getLocationSpec(def2).get();
         assertEquals(spec2.getDisplayName(), "My name in top-level metadata");
         
-        LocationDefinition def3b = 
mgmt().getLocationRegistry().getDefinedLocations().get("loc3b");
+        LocationDefinition def3b = 
mgmt().getLocationRegistry().getDefinedLocationByName("loc3b");
         LocationSpec<? extends Location> spec3b = 
mgmt().getLocationRegistry().getLocationSpec(def3b).get();
         assertEquals(spec3b.getDisplayName(), "My name within item 3b");
     }
@@ -261,7 +260,7 @@ public class CatalogYamlLocationTest extends 
AbstractYamlTest {
         
         addCatalogItems(yaml);
 
-        LocationDefinition def = 
mgmt().getLocationRegistry().getDefinedLocations().get("loc1");
+        LocationDefinition def = 
mgmt().getLocationRegistry().getDefinedLocationByName("loc1");
         LocationSpec<? extends Location> spec = 
mgmt().getLocationRegistry().getLocationSpec(def).get();
         assertEquals(spec.getDisplayName(), "My name in top-level");
     }
@@ -280,7 +279,7 @@ public class CatalogYamlLocationTest extends 
AbstractYamlTest {
         
         addCatalogItems(yaml);
 
-        LocationDefinition def = 
mgmt().getLocationRegistry().getDefinedLocations().get("loc1");
+        LocationDefinition def = 
mgmt().getLocationRegistry().getDefinedLocationByName("loc1");
         LocationSpec<? extends Location> spec = 
mgmt().getLocationRegistry().getLocationSpec(def).get();
         assertEquals(spec.getDisplayName(), "My name in item metadata");
     }
@@ -299,7 +298,7 @@ public class CatalogYamlLocationTest extends 
AbstractYamlTest {
         
         addCatalogItems(yaml);
 
-        LocationDefinition def = 
mgmt().getLocationRegistry().getDefinedLocations().get("loc1");
+        LocationDefinition def = 
mgmt().getLocationRegistry().getDefinedLocationByName("loc1");
         LocationSpec<? extends Location> spec = 
mgmt().getLocationRegistry().getLocationSpec(def).get();
         assertEquals(spec.getDisplayName(), "My name within item");
     }
@@ -403,7 +402,7 @@ public class CatalogYamlLocationTest extends 
AbstractYamlTest {
     }
     
     private void assertLocationRegistryCount(int size) {
-        
Asserts.assertThat(mgmt().getLocationRegistry().getDefinedLocations().keySet(), 
CollectionFunctionals.sizeEquals(size));
+        
Asserts.assertThat(mgmt().getLocationRegistry().getDefinedLocations(true).keySet(),
 CollectionFunctionals.sizeEquals(size));
     }
     private void assertLocationManagerInstancesCount(int size) {
         Asserts.assertThat(mgmt().getLocationManager().getLocations(), 
CollectionFunctionals.sizeEquals(size));

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c5af5495/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java
----------------------------------------------------------------------
diff --git 
a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java
 
b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java
index 0d858e0..40655bc 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
@@ -330,11 +330,6 @@ public class BasicBrooklynCatalog implements 
BrooklynCatalog {
         if (log.isTraceEnabled()) {
             log.trace("Scheduling item for persistence removal: {}", 
itemDto.getId());
         }
-        if (itemDto.getCatalogItemType() == CatalogItemType.LOCATION) {
-            @SuppressWarnings("unchecked")
-            CatalogItem<Location,LocationSpec<?>> locationItem = 
(CatalogItem<Location, LocationSpec<?>>) itemDto;
-            
((BasicLocationRegistry)mgmt.getLocationRegistry()).removeDefinedLocation(locationItem);
-        }
         mgmt.getRebindManager().getChangeListener().onUnmanaged(itemDto);
 
     }
@@ -1629,11 +1624,7 @@ public class BasicBrooklynCatalog implements 
BrooklynCatalog {
     }
 
     private void onAdditionUpdateOtherRegistries(CatalogItemDtoAbstract<?, ?> 
itemDto) {
-        if (itemDto.getCatalogItemType() == CatalogItemType.LOCATION) {
-            @SuppressWarnings("unchecked")
-            CatalogItem<Location,LocationSpec<?>> locationItem = 
(CatalogItem<Location, LocationSpec<?>>) itemDto;
-            
((BasicLocationRegistry)mgmt.getLocationRegistry()).updateDefinedLocation(locationItem);
-        }
+        // nothing needed (previously updated BasicLocationRegistry but now 
that is a facade)
     }
 
     /** returns item DTO if item is an allowed duplicate, or null if it should 
be added (there is no duplicate), 

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c5af5495/core/src/main/java/org/apache/brooklyn/core/location/BasicLocationRegistry.java
----------------------------------------------------------------------
diff --git 
a/core/src/main/java/org/apache/brooklyn/core/location/BasicLocationRegistry.java
 
b/core/src/main/java/org/apache/brooklyn/core/location/BasicLocationRegistry.java
index a761513..cbfd140 100644
--- 
a/core/src/main/java/org/apache/brooklyn/core/location/BasicLocationRegistry.java
+++ 
b/core/src/main/java/org/apache/brooklyn/core/location/BasicLocationRegistry.java
@@ -47,6 +47,7 @@ import org.apache.brooklyn.core.internal.BrooklynProperties;
 import org.apache.brooklyn.core.location.internal.LocationInternal;
 import org.apache.brooklyn.core.mgmt.internal.LocalLocationManager;
 import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
+import org.apache.brooklyn.core.typereg.RegisteredTypeLoadingContexts;
 import org.apache.brooklyn.core.typereg.RegisteredTypePredicates;
 import org.apache.brooklyn.location.localhost.LocalhostLocationResolver;
 import org.apache.brooklyn.util.collections.MutableList;
@@ -71,74 +72,46 @@ import com.google.common.collect.Sets;
 /**
  * See {@link LocationRegistry} for general description.
  * <p>
- * TODO The relationship between the catalog and the location registry is a 
bit messy.
- * For all existing code, the location registry is the definitive way to 
resolve
- * locations. 
+ * This implementation unfortunately has to deal with location definitions 
coming from three places:
+ * 
+ * <li> brooklyn.properties
+ * <li> {@link BrooklynCatalog}
+ * <li> {@link BrooklynTypeRegistry}
+ * <p> 
+ * The first is read on init in the call to {@link #updateDefinedLocations()}.
+ * The latter two are <i>not</i> loaded here but are treated as lookups to 
{@link BrooklynTypeRegistry}.
  * <p>
- * Any location item added to the catalog must therefore be registered here.
- * Whenever an item is added to the catalog, it will automatically call 
- * {@link #updateDefinedLocation(RegisteredType)}. Similarly, when a location
- * is deleted from the catalog it will call {@link 
#removeDefinedLocation(RegisteredType)}.
+ * (Prior to 0.12.0 items in {@link BrooklynCatalog} were scanned on init 
here, and then 
+ * it updated this on additions and removals there. That is no longer done 
with type registry.)
  * <p>
- * However, the location item in the catalog has an unparsed blob of YAML, 
which contains
- * important things like the type and the config of the location. This is only 
parsed when 
- * {@link BrooklynCatalog#createSpec(CatalogItem)} is called. We therefore 
jump through 
- * some hoops to wire together the catalog and the registry.
+ * We should consider deprecating this as a place where locations can be 
defined (switch to type registry only).
  * <p>
- * To add a location to the catalog, and then to resolve a location that is in 
the catalog, 
- * it goes through the following steps:
- * 
- * <ol>
- *   <li>Call {@link BrooklynCatalog#addItems(String)}
- *     <ol>
- *       <li>This automatically calls {@link 
#updateDefinedLocation(RegisteredType)}
- *       <li>A LocationDefinition is creating, using as its id the {@link 
RegisteredType#getSymbolicName()}.
- *           The definition's spec is {@code 
brooklyn.catalog:<symbolicName>:<version>},
- *     </ol>
- *   <li>A blueprint can reference the catalog item using its symbolic name, 
- *       such as the YAML {@code location: my-new-location}.
- *       (this feels similar to the "named locations").
- *     <ol>
- *       <li>This automatically calls {@link #getLocationManaged(String)}.
- *       <li>The LocationDefinition is found by lookig up this name.
- *       <li>The {@link LocationDefiniton.getSpec()} is retrieved; the right 
{@link LocationResolver} is
- *           found for it.
- *       <li>This uses the {@link CatalogLocationResolver}, because the spec 
starts with {@code brooklyn.catalog:}.
- *       <li>This resolver extracts from the spec the 
<symobolicName>:<version>, and looks up the 
- *           location item using the {@link BrooklynTypeRegistry}.
- *       <li>It then creates a {@link LocationSpec} by calling {@link 
BrooklynTypeRegistry#createSpec(RegisteredType)}.
- *         <ol>
- *           <li>This first tries to use the type (that is in the YAML) as a 
simple Java class.
- *           <li>If that fails, it will resolve the type using {@link 
#resolve(String, Boolean, Map)}, which
- *               returns an actual location object.
- *           <li>It extracts from that location object the appropriate 
metadata to create a {@link LocationSpec},
- *               returns the spec and discards the location object.
- *         </ol>
- *       <li>The resolver creates the {@link Location} from the {@link 
LocationSpec}
- *     </ol>
- * </ol>
- * 
- * TODO we should change the registry to be a pass-through facade on top of 
the catalog,
- * and shift to preferring catalog access mechanisms.
- * this brings it in line with how we do other things;
- * also this does not understand versions.
- * to do this we will need to:
- * <li> update the catalog on addition, setting a plan (ensuring 
serialization);
- *      in case of a definition CHANGED in brooklyn.properties give an error 
if it means the plan has changed
- *      (user could then remove from brooklyn.properties, if persistence on, 
or apply the update in the catalog;
- *      ie similar semantics to defining an initial catalog via the CLI)
- * <li> find and return the RegisteredType from the type-registry/catalog here
+ * However this is used for items from brooklyn.properties -- we could create 
a bundle to store those
+ * to help with migration, and then deprecate/block changes to those after the 
bundle is created. 
+ * However properties are the only way currently to set default data used for 
various clouds; 
+ * either we'd need to deprecate that also, or find a way to support that in a 
bundled world.
+ * <p>
+ * This is also used in a few other places, such as clocker and the server 
pool entity.
+ * Things here are never persisted though, so it is those callers' 
responsibility to recreate.
+ * Alternatively (better) would be for them to add items to the catalog 
instead of here. 
+ * <p>
+ * Or we keep this lying around for now. Probably not worth the pain above.
  * <p>
- * Once done, update the UI use /v1/catalog/locations instead of /v1/locations
- * (currently the latter is the only way to list locations known in the 
LocationRegistry 
- * ie those from brookln.properties.)
+ * Also note the location item in the catalog has an unparsed blob of YAML, 
which contains
+ * important things like the type and the config of the location. This is only 
parsed when 
+ * {@link BrooklynTypeRegistry#createSpec(RegisteredType)} is called, so we do 
that for
+ * lookup possibly more often than one might expect. (If a time hole this 
could be cached.)
+ * We also need to make sure some of the resolvers here can be used to 
identify types, so
+ * when loading types, the spec creation routines are able to call this to 
+ * {@link #resolve(String, Boolean, Map)} (that returns a {@link Location} not 
a spec,
+ * but the {@link Location} instance is discarded).
+ * <p>
+ * Finally note /v1/catalog/locations does not list items stored here; 
+ * only /v1/locations does that.
  */
 @SuppressWarnings({"rawtypes","unchecked"})
 public class BasicLocationRegistry implements LocationRegistry {
 
-    // TODO save / serialize
-    // (we persist live locations, ie those in the LocationManager, but not 
"catalog" locations, ie those in this Registry)
-    
     public static final Logger log = 
LoggerFactory.getLogger(BasicLocationRegistry.class);
 
     /**
@@ -194,29 +167,68 @@ public class BasicLocationRegistry implements 
LocationRegistry {
     }
     
     @Override
-    public Map<String,LocationDefinition> getDefinedLocations() {
+    public Map<String, LocationDefinition> getDefinedLocations(boolean 
includeThingsWeAreFacadeFor) {
+        Map<String, LocationDefinition> result = MutableMap.of();
         synchronized (definedLocations) {
-            return 
ImmutableMap.<String,LocationDefinition>copyOf(definedLocations);
+            result.putAll(definedLocations);
+        }
+        for (RegisteredType rt: 
mgmt.getTypeRegistry().getMatching(RegisteredTypePredicates.IS_LOCATION)) {
+            result.put(rt.getId(), newDefinedLocation(rt));
         }
+        return result;
+    }
+    
+    @Override @Deprecated
+    public Map<String,LocationDefinition> getDefinedLocations() {
+        return getDefinedLocations(true);
     }
     
     @Override
     public LocationDefinition getDefinedLocationById(String id) {
-        return definedLocations.get(id);
+        return getDefinedLocation(id, true);
     }
 
     @Override
     public LocationDefinition getDefinedLocationByName(String name) {
+        return getDefinedLocation(name, false);
+    }
+    
+    private LocationDefinition getDefinedLocation(String nameOrId, boolean 
isId) {
+        RegisteredType lt = mgmt.getTypeRegistry().get(nameOrId, 
RegisteredTypeLoadingContexts.withSpecSuperType(null, LocationSpec.class));
+        if (lt!=null) {
+            return newDefinedLocation(lt);
+        }
+        
         synchronized (definedLocations) {
-            for (LocationDefinition l: definedLocations.values()) {
-                if (l.getName().equals(name)) return l;
+            if (isId) {
+                LocationDefinition ld = definedLocations.get(nameOrId);
+                if (ld!=null) {
+                    return ld;
+                }
+            } else {
+                for (LocationDefinition l: definedLocations.values()) {
+                    if (l.getName().equals(nameOrId)) return l;
+                }
             }
-            return null;
         }
+        
+        // fall back to ignoring supertypes, in case they weren't set
+        lt = mgmt.getTypeRegistry().get(nameOrId);
+        if (lt!=null) {
+            log.warn("Location registry only found "+nameOrId+" when ignoring 
supertypes; check it is correctly resolved.");
+            return newDefinedLocation(lt);
+        }
+        
+        return null;
     }
 
-    @Override
+    @Override @Deprecated
     public void updateDefinedLocation(LocationDefinition l) {
+        updateDefinedLocationNonPersisted(l);
+    }
+    
+    @Override
+    public void updateDefinedLocationNonPersisted(LocationDefinition l) {
         synchronized (definedLocations) { 
             definedLocations.put(l.getId(), l); 
         }
@@ -226,7 +238,9 @@ public class BasicLocationRegistry implements 
LocationRegistry {
      * Converts the given item from the catalog into a LocationDefinition, and 
adds it
      * to the registry (overwriting anything already registered with the id
      * {@link CatalogItem#getCatalogItemId()}.
+     * @deprecated since 0.12.0 this class is a facade so method no longer 
wanted
      */
+    @Deprecated  // not used
     public void updateDefinedLocation(CatalogItem<Location, LocationSpec<?>> 
item) {
         String id = item.getCatalogItemId();
         String symbolicName = item.getSymbolicName();
@@ -241,17 +255,22 @@ public class BasicLocationRegistry implements 
LocationRegistry {
      * Converts the given item from the catalog into a LocationDefinition, and 
adds it
      * to the registry (overwriting anything already registered with the id
      * {@link RegisteredType#getId()}.
+     * @deprecated since 0.12.0 this class is a facade so method no longer 
wanted
      */
+    @Deprecated  // not used
     public void updateDefinedLocation(RegisteredType item) {
+        BasicLocationDefinition locDefinition = newDefinedLocation(item);
+        updateDefinedLocation(locDefinition);
+    }
+
+    protected BasicLocationDefinition newDefinedLocation(RegisteredType item) {
         String id = item.getId();
-        String symbolicName = item.getSymbolicName();
         String spec = CatalogLocationResolver.createLegacyWrappedReference(id);
-        Map<String, Object> config = ImmutableMap.<String, Object>of();
-        BasicLocationDefinition locDefinition = new 
BasicLocationDefinition(symbolicName, symbolicName, spec, config);
-        
-        updateDefinedLocation(locDefinition);
+        return new BasicLocationDefinition(id, item.getSymbolicName(), spec, 
ImmutableMap.<String, Object>of());
     }
 
+    /** @deprecated since 0.12.0 this class is a facade so method no longer 
wanted */
+    @Deprecated  // not used
     public void removeDefinedLocation(CatalogItem<Location, LocationSpec<?>> 
item) {
         removeDefinedLocation(item.getSymbolicName());
     }
@@ -290,11 +309,6 @@ public class BasicLocationRegistry implements 
LocationRegistry {
             }
             if (log.isDebugEnabled())
                 log.debug("Found "+count+" defined locations from properties 
(*.named.* syntax): "+definedLocations.values());
-            
-            for (RegisteredType item: 
mgmt.getTypeRegistry().getMatching(RegisteredTypePredicates.IS_LOCATION)) {
-                updateDefinedLocation(item);
-                count++;
-            }
         }
     }
     

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c5af5495/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java
----------------------------------------------------------------------
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 f7f71e5..f1a3e66 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
@@ -19,6 +19,7 @@
 package org.apache.brooklyn.core.typereg;
 
 import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.Objects;
 import java.util.Set;
 
@@ -28,6 +29,7 @@ import org.apache.brooklyn.api.catalog.BrooklynCatalog;
 import org.apache.brooklyn.api.catalog.CatalogItem;
 import org.apache.brooklyn.api.catalog.CatalogItem.CatalogItemType;
 import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec;
+import org.apache.brooklyn.api.location.Location;
 import org.apache.brooklyn.api.mgmt.ManagementContext;
 import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry;
 import org.apache.brooklyn.api.typereg.RegisteredType;
@@ -35,7 +37,9 @@ import 
org.apache.brooklyn.api.typereg.RegisteredType.TypeImplementationPlan;
 import org.apache.brooklyn.api.typereg.RegisteredTypeLoadingContext;
 import org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog;
 import org.apache.brooklyn.core.catalog.internal.CatalogItemBuilder;
+import org.apache.brooklyn.core.catalog.internal.CatalogItemDtoAbstract;
 import org.apache.brooklyn.core.catalog.internal.CatalogUtils;
+import org.apache.brooklyn.core.location.BasicLocationRegistry;
 import org.apache.brooklyn.core.mgmt.ha.OsgiManager;
 import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal;
 import org.apache.brooklyn.test.Asserts;
@@ -419,14 +423,18 @@ public class BasicBrooklynTypeRegistry implements 
BrooklynTypeRegistry {
 
     @Beta // API stabilising
     public void delete(VersionedName type) {
-        if (localRegisteredTypes.remove(type.toString()) != null) {
+        RegisteredType registeredTypeRemoved = 
localRegisteredTypes.remove(type.toString());
+        if (registeredTypeRemoved != null) {
             return ;
         }
-        // TODO may need to support version-less here?
         
         // legacy deletion (may call back to us, but max once)
         mgmt.getCatalog().deleteCatalogItem(type.getSymbolicName(), 
type.getVersionString());
-        // if nothing deleted, throw NoSuchElement
+        // currently the above will succeed or throw; but if we delete that, 
we need to enable the code below
+//        if (Strings.isBlank(type.getVersionString()) || 
BrooklynCatalog.DEFAULT_VERSION.equals(type.getVersionString())) {
+//            throw new IllegalStateException("Deleting items with unspecified 
version (argument DEFAULT_VERSION) not supported.");
+//        }
+//        throw new NoSuchElementException("No catalog item found with id 
"+type);
     }
     
     public void delete(RegisteredType type) {

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c5af5495/core/src/test/java/org/apache/brooklyn/core/location/LocationManagementTest.java
----------------------------------------------------------------------
diff --git 
a/core/src/test/java/org/apache/brooklyn/core/location/LocationManagementTest.java
 
b/core/src/test/java/org/apache/brooklyn/core/location/LocationManagementTest.java
index ed6758a..da85a9e 100644
--- 
a/core/src/test/java/org/apache/brooklyn/core/location/LocationManagementTest.java
+++ 
b/core/src/test/java/org/apache/brooklyn/core/location/LocationManagementTest.java
@@ -106,23 +106,23 @@ public class LocationManagementTest extends 
BrooklynAppUnitTestSupport {
 
     @Test
     public void testManagedLocationsNamedCreateAndCleanup() {
-        
Asserts.assertThat(mgmt.getLocationRegistry().getDefinedLocations().keySet(), 
CollectionFunctionals.sizeEquals(0));
+        
Asserts.assertThat(mgmt.getLocationRegistry().getDefinedLocations(true).keySet(),
 CollectionFunctionals.sizeEquals(0));
         Asserts.assertThat(mgmt.getCatalog().getCatalogItems(), 
CollectionFunctionals.sizeEquals(0));
         Asserts.assertThat(locationManager.getLocations(), 
CollectionFunctionals.sizeEquals(0));
         
-        mgmt.getLocationRegistry().updateDefinedLocation( new 
BasicLocationDefinition("lh1", "localhost", null) );
+        mgmt.getLocationRegistry().updateDefinedLocationNonPersisted( new 
BasicLocationDefinition("lh1", "localhost", null) );
         
-        
Asserts.assertThat(mgmt.getLocationRegistry().getDefinedLocations().keySet(), 
CollectionFunctionals.sizeEquals(1));
+        
Asserts.assertThat(mgmt.getLocationRegistry().getDefinedLocations(true).keySet(),
 CollectionFunctionals.sizeEquals(1));
         Asserts.assertThat(locationManager.getLocations(), 
CollectionFunctionals.sizeEquals(0));
         // currently such defined locations do NOT appear in catalog -- see 
CatalogYamlLocationTest
         Asserts.assertThat(mgmt.getCatalog().getCatalogItems(), 
CollectionFunctionals.sizeEquals(0));
         
         Location loc = mgmt.getLocationRegistry().getLocationManaged("lh1");
-        
Asserts.assertThat(mgmt.getLocationRegistry().getDefinedLocations().keySet(), 
CollectionFunctionals.sizeEquals(1));
+        
Asserts.assertThat(mgmt.getLocationRegistry().getDefinedLocations(true).keySet(),
 CollectionFunctionals.sizeEquals(1));
         Asserts.assertThat(locationManager.getLocations(), 
CollectionFunctionals.sizeEquals(1));
         
         mgmt.getLocationManager().unmanage(loc);
-        
Asserts.assertThat(mgmt.getLocationRegistry().getDefinedLocations().keySet(), 
CollectionFunctionals.sizeEquals(1));
+        
Asserts.assertThat(mgmt.getLocationRegistry().getDefinedLocations(true).keySet(),
 CollectionFunctionals.sizeEquals(1));
         Asserts.assertThat(locationManager.getLocations(), 
CollectionFunctionals.sizeEquals(0));
     }
 

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c5af5495/core/src/test/java/org/apache/brooklyn/core/location/LocationRegistryTest.java
----------------------------------------------------------------------
diff --git 
a/core/src/test/java/org/apache/brooklyn/core/location/LocationRegistryTest.java
 
b/core/src/test/java/org/apache/brooklyn/core/location/LocationRegistryTest.java
index bd27f81..b9c8d5a 100644
--- 
a/core/src/test/java/org/apache/brooklyn/core/location/LocationRegistryTest.java
+++ 
b/core/src/test/java/org/apache/brooklyn/core/location/LocationRegistryTest.java
@@ -53,9 +53,9 @@ public class LocationRegistryTest {
         properties.put("brooklyn.location.named.foo", 
"byon:(hosts=\"[email protected].{1,2,3,4}\")");
         properties.put("brooklyn.location.named.foo.privateKeyFile", 
"~/.ssh/foo.id_rsa");
         mgmt = LocalManagementContextForTests.newInstance(properties);
-        log.info("foo properties gave defined locations: 
"+mgmt.getLocationRegistry().getDefinedLocations());
+        log.info("foo properties gave defined locations: 
"+mgmt.getLocationRegistry().getDefinedLocations(true));
         locdef = mgmt.getLocationRegistry().getDefinedLocationByName("foo");
-        Assert.assertNotNull(locdef, "Expected 'foo' location; but had 
"+mgmt.getLocationRegistry().getDefinedLocations());
+        Assert.assertNotNull(locdef, "Expected 'foo' location; but had 
"+mgmt.getLocationRegistry().getDefinedLocations(true));
         Assert.assertEquals(locdef.getConfig().get("privateKeyFile"), 
"~/.ssh/foo.id_rsa");
     }
     
@@ -67,7 +67,7 @@ public class LocationRegistryTest {
         mgmt = LocalManagementContextForTests.newInstance(properties);
 
         locdef = mgmt.getLocationRegistry().getDefinedLocationByName("foo");
-        log.info("testResovlesBy has defined locations: 
"+mgmt.getLocationRegistry().getDefinedLocations());
+        log.info("testResovlesBy has defined locations: 
"+mgmt.getLocationRegistry().getDefinedLocations(true));
         
         LocationSpec<?> ls = 
mgmt.getLocationRegistry().getLocationSpec("named:foo").get();
         Location l = mgmt.getLocationManager().createLocation(ls);
@@ -127,7 +127,7 @@ public class LocationRegistryTest {
         BrooklynProperties properties = BrooklynProperties.Factory.newEmpty();
         properties.put("brooklyn.location.named.bar", "named:bar");
         mgmt = LocalManagementContextForTests.newInstance(properties);
-        log.info("bar properties gave defined locations: 
"+mgmt.getLocationRegistry().getDefinedLocations());
+        log.info("bar properties gave defined locations: 
"+mgmt.getLocationRegistry().getDefinedLocations(true));
         try {
             mgmt.getLocationRegistry().getLocationSpec("bar").get();
             Asserts.shouldHaveFailedPreviously("Circular reference gave a 
location");
@@ -137,7 +137,7 @@ public class LocationRegistryTest {
     }
 
     protected boolean findLocationMatching(String regex) {
-        for (LocationDefinition d: 
mgmt.getLocationRegistry().getDefinedLocations().values()) {
+        for (LocationDefinition d: 
mgmt.getLocationRegistry().getDefinedLocations(true).values()) {
             if (d.getName()!=null && d.getName().matches(regex)) return true;
         }
         return false;
@@ -159,8 +159,7 @@ public class LocationRegistryTest {
         properties.put(LocalhostLocationResolver.LOCALHOST_ENABLED.getName(), 
false);
         mgmt = LocalManagementContextForTests.newInstance(properties);
         
-        log.info("RESOLVERS: 
"+mgmt.getLocationRegistry().getDefinedLocations());
-        log.info("DEFINED LOCATIONS: 
"+mgmt.getLocationRegistry().getDefinedLocations());
+        log.info("DEFINED LOCATIONS: 
"+mgmt.getLocationRegistry().getDefinedLocations(true));
         Assert.assertFalse( findLocationMatching("localhost") );
     }
 

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c5af5495/launcher-common/src/main/java/org/apache/brooklyn/launcher/command/support/CloudExplorerSupport.java
----------------------------------------------------------------------
diff --git 
a/launcher-common/src/main/java/org/apache/brooklyn/launcher/command/support/CloudExplorerSupport.java
 
b/launcher-common/src/main/java/org/apache/brooklyn/launcher/command/support/CloudExplorerSupport.java
index dc4000b..e35517d 100644
--- 
a/launcher-common/src/main/java/org/apache/brooklyn/launcher/command/support/CloudExplorerSupport.java
+++ 
b/launcher-common/src/main/java/org/apache/brooklyn/launcher/command/support/CloudExplorerSupport.java
@@ -104,7 +104,7 @@ public abstract class CloudExplorerSupport implements 
Callable<Void> {
             locs.add(loc);
         } else if (allLocations) {
             // Find all named locations that point at different target clouds
-            Map<String, LocationDefinition> definedLocations = 
managementContext.getLocationRegistry().getDefinedLocations();
+            Map<String, LocationDefinition> definedLocations = 
managementContext.getLocationRegistry().getDefinedLocations(true);
             for (LocationDefinition locationDef : definedLocations.values()) {
 
                 Location loc = 
managementContext.getLocationManager().createLocation(

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c5af5495/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/LocationResource.java
----------------------------------------------------------------------
diff --git 
a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/LocationResource.java
 
b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/LocationResource.java
index 55159fa..052114a 100644
--- 
a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/LocationResource.java
+++ 
b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/LocationResource.java
@@ -83,7 +83,7 @@ public class LocationResource extends 
AbstractBrooklynRestResource implements Lo
                 }
             }
         };
-        return 
FluentIterable.from(brooklyn().getLocationRegistry().getDefinedLocations().values())
+        return 
FluentIterable.from(brooklyn().getLocationRegistry().getDefinedLocations(true).values())
                 .transform(transformer)
                 .filter(LocationSummary.class)
                 .toSortedList(nameOrSpecComparator());

http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/c5af5495/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/LocationResourceTest.java
----------------------------------------------------------------------
diff --git 
a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/LocationResourceTest.java
 
b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/LocationResourceTest.java
index c61c395..d4a484d 100644
--- 
a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/LocationResourceTest.java
+++ 
b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/LocationResourceTest.java
@@ -180,7 +180,7 @@ public class LocationResourceTest extends 
BrooklynRestResourceTest {
     @Test(dependsOnMethods = { "testAddLegacyLocationDefinition" })
     @Deprecated
     public void testDeleteLocation() {
-        final int size = getLocationRegistry().getDefinedLocations().size();
+        final int size = 
getLocationRegistry().getDefinedLocations(true).size();
         URI expectedLocationUri = URI.create("/locations/"+legacyLocationName);
 
         Response response = client().path(expectedLocationUri).delete();
@@ -188,7 +188,7 @@ public class LocationResourceTest extends 
BrooklynRestResourceTest {
         Asserts.succeedsEventually(new Runnable() {
             @Override
             public void run() {
-                
assertEquals(getLocationRegistry().getDefinedLocations().size(), size - 1);
+                
assertEquals(getLocationRegistry().getDefinedLocations(true).size(), size - 1);
             }
         });
     }

Reply via email to