API clean up #2

CHANGELOG.md

@@ -28,13 +28,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   [#92](https://github.com/scionproto-contrib/jpan/pull/92)
 - **BREAKING CHANGE**: Path metadata has been moved to `PathMetadata`.
   [#93](https://github.com/scionproto-contrib/jpan/pull/93)
-- **BREAKING CHANGE**: `receive()` returns `ScionSocketAddress`; `RequestAddress` and 
-  `RequestPath` are removed from from public API.
+- **BREAKING CHANGE**: `receive()` returns `ScionSocketAddress`; `ResponseAddress` and 
+  `ResponsePath` are removed from from public API.
   [#94](https://github.com/scionproto-contrib/jpan/pull/94)
+- **BREAKING CHANGE**: removed `RequestPath` and `ScionAddress` from public API.
+  [#95](https://github.com/scionproto-contrib/jpan/pull/95)
 
 ### Fixed
 - Fixed locking and resizing of buffers. [#68](https://github.com/scionproto-contrib/jpan/pull/68)
-- Fixed unhelpful error & log message when with topofile has wrong permissions.
+- Fixed unhelpful error & log message when with topo file has wrong permissions.
   [#74](https://github.com/scionproto-contrib/jpan/issues/74)
 - Topology file parser support for new "local" attribute.
   [#72](https://github.com/scionproto-contrib/jpan/issues/72)

README.md

@@ -29,7 +29,6 @@ The following artifact contains the complete SCION Java implementation:
 
 ### Planned features
 - API: `Selector` for `DatagramChannel`
-- API: `Path` extends `InetSocketAddress`
 - Autodetection of NAT external IP
 - Path creation with short-cuts, on-path and peering routes
 - Improve docs, demos and testing
@@ -74,11 +73,13 @@ The central classes of the API are:
 - `DatagramChannel`: This class works like a `java.nio.channel.DatagramChannel`. It implements 
   `Channel` and `ByteChannel`. Scattering, gathering, multicast and selectors are currently not
   supported.
-- `Path`, `RequestPath`, `ResponsePath`: The notion of path is slightly different than in other 
-    parts of SCION. A `Path` contains a route to a destination ("raw path") plus the full 
-    destination, i.e. IP-address and port.
-  - `RequestPath` is a `Path` with meta information (bandwidth, geo info, etc).
-  - `ResponsePath` is a `Path` with source IA, IP & port.
+- `ScionSocketAddress` is an `InetSocketAddress` with the IP of a Scion enabled endhost.
+  A `ScionSocketAddress` also has the ISD/AS code of that endhost and a path to the that endhost. 
+- `Path` objects contain a route to a destination ("raw path") plus the full 
+    destination, i.e. SCION-enabled IP address and port. 
+  - If the path was created by the `ScionService` then it has `PathMetadata` with meta information 
+    (bandwidth, geo info, etc).
+  - A path returned by `receive()` (as part of a `ScionSocketAddress`) has no meta information.
 - `PathPolicy` is an interface with several example implementations for:
   first path returned by daemon (default), max bandwidth, min latency, min hops, ...
 - `ScionService`: Provides methods to request paths and get ISD/AS information.
@@ -213,6 +214,12 @@ use something like:
 InetSocketAddress serverAddress = new InetSocketAddress("your-domain.org", 80);
 channel.connect(serverAddress);
 ```
+or
+```java
+InetSocketAddress serverAddress = new InetSocketAddress("your-domain.org", 80);
+Path path = Scion.getDefaultService().lookupAndGetPath(serverAddress, PathPolicy.DEFAULT);
+channel.send(buffer, path);
+```
 
 Alternatively, the ISD/AS can be specified explicitly in several ways.
 
@@ -278,7 +285,7 @@ API beyond the standard Java `DatagramScoket`:
 
 * `create(ScionService)` and `create(SocketAddress, ScionService)` for creating a `DatagramSocket`
   with a non-default `ScionService`.
-* `connect(RequestPath path)` for connecting to a remote host
+* `connect(Path path)` for connecting to a remote host
 * `getConnectionPath()` gets the connected path if the socket has been connected 
 * `getCachedPath(InetAddress address)` get the cached path for a given IP
 * `setPathCacheCapacity(int capacity)` and `getPathCacheCapacity()` for managing the cache size
@@ -292,11 +299,11 @@ API beyond the standard Java `DatagramScoket`:
   multiple packets to the same destination, one should use `path = send(buffer, path)` or `connect()` + `write()` in 
   order to avoid frequent path lookups.
 
-- **Using expired path (client).** When using `send(buffer, path)` with an expired `RequestPath`, the channel will 
+- **Using expired path (client).** When using `send(buffer, path)` with an expired `Path`, the channel will 
   transparently look up a new path. This works but causes a path lookup for every `send()`.
   Solution: always use the latest path returned by send, e.g. `path = send(buffer, path)`.
 
-- **Using expired path (server).** When using `send(buffer, path)` with an expired `ResponsePath`, the channel will
+- **Using expired path (server).** When using `send(buffer, path)` with an expired `Path`, the channel will
   simple send it anyway.
 
 ## Configuration

doc/Design.md

@@ -19,7 +19,7 @@ at least a topology server + control server.
 
 ## Server / Client
 
-Server functionality, such as `open()` and `receive()` -> `send(RequestPath)`, should _not_ require
+Server functionality, such as `open()` and `receive()` -> `send(Path)`, should _not_ require
 any access to a daemon or control service. The main reason is that it ensures efficiency
 by completely avoiding any type of interaction with daemon/CS.
 Interaction with daemon/CS should not be denied but it should not occur during default operations.
@@ -64,7 +64,7 @@ used or returned by `Scion.defaultService()`.
 
 ## Paths
 
-There are two types of paths (both inherit `Path`:
+There are two types of paths (both inherit `Path`, both are (will be) package private):
 
 - `RequestPath` are used to send initial request. They are retrieved from a path service and contain
   meta information.

src/main/java/org/scion/jpan/AbstractDatagramChannel.java

@@ -104,7 +104,8 @@
     synchronized (stateLock) {
       this.pathPolicy = pathPolicy;
       if (isConnected()) {
-        connectionPath = pathPolicy.filter(getOrCreateService().getPaths(connectionPath));
+        connectionPath =
+            (RequestPath) pathPolicy.filter(getOrCreateService().getPaths(connectionPath));
         updateConnection(connectionPath, true);
       }
     }
@@ -181,7 +182,7 @@
    * @return The remote address or 'null' if this channel is not connected.
    * @see DatagramChannel#getRemoteAddress()
    * @see #connect(SocketAddress)
-   * @see #connect(RequestPath)
+   * @see #connect(Path)
    * @throws IOException If an I/O error occurs
    */
   public InetSocketAddress getRemoteAddress() throws IOException {
@@ -217,7 +218,9 @@
    * Connect to a destination host.
    *
    * <p>NB: A SCION channel will internally connect to the next border router (first hop) instead of
-   * the remote host.
+   * the remote host. <br>
+   * If the address is an instance of {@link ScionSocketAddress} then connect will use the path
+   * associated with the address, see {@link #connect(Path)}.
    *
    * <p>NB: This method does internally not call {@link
    * java.nio.channels.DatagramChannel}.connect(), instead it calls bind(). That means this method
@@ -235,7 +238,11 @@
         throw new IllegalArgumentException(
             "connect() requires an InetSocketAddress or a ScionSocketAddress.");
       }
-      return connect(getOrCreateService().lookupAndGetPath((InetSocketAddress) addr, pathPolicy));
+      if (addr instanceof ScionSocketAddress) {
+        return connect(((ScionSocketAddress) addr).getPath());
+      }
+      Path path = getOrCreateService().lookupAndGetPath((InetSocketAddress) addr, pathPolicy);
+      return connect(path);
     }
   }
 
@@ -261,7 +268,11 @@
    * @throws IOException for example when the first hop (border router) cannot be connected.
    */
   @SuppressWarnings("unchecked")
-  public C connect(RequestPath path) throws IOException {
+  public C connect(Path path) throws IOException {
+    if (!(path instanceof RequestPath)) {
+      // Technically we could probably allow this, but it feels like an abuse of the API,
+      throw new IllegalStateException("The path must be a request path.");
+    }
     // For reference: Java DatagramChannel behavior:
     // - fresh channel has getLocalAddress() == null
     // - connect() and send() cause internal bind()
@@ -293,18 +304,18 @@
     synchronized (stateLock) {
       checkConnected(false);
       ensureBound();
-      updateConnection(path, false);
+      updateConnection((RequestPath) path, false);
       return (C) this;
     }
   }
 
   /**
-   * Get the currently connected path. The connected path is set during {@link
-   * #connect(RequestPath)} and may be refreshed when expired.
+   * Get the currently connected path. The connected path is set during {@link #connect(Path)} and
+   * may be refreshed when expired.
    *
    * @return the current Path or `null` if not path is connected.
    */
-  public RequestPath getConnectionPath() {
+  public Path getConnectionPath() {
     synchronized (stateLock) {
       return connectionPath;
     }

src/main/java/org/scion/jpan/Path.java

@@ -53,6 +53,8 @@ public ScionSocketAddress getRemoteSocketAddress() {
     return dstAddress;
   }
 
+  public abstract PathMetadata getMetadata();
+
   @Override
   public String toString() {
     try {

src/main/java/org/scion/jpan/PathMetadata.java

@@ -26,8 +26,8 @@
 
 /**
  * PathMetadata contains the raw path and meta information such as bandwidth, latency or geo
- * coordinates. PathMetadata is available from RequestPaths that are created/returned by the
- * ScionService when requesting a new path from the control service.
+ * coordinates. PathMetadata is available from Paths that are created/returned by the ScionService
+ * when requesting a new path from the control service.
  */
 public class PathMetadata {
 

src/main/java/org/scion/jpan/PathPolicy.java

@@ -17,28 +17,29 @@
 import java.util.*;
 
 public interface PathPolicy {
+  String NO_PATH = "No path found to destination.";
   PathPolicy FIRST = new First();
   PathPolicy MAX_BANDWIDTH = new MaxBandwith();
   PathPolicy MIN_LATENCY = new MinLatency();
   PathPolicy MIN_HOPS = new MinHopCount();
   PathPolicy DEFAULT = MIN_HOPS;
 
   class First implements PathPolicy {
-    public RequestPath filter(List<RequestPath> paths) {
-      return paths.stream().findFirst().orElseThrow(NoSuchElementException::new);
+    public Path filter(List<Path> paths) {
+      return paths.stream().findFirst().orElseThrow(() -> new NoSuchElementException(NO_PATH));
     }
   }
 
   class MaxBandwith implements PathPolicy {
-    public RequestPath filter(List<RequestPath> paths) {
+    public Path filter(List<Path> paths) {
       return paths.stream()
           .max(Comparator.comparing(path -> Collections.min(path.getMetadata().getBandwidthList())))
-          .orElseThrow(NoSuchElementException::new);
+          .orElseThrow(() -> new NoSuchElementException(NO_PATH));
     }
   }
 
   class MinLatency implements PathPolicy {
-    public RequestPath filter(List<RequestPath> paths) {
+    public Path filter(List<Path> paths) {
       // A 0-value indicates that the AS did not announce a latency for this hop.
       // We use Integer.MAX_VALUE for comparison of these ASes.
       return paths.stream()
@@ -48,15 +49,15 @@
                       path.getMetadata().getLatencyList().stream()
                           .mapToLong(l -> l > 0 ? l : Integer.MAX_VALUE)
                           .reduce(0, Long::sum)))
-          .orElseThrow(NoSuchElementException::new);
+          .orElseThrow(() -> new NoSuchElementException(NO_PATH));
     }
   }
 
   class MinHopCount implements PathPolicy {
-    public RequestPath filter(List<RequestPath> paths) {
+    public Path filter(List<Path> paths) {
       return paths.stream()
           .min(Comparator.comparing(path -> path.getMetadata().getInternalHopsList().size()))
-          .orElseThrow(NoSuchElementException::new);
+          .orElseThrow(() -> new NoSuchElementException(NO_PATH));
     }
   }
 
@@ -68,14 +69,14 @@
     }
 
     @Override
-    public RequestPath filter(List<RequestPath> paths) {
+    public Path filter(List<Path> paths) {
       return paths.stream()
           .filter(this::checkPath)
           .findAny()
-          .orElseThrow(NoSuchElementException::new);
+          .orElseThrow(() -> new NoSuchElementException(NO_PATH));
     }
 
-    private boolean checkPath(RequestPath path) {
+    private boolean checkPath(Path path) {
       for (PathMetadata.PathInterface pif : path.getMetadata().getInterfacesList()) {
         int isd = (int) (pif.getIsdAs() >>> 48);
         if (!allowedIsds.contains(isd)) {
@@ -94,14 +95,14 @@
     }
 
     @Override
-    public RequestPath filter(List<RequestPath> paths) {
+    public Path filter(List<Path> paths) {
       return paths.stream()
           .filter(this::checkPath)
           .findAny()
-          .orElseThrow(NoSuchElementException::new);
+          .orElseThrow(() -> new NoSuchElementException(NO_PATH));
     }
 
-    private boolean checkPath(RequestPath path) {
+    private boolean checkPath(Path path) {
       for (PathMetadata.PathInterface pif : path.getMetadata().getInterfacesList()) {
         int isd = (int) (pif.getIsdAs() >>> 48);
         if (disallowedIsds.contains(isd)) {
@@ -117,5 +118,5 @@
    * @return The "best" path according to the filter's policy.
    * @throws NoSuchElementException if no matching path could be found.
    */
-  RequestPath filter(List<RequestPath> paths);
+  Path filter(List<Path> paths);
 }

src/main/java/org/scion/jpan/ResponsePath.java

@@ -67,6 +67,11 @@
     return firstHopAddress;
   }
 
+  @Override
+  public PathMetadata getMetadata() {
+    return null;
+  }
+
   public long getLocalIsdAs() {
     return srcIsdAs;
   }

src/main/java/org/scion/jpan/ScionAddress.java

@@ -21,7 +21,10 @@
  * ScionAddress is an InetAddress + ISD/AS information.
  *
  * <p>This class is threadsafe.
+ *
+ * @deprecated This will be made package private in 0.3.0
  */
+@Deprecated // This will be made package private in 0.3.0
 public class ScionAddress {
   private final long isdAs;
   private final String hostName;

src/main/java/org/scion/jpan/ScionDatagramChannel.java

@@ -117,7 +117,7 @@
     synchronized (stateLock()) {
       path = resolvedDestinations.get(dst);
       if (path == null) {
-        path = getOrCreateService().lookupAndGetPath(dst, getPathPolicy());
+        path = (RequestPath) getOrCreateService().lookupAndGetPath(dst, getPathPolicy());
         resolvedDestinations.put(dst, path);
       }
     }
@@ -251,7 +251,7 @@
       return null;
     }
     // expired, get new path
-    List<RequestPath> paths = getOrCreateService().getPaths(path);
+    List<Path> paths = getOrCreateService().getPaths(path);
     switch (refreshPolicy) {
       case OFF:
         // let this pass until it is ACTUALLY expired
@@ -260,17 +260,17 @@
         }
         throw new ScionRuntimeException("Path is expired");
       case POLICY:
-        return getPathPolicy().filter(getOrCreateService().getPaths(path));
+        return (RequestPath) getPathPolicy().filter(getOrCreateService().getPaths(path));
       case SAME_LINKS:
         return findPathSameLinks(paths, path);
       default:
         throw new UnsupportedOperationException();
     }
   }
 
-  private RequestPath findPathSameLinks(List<RequestPath> paths, RequestPath path) {
+  private RequestPath findPathSameLinks(List<Path> paths, RequestPath path) {
     List<PathMetadata.PathInterface> reference = path.getMetadata().getInterfacesList();
-    for (RequestPath newPath : paths) {
+    for (Path newPath : paths) {
       List<PathMetadata.PathInterface> ifs = newPath.getMetadata().getInterfacesList();
       if (ifs.size() != reference.size()) {
         continue;
@@ -286,7 +286,7 @@
         }
       }
       if (isSame) {
-        return newPath;
+        return (RequestPath) newPath;
       }
     }
     return null;
@@ -300,7 +300,7 @@
    * @param address A destination address
    * @return The mapped path or the path itself if no mapping is available.
    */
-  public RequestPath getMappedPath(InetSocketAddress address) {
+  public Path getMappedPath(InetSocketAddress address) {
     synchronized (stateLock()) {
       return resolvedDestinations.get(address);
     }
@@ -315,9 +315,12 @@
    * @param path A Path
    * @return The mapped path or the path itself if no mapping is available.
    */
-  public RequestPath getMappedPath(RequestPath path) {
+  public Path getMappedPath(Path path) {
+    if (!(path instanceof RequestPath)) {
+      return null;
+    }
     synchronized (stateLock()) {
-      return refreshedPaths.getOrDefault(path, path);
+      return refreshedPaths.getOrDefault(path, (RequestPath) path);
     }
   }
 }