protocol: Improve handling early exits & picture

Picture data was a part of the Client class, which
wasn't useful in a real use case. This change
removes that part and makes PersistenceProvider
responsible for it. Also, checksum is now is
considered 'revision number', so it doesn't mean
it is the hash code of the picture data but
rather a date on which the picture was changed.

Author: velitasali

pom.xml

@@ -22,7 +22,7 @@
 
     <groupId>org.monora.uprotocol</groupId>
     <artifactId>core</artifactId>
-    <version>0.1.0-ALPHA32</version>
+    <version>0.1.0-ALPHA33</version>
     <name>UProtocol</name>
     <description>Java implementation of uprotocol, an open specification for p2p content exchange</description>
     <url>https://github.com/uprotocol</url>
@@ -165,19 +165,19 @@
         <dependency>
             <groupId>junit</groupId>
             <artifactId>junit</artifactId>
-            <version>4.13.1</version>
+            <version>4.13.2</version>
             <scope>test</scope>
         </dependency>
         <dependency>
             <groupId>org.junit.jupiter</groupId>
             <artifactId>junit-jupiter</artifactId>
-            <version>5.6.1</version>
+            <version>5.7.2</version>
             <scope>test</scope>
         </dependency>
         <dependency>
             <groupId>org.json</groupId>
             <artifactId>json</artifactId>
-            <version>20201115</version>
+            <version>20210307</version>
         </dependency>
         <dependency>
             <groupId>org.monora.coolsocket</groupId>
@@ -187,7 +187,7 @@
         <dependency>
             <groupId>org.junit.vintage</groupId>
             <artifactId>junit-vintage-engine</artifactId>
-            <version>5.7.0</version>
+            <version>5.7.2</version>
             <scope>test</scope>
         </dependency>
         <dependency>
@@ -205,8 +205,8 @@
         <dependency>
             <groupId>org.jetbrains</groupId>
             <artifactId>annotations</artifactId>
-            <version>16.0.2</version>
+            <version>21.0.1</version>
             <scope>compile</scope>
         </dependency>
     </dependencies>
-</project>
+</project>

src/main/java/org/monora/uprotocol/core/ClientLoader.java

@@ -30,7 +30,7 @@ public class ClientLoader
      * @param persistenceProvider That stores persistent data.
      * @param object              To load the details from.
      * @param clientUid           For which this method invocation is being made.
-     * @param unblock             True if this should unblocked the remote if blocked.
+     * @param unblock             True if this should unblock the remote if blocked.
      * @return The client produced from the JSON object and persistence database.
      * @throws JSONException                If something goes wrong when inflating the JSON data.
      * @throws BlockedRemoteClientException If the remote is blocked on the side and 'unblock' parameter is false.
@@ -57,7 +57,7 @@ public class ClientLoader
      * @param clientUid           For which this method invocation is being made.
      * @param hasPin              Whether the request has a valid PIN. When it does, the remote client will be unblocked
      *                            if blocked.
-     * @return The client produced from the JSON object and persistence database.
+     * @return The client data produced from the JSON object and persistence database.
      * @throws JSONException                If something goes wrong when inflating the JSON data.
      * @throws BlockedRemoteClientException If remote is blocked and has no valid PIN. The underlying data is loaded
      *                                      after this is thrown.
@@ -79,33 +79,30 @@ public class ClientLoader
                                             @NotNull JSONObject response, @NotNull String clientUid, boolean hasPin,
                                             boolean asClient, boolean unblockAsClient) throws JSONException
     {
-        String nickname = response.getString(Keyword.CLIENT_NICKNAME);
-        String manufacturer = response.getString(Keyword.CLIENT_MANUFACTURER);
-        String product = response.getString(Keyword.CLIENT_PRODUCT);
-        ClientType clientType = ClientType.from(response.getString(Keyword.CLIENT_TYPE));
-        String versionName = response.getString(Keyword.CLIENT_VERSION_NAME);
-        int versionCode = response.getInt(Keyword.CLIENT_VERSION_CODE);
-        int protocolVersion = response.getInt(Keyword.CLIENT_PROTOCOL_VERSION);
-        int protocolVersionMin = response.getInt(Keyword.CLIENT_PROTOCOL_VERSION_MIN);
-        long lastUsageTime = System.currentTimeMillis();
-        boolean local = persistenceProvider.getClientUid().equals(clientUid);
-
-        if (nickname.length() > LENGTH_CLIENT_USERNAME) {
-            nickname = nickname.substring(0, LENGTH_CLIENT_USERNAME);
-        }
+        final String nickname = Clients.cleanNickname(response.getString(Keyword.CLIENT_NICKNAME));
+        final String manufacturer = response.getString(Keyword.CLIENT_MANUFACTURER);
+        final String product = response.getString(Keyword.CLIENT_PRODUCT);
+        final ClientType clientType = ClientType.from(response.getString(Keyword.CLIENT_TYPE));
+        final String versionName = response.getString(Keyword.CLIENT_VERSION_NAME);
+        final int versionCode = response.getInt(Keyword.CLIENT_VERSION_CODE);
+        final int protocolVersion = response.getInt(Keyword.CLIENT_PROTOCOL_VERSION);
+        final int protocolVersionMin = response.getInt(Keyword.CLIENT_PROTOCOL_VERSION_MIN);
+        final long revisionOfPicture = response.getLong(Keyword.CLIENT_REVISION_PICTURE);
+        final long lastUsageTime = System.currentTimeMillis();
+        final boolean local = persistenceProvider.getClientUid().equals(clientUid);
 
         Client client = persistenceProvider.getClientFor(clientUid);
-        boolean updating = false;
+
+        final boolean updating = client != null;
+        final boolean needsPictureRevision = client == null || client.getClientRevisionOfPicture() != revisionOfPicture;
 
         if (client == null) {
             client = persistenceProvider.createClientFor(clientUid, nickname, manufacturer, product, clientType,
-                    versionName, versionCode, protocolVersion, protocolVersionMin);
+                    versionName, versionCode, protocolVersion, protocolVersionMin, revisionOfPicture);
         } else {
             Clients.fill(client, clientUid, client.getClientCertificate(), nickname, manufacturer, product, clientType,
-                    versionName, versionCode, protocolVersion, protocolVersionMin, client.isClientTrusted(),
-                    client.isClientBlocked());
-
-            updating = true;
+                    versionName, versionCode, protocolVersion, protocolVersionMin, revisionOfPicture,
+                    client.isClientTrusted(), client.isClientBlocked());
         }
 
         try {
@@ -122,13 +119,10 @@ public class ClientLoader
             client.setClientLocal(local);
             persistenceProvider.persist(client, updating);
 
-            int checksum = response.getInt(Keyword.CLIENT_PICTURE_CHECKSUM);
-
-            if (client.getClientPictureChecksum() != checksum) {
+            if (needsPictureRevision) {
                 String data = response.optString(Keyword.CLIENT_PICTURE);
                 try {
-                    persistenceProvider.persistClientPicture(client,
-                            data != null ? Base64.decode(data) : null, checksum);
+                    persistenceProvider.persistClientPicture(client, data != null ? Base64.decode(data) : null);
                 } catch (IOException ignored) {
                 }
             }

src/main/java/org/monora/uprotocol/core/CommunicationBridge.java

@@ -252,7 +252,7 @@ static void convertToSSL(@NotNull ConnectionFactory connectionFactory,
      * <p>
      * For instance, the remote is setting up a file transfer request and is about to pick a client. If you make this
      * request in that timespan, this will invoke {@link TransportSeat#handleAcquaintanceRequest(Client, ClientAddress)}
-     * method on the remote and it will choose you.
+     * method on the remote, and it will choose you.
      *
      * @return True if successful.
      * @throws IOException       If an IO error occurs.
@@ -572,11 +572,12 @@ public CommunicationBridge connect() throws IOException, JSONException, Protocol
             String remoteClientUid = activeConnection.receive().getAsString();
 
             if (clientUid != null && !clientUid.equals(remoteClientUid)) {
-                activeConnection.closeSafely();
+                Responses.send(activeConnection, false, new JSONObject());
                 throw new DifferentRemoteClientException(clientUid, remoteClientUid, address);
             }
 
-            activeConnection.reply(persistenceProvider.clientAsJson(pin));
+            Responses.send(activeConnection, true, persistenceProvider.clientAsJson(pin));
+
             JSONObject jsonObject = activeConnection.receive().getAsJson();
             Client client = ClientLoader.loadAsClient(persistenceProvider, jsonObject, remoteClientUid,
                     clearBlockedStatus);
@@ -644,4 +645,4 @@ public void setPin(int pin)
             this.pin = pin;
         }
     }
-}
+}

src/main/java/org/monora/uprotocol/core/TransportSession.java

@@ -69,6 +69,11 @@ public void onConnected(@NotNull ActiveConnection activeConnection)
             activeConnection.reply(persistenceProvider.getClientUid());
 
             final JSONObject response = activeConnection.receive().getAsJson();
+            if (!Responses.getResult(response)) {
+                getLogger().log(Level.INFO, "Remote returned false");
+                return;
+            }
+
             final int activePin = persistenceProvider.getNetworkPin();
             final boolean hasPin = activePin != 0 && activePin == response.getInt(Keyword.CLIENT_PIN);
 

src/main/java/org/monora/uprotocol/core/persistence/PersistenceProvider.java

@@ -3,7 +3,6 @@
 import net.iharder.Base64;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
-import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.monora.uprotocol.core.CommunicationBridge;
@@ -27,7 +26,6 @@
 import java.security.cert.Certificate;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
-import java.util.ArrayList;
 import java.util.List;
 
 /**
@@ -71,7 +69,8 @@ public interface PersistenceProvider
     default @NotNull JSONObject clientAsJson(int pin) throws JSONException
     {
         Client client = getClient();
-        String pictureData = client.hasPicture() ? Base64.encodeBytes(client.getClientPictureData()) : null;
+        byte[] picture = getClientPicture(client);
+        String pictureData = picture == null ? null : Base64.encodeBytes(picture);
 
         return new JSONObject()
                 .put(Keyword.CLIENT_UID, client.getClientUid())
@@ -81,11 +80,11 @@ public interface PersistenceProvider
                 .put(Keyword.CLIENT_TYPE, client.getClientType().getProtocolValue())
                 .put(Keyword.CLIENT_VERSION_CODE, client.getClientVersionCode())
                 .put(Keyword.CLIENT_VERSION_NAME, client.getClientVersionName())
+                .put(Keyword.CLIENT_PICTURE, pictureData)
                 .put(Keyword.CLIENT_PROTOCOL_VERSION, client.getClientProtocolVersion())
                 .put(Keyword.CLIENT_PROTOCOL_VERSION_MIN, client.getClientProtocolVersionMin())
-                .put(Keyword.CLIENT_PIN, pin)
-                .put(Keyword.CLIENT_PICTURE, pictureData)
-                .put(Keyword.CLIENT_PICTURE_CHECKSUM, client.getClientPictureChecksum());
+                .put(Keyword.CLIENT_REVISION_PICTURE, client.getClientRevisionOfPicture())
+                .put(Keyword.CLIENT_PIN, pin);
     }
 
     /**
@@ -116,14 +115,16 @@ public interface PersistenceProvider
      * @param versionName        Points to {@link Client#getClientVersionName()}
      * @param versionCode        Points to {@link Client#getClientVersionCode()}.
      * @param protocolVersion    Points to {@link Client#getClientProtocolVersion()}.
-     * @param protocolVersionMin Points {@link Client#getClientProtocolVersionMin()}.
+     * @param protocolVersionMin Points to {@link Client#getClientProtocolVersionMin()}.
+     * @param revisionOfPicture  Points to {@link Client#getClientRevisionOfPicture()}
      * @return The client instance.
      * @see #persist(Client, boolean)
      * @see #getClientFor(String)
      */
     @NotNull Client createClientFor(@NotNull String uid, @NotNull String nickname, @NotNull String manufacturer,
                                     @NotNull String product, @NotNull ClientType type, @NotNull String versionName,
-                                    int versionCode, int protocolVersion, int protocolVersionMin);
+                                    int versionCode, int protocolVersion, int protocolVersionMin,
+                                    long revisionOfPicture);
 
     /**
      * todo: Should this really exist? This user can avoid using it. The benefit may be to use it as a factory.
@@ -172,6 +173,17 @@ public interface PersistenceProvider
      */
     @NotNull String getClientNickname();
 
+    /**
+     * Returns the profile picture data for the given client.
+     * <p>
+     * If the given client's UID matches {@link #getClientUid()}, you should return your own picture data to be sent to
+     * remote clients.
+     *
+     * @param client For which the picture will be provided.
+     * @return The client picture data or null if no such data exists for the given client.
+     */
+    byte @Nullable [] getClientPicture(@NotNull Client client);
+
     /**
      * This should return the unique identifier for this client. It should be both unique and persistent.
      * <p>
@@ -402,11 +414,10 @@ public void checkServerTrusted(X509Certificate[] certs, String authType)
      * <p>
      * The invocation of this method mean the picture is new and should be saved.
      *
-     * @param client   The client that owns the picture.
-     * @param data     The bitmap data of the picture which is null if the owner client no longer has one.
-     * @param checksum The hash code of the given data which is invalidated if the data is null.
+     * @param client The client that owns the picture.
+     * @param data   The bitmap data of the picture which is null if the owner client no longer has one.
      */
-    void persistClientPicture(@NotNull Client client, byte @Nullable [] data, int checksum);
+    void persistClientPicture(@NotNull Client client, byte @Nullable [] data);
 
     /**
      * Revoke the current valid network PIN.