Clean up handling of lat/long geo-coordinates.
Change-Id: I64fca56c7deb9a8baa6c68558365ec2a8c38168c
diff --git a/core/api/src/main/java/org/onosproject/net/config/basics/BasicElementConfig.java b/core/api/src/main/java/org/onosproject/net/config/basics/BasicElementConfig.java
index 3033b3c..74257e6 100644
--- a/core/api/src/main/java/org/onosproject/net/config/basics/BasicElementConfig.java
+++ b/core/api/src/main/java/org/onosproject/net/config/basics/BasicElementConfig.java
@@ -30,12 +30,13 @@
protected static final String RACK_ADDRESS = "rackAddress";
protected static final String OWNER = "owner";
- protected static final double DEFAULT_COORD = -1.0;
+ protected static final double ZERO_THRESHOLD = Double.MIN_VALUE * 2.0;
+ private static final double DEFAULT_COORD = 0.0;
/**
* Returns friendly label for the element.
*
- * @return friendly label or element id itself if not set
+ * @return friendly label or element identifier itself if not set
*/
public String name() {
return get(NAME, subject.toString());
@@ -51,10 +52,25 @@
return (BasicElementConfig) setOrClear(NAME, name);
}
+ private static boolean doubleIsZero(double value) {
+ return value >= -ZERO_THRESHOLD && value <= ZERO_THRESHOLD;
+ }
+
+ /**
+ * Returns true if the geographical coordinates (latitude and longitude)
+ * are set on this element.
+ *
+ * @return true if geo-coordinates are set
+ */
+ public boolean geoCoordsSet() {
+ return !doubleIsZero(latitude()) || !doubleIsZero(longitude());
+ }
+
/**
* Returns element latitude.
*
- * @return element latitude; -1 if not set
+ * @return element latitude; 0.0 if (possibly) not set
+ * @see #geoCoordsSet()
*/
public double latitude() {
return get(LATITUDE, DEFAULT_COORD);
@@ -71,9 +87,10 @@
}
/**
- * Returns element latitude.
+ * Returns element longitude.
*
- * @return element latitude; -1 if not set
+ * @return element longitude; 0 if (possibly) not set
+ * @see #geoCoordsSet()
*/
public double longitude() {
return get(LONGITUDE, DEFAULT_COORD);
diff --git a/core/api/src/test/java/org/onosproject/net/config/basics/BasicElementConfigTest.java b/core/api/src/test/java/org/onosproject/net/config/basics/BasicElementConfigTest.java
new file mode 100644
index 0000000..63040af
--- /dev/null
+++ b/core/api/src/test/java/org/onosproject/net/config/basics/BasicElementConfigTest.java
@@ -0,0 +1,98 @@
+/*
+ * Copyright 2016-present Open Networking Laboratory
+ *
+ * Licensed 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.onosproject.net.config.basics;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.onosproject.net.config.basics.BasicElementConfig.ZERO_THRESHOLD;
+
+/**
+ * Unit tests for {@link BasicElementConfig}.
+ */
+public class BasicElementConfigTest {
+
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+
+ private static final String E1 = "e1";
+
+ // concrete subclass of abstract class we are testing
+ private static class ElmCfg extends BasicElementConfig<String> {
+ ElmCfg() {
+ object = MAPPER.createObjectNode();
+ }
+
+ @Override
+ public String toString() {
+ return object.toString();
+ }
+ }
+
+ private static void print(String fmt, Object... args) {
+ System.out.println(String.format(fmt, args));
+ }
+
+ private static void print(Object o) {
+ print("%s", o);
+ }
+
+ private BasicElementConfig<?> cfg;
+
+ @Before
+ public void setUp() {
+ cfg = new ElmCfg().name(E1);
+ }
+
+ @Test
+ public void basicNoGeo() {
+ print(cfg);
+ assertFalse("geo set?", cfg.geoCoordsSet());
+ assertEquals("lat", 0.0, cfg.latitude(), ZERO_THRESHOLD);
+ assertEquals("lon", 0.0, cfg.longitude(), ZERO_THRESHOLD);
+ }
+
+ @Test
+ public void geoLatitudeOnly() {
+ cfg.latitude(0.1);
+ print(cfg);
+ assertTrue("geo NOT set", cfg.geoCoordsSet());
+ assertEquals("lat", 0.1, cfg.latitude(), ZERO_THRESHOLD);
+ assertEquals("lon", 0.0, cfg.longitude(), ZERO_THRESHOLD);
+ }
+
+ @Test
+ public void geoLongitudeOnly() {
+ cfg.longitude(-0.1);
+ print(cfg);
+ assertTrue("geo NOT set", cfg.geoCoordsSet());
+ assertEquals("lat", 0.0, cfg.latitude(), ZERO_THRESHOLD);
+ assertEquals("lon", -0.1, cfg.longitude(), ZERO_THRESHOLD);
+ }
+
+ @Test
+ public void geoLatLong() {
+ cfg.latitude(3.1415).longitude(2.71828);
+ print(cfg);
+ assertTrue("geo NOT set", cfg.geoCoordsSet());
+ assertEquals("lat", 3.1415, cfg.latitude(), ZERO_THRESHOLD);
+ assertEquals("lon", 2.71828, cfg.longitude(), ZERO_THRESHOLD);
+ }
+}
diff --git a/core/net/src/main/java/org/onosproject/net/host/impl/BasicHostOperator.java b/core/net/src/main/java/org/onosproject/net/host/impl/BasicHostOperator.java
index 9645c5f..255ee46 100644
--- a/core/net/src/main/java/org/onosproject/net/host/impl/BasicHostOperator.java
+++ b/core/net/src/main/java/org/onosproject/net/host/impl/BasicHostOperator.java
@@ -34,8 +34,6 @@
*/
public final class BasicHostOperator implements ConfigOperator {
- protected static final double DEFAULT_COORD = -1.0;
-
private BasicHostOperator() {
}
@@ -81,10 +79,8 @@
if (cfg.name() != null) {
newBuilder.set(AnnotationKeys.NAME, cfg.name());
}
- if (cfg.latitude() != DEFAULT_COORD) {
+ if (cfg.geoCoordsSet()) {
newBuilder.set(AnnotationKeys.LATITUDE, Double.toString(cfg.latitude()));
- }
- if (cfg.longitude() != DEFAULT_COORD) {
newBuilder.set(AnnotationKeys.LONGITUDE, Double.toString(cfg.longitude()));
}
if (cfg.rackAddress() != null) {
diff --git a/core/net/src/test/java/org/onosproject/net/host/impl/BasicHostOperatorTest.java b/core/net/src/test/java/org/onosproject/net/host/impl/BasicHostOperatorTest.java
index caf36d1..3d1331a 100644
--- a/core/net/src/test/java/org/onosproject/net/host/impl/BasicHostOperatorTest.java
+++ b/core/net/src/test/java/org/onosproject/net/host/impl/BasicHostOperatorTest.java
@@ -54,18 +54,20 @@
private static final BasicHostConfig BHC = new BasicHostConfig();
private static final String NAME = "testhost";
private static final double LAT = 40.96;
+ private static final double LON = 0.0;
@Before
public void setUp() {
BHC.init(ID, "test", JsonNodeFactory.instance.objectNode(), mapper, delegate);
BHC.name(NAME).latitude(40.96);
+ // if you set lat or long, the other becomes valid as 0.0 (not null)
}
@Test
public void testDescOps() {
HostDescription desc = BasicHostOperator.combine(BHC, HOST);
assertEquals(NAME, desc.annotations().value(AnnotationKeys.NAME));
- assertEquals(null, desc.annotations().value(AnnotationKeys.LONGITUDE));
+ assertEquals(String.valueOf(LON), desc.annotations().value(AnnotationKeys.LONGITUDE));
assertEquals(String.valueOf(LAT), desc.annotations().value(AnnotationKeys.LATITUDE));
}
}
diff --git a/tools/test/topos/regions-topo-2 b/tools/test/topos/regions-topo-2
index befed3b..297bc65 100755
--- a/tools/test/topos/regions-topo-2
+++ b/tools/test/topos/regions-topo-2
@@ -7,12 +7,15 @@
onos ${host} wipe-out please
onos ${host} null-simulation start custom
+# null-create-device <type> <name> <#ports> <latitude> <longitude>
+# null-create-link <type> <src> <dst>
+
onos ${host} <<-EOF
-null-create-device switch s1 10 0 0
-null-create-device switch s2 10 0 0
-null-create-device switch s3 10 0 0
-null-create-device switch s4 10 0 0
+null-create-device switch s1-Bristol 10 51.4500 -2.5833
+null-create-device switch s2-London 10 51.5072 -0.1275
+null-create-device switch s3-Dover 10 51.1295 1.3089
+null-create-device switch s4-Brighton 10 50.8429 -0.1313
null-create-device switch s5 10 0 0
null-create-device switch s6 10 0 0
null-create-device switch s7 10 0 0
@@ -20,11 +23,11 @@
null-create-device switch s9 10 0 0
# null-create-device switch s10 10 0 0
-null-create-link direct s1 s2
-null-create-link direct s2 s3
-null-create-link direct s2 s4
-null-create-link direct s3 s4
-null-create-link direct s3 s5
+null-create-link direct s1-Bristol s2-London
+null-create-link direct s2-London s3-Dover
+null-create-link direct s2-London s4-Brighton
+null-create-link direct s3-Dover s4-Brighton
+null-create-link direct s3-Dover s5
null-create-link direct s6 s5
null-create-link direct s6 s8
null-create-link direct s7 s9
diff --git a/web/gui/src/main/java/org/onosproject/ui/impl/TopologyViewMessageHandlerBase.java b/web/gui/src/main/java/org/onosproject/ui/impl/TopologyViewMessageHandlerBase.java
index 872a099..985cb17 100644
--- a/web/gui/src/main/java/org/onosproject/ui/impl/TopologyViewMessageHandlerBase.java
+++ b/web/gui/src/main/java/org/onosproject/ui/impl/TopologyViewMessageHandlerBase.java
@@ -31,7 +31,6 @@
import org.onosproject.incubator.net.tunnel.Tunnel;
import org.onosproject.incubator.net.tunnel.TunnelService;
import org.onosproject.mastership.MastershipService;
-import org.onosproject.net.ElementId;
import org.onosproject.net.Annotated;
import org.onosproject.net.AnnotationKeys;
import org.onosproject.net.Annotations;
@@ -40,6 +39,7 @@
import org.onosproject.net.Device;
import org.onosproject.net.DeviceId;
import org.onosproject.net.EdgeLink;
+import org.onosproject.net.ElementId;
import org.onosproject.net.Host;
import org.onosproject.net.HostId;
import org.onosproject.net.HostLocation;
@@ -77,8 +77,8 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
-import java.util.Set;
import java.util.Optional;
+import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import static com.google.common.base.Preconditions.checkNotNull;
@@ -94,6 +94,8 @@
*/
public abstract class TopologyViewMessageHandlerBase extends UiMessageHandler {
+ private static final String NO_GEO_VALUE = "0.0";
+
// default to an "add" event...
private static final DefaultHashMap<ClusterEvent.Type, String> CLUSTER_EVENT =
new DefaultHashMap<>("addInstance");
@@ -361,10 +363,10 @@
String slng = annotations.value(AnnotationKeys.LONGITUDE);
String slat = annotations.value(AnnotationKeys.LATITUDE);
- boolean haveLng = slng != null && !slng.isEmpty();
- boolean haveLat = slat != null && !slat.isEmpty();
- try {
- if (haveLng && haveLat) {
+ boolean validLng = slng != null && !slng.equals(NO_GEO_VALUE);
+ boolean validLat = slat != null && !slat.equals(NO_GEO_VALUE);
+ if (validLat && validLng) {
+ try {
double lng = Double.parseDouble(slng);
double lat = Double.parseDouble(slat);
ObjectNode loc = objectNode()
@@ -372,11 +374,9 @@
.put("lng", lng)
.put("lat", lat);
payload.set("location", loc);
- } else {
- log.trace("missing Lng/Lat: lng={}, lat={}", slng, slat);
+ } catch (NumberFormatException e) {
+ log.warn("Invalid geo data: longitude={}, latitude={}", slng, slat);
}
- } catch (NumberFormatException e) {
- log.warn("Invalid geo data: longitude={}, latitude={}", slng, slat);
}
}