Merge branch 'master' into gg-202301-sm-errors

RELEASE_NOTES.md

@@ -7,3 +7,4 @@
 * update to latest FHIRPath for older versions 
 * clean up error handling when parsing structure maps
 * go-publish related changes
+* FTP Client upload and logging improvements

org.hl7.fhir.dstu2/src/main/java/org/hl7/fhir/dstu2/model/Base64BinaryType.java

@@ -99,7 +99,7 @@ public class Base64BinaryType extends PrimitiveType<byte[]> {
    * @throws DataFormatException
    */
   public void checkValidBase64(String toCheck) throws DataFormatException {
-    if (!Base64.isBase64(toCheck.getBytes())) {
+    if (!org.hl7.fhir.utilities.Base64.isBase64(toCheck.getBytes())) {
       throw new DataFormatException("");
     }
   }

org.hl7.fhir.dstu2016may/src/main/java/org/hl7/fhir/dstu2016may/model/Base64BinaryType.java

@@ -100,7 +100,7 @@ public class Base64BinaryType extends PrimitiveType<byte[]> {
    * @throws DataFormatException
    */
   public void checkValidBase64(String toCheck) throws DataFormatException {
-    if (!Base64.isBase64(toCheck.getBytes())) {
+    if (!org.hl7.fhir.utilities.Base64.isBase64(toCheck.getBytes())) {
       throw new DataFormatException("");
     }
   }

org.hl7.fhir.dstu3/src/main/java/org/hl7/fhir/dstu3/model/Base64BinaryType.java

@@ -152,7 +152,7 @@ public class Base64BinaryType extends PrimitiveType<byte[]> implements IPrimitiv
    * @throws DataFormatException
    */
   public void checkValidBase64(String toCheck) throws DataFormatException {
-    if (!Base64.isBase64(toCheck.getBytes())) {
+    if (!org.hl7.fhir.utilities.Base64.isBase64(toCheck.getBytes())) {
       throw new DataFormatException("");
     }
   }

org.hl7.fhir.r4/src/main/java/org/hl7/fhir/r4/model/Base64BinaryType.java

@@ -152,7 +152,7 @@ public class Base64BinaryType extends PrimitiveType<byte[]> implements IPrimitiv
    * @throws DataFormatException
    */
   public void checkValidBase64(String toCheck) throws DataFormatException {
-    if (!Base64.isBase64(toCheck.getBytes())) {
+    if (!org.hl7.fhir.utilities.Base64.isBase64(toCheck.getBytes())) {
       throw new DataFormatException("");
     }
   }

org.hl7.fhir.r4b/src/main/java/org/hl7/fhir/r4b/model/Base64BinaryType.java

@@ -156,7 +156,7 @@ public class Base64BinaryType extends PrimitiveType<byte[]> implements IPrimitiv
    * @throws DataFormatException
    */
   public void checkValidBase64(String toCheck) throws DataFormatException {
-    if (!Base64.isBase64(toCheck.getBytes())) {
+    if (!org.hl7.fhir.utilities.Base64.isBase64(toCheck.getBytes())) {
       throw new DataFormatException("");
     }
   }

org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/model/Base64BinaryType.java

@@ -156,7 +156,7 @@ public class Base64BinaryType extends PrimitiveType<byte[]> implements IPrimitiv
    * @throws DataFormatException
    */
   public void checkValidBase64(String toCheck) throws DataFormatException {
-    if (!Base64.isBase64(toCheck.getBytes())) {
+    if (!org.hl7.fhir.utilities.Base64.isBase64(toCheck.getBytes())) {
       throw new DataFormatException("");
     }
   }

org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/Base64.java

@@ -0,0 +1,40 @@
+package org.hl7.fhir.utilities;
+
+/**
+ * This is a partial duplication of org.apache.commons.codec.binary.Base64
+ *
+ * It exists because Android compatibility only supports version 1.2 of that
+ * library, which only has the deprecated isArrayByteBase64. The use of
+ * isBase64 from this class will allow us to avoid using a deprecated method
+ * or hacking a solution that involves catching exceptions on decoding.
+ */
+public class Base64 {
+
+  private static final byte[] DECODE_TABLE = new byte[]{-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 62, -1, 62, -1, 63, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, -1, -1, -1, -1, -1, -1, -1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, -1, -1, -1, -1, 63, -1, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51};
+
+  public static boolean isBase64(byte octet) {
+    return octet == 61 || octet >= 0 && octet < DECODE_TABLE.length && DECODE_TABLE[octet] != -1;
+  }
+
+  public static boolean isBase64(byte[] arrayOctet) {
+    for(int i = 0; i < arrayOctet.length; ++i) {
+      if (!isBase64(arrayOctet[i]) && !isWhiteSpace(arrayOctet[i])) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
+  protected static boolean isWhiteSpace(byte byteToCheck) {
+    switch (byteToCheck) {
+      case 9:
+      case 10:
+      case 13:
+      case 32:
+        return true;
+      default:
+        return false;
+    }
+  }
+}

org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/FTPClient.java

@@ -1,26 +1,39 @@
 package org.hl7.fhir.utilities;
 
+import lombok.Getter;
 import org.apache.commons.net.ftp.FTP;
 import org.apache.commons.net.ftp.FTPReply;
+import org.hl7.fhir.exceptions.FHIRException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
-
-import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 
 public class FTPClient {
 
+  private static final Logger logger = LoggerFactory.getLogger(FTPClient.class);
+
   private final org.apache.commons.net.ftp.FTPClient clientImpl;
 
-  final String server;
+  @Getter
+  private final String server;
 
-  final String path;
+  @Getter
+  private final String path;
 
-  final String user;
+  private String resolvedPath = null;
 
-  final String password;
+  @Getter
+  private final String user;
 
-  final int port;
+  @Getter
+  private final String password;
+
+  @Getter
+  private final int port;
+
+  private final String remoteSeparator;
 
   /**
    * Connect to an FTP server
@@ -36,17 +49,26 @@ public class FTPClient {
   protected FTPClient(String server, int port, String path, String user, String password) {
     this.server = server;
     this.port = port;
-    if (path.endsWith("/")) {
+    this.remoteSeparator = "/";
-      this.path = path;
+    this.path = buildPath(path);
-    } else {
+
-      this.path = path + "/";
-    }
     this.user = user;
     this.password = password;
 
     clientImpl = new org.apache.commons.net.ftp.FTPClient();
   }
 
+  private String buildPath(String path) {
+    if (path.length() == 0) {
+      return "";
+    }
+    if (path.endsWith(remoteSeparator))
+    {
+      return path;
+    }
+    return path + remoteSeparator;
+  }
+
   /**
    * Connect to the server, throw an exception if it fails
    */
@@ -60,13 +82,17 @@ public class FTPClient {
 
     clientImpl.login(user, password);
 
+    checkForPositiveCompletionAndLogErrors("FTP server could not connect.", true);
 
-    int reply = clientImpl.getReplyCode();
+    logger.debug("Initial Working directory: " + clientImpl.printWorkingDirectory());
 
-    if(!FTPReply.isPositiveCompletion(reply)) {
+    clientImpl.changeWorkingDirectory(path);
-      clientImpl.disconnect();
+
-      throw new IOException("FTP server refused connection. Reply code: " + reply);
+    checkForPositiveCompletionAndLogErrors("FTP server could not establish default working directory", true);
-    }
+
+    resolvedPath = clientImpl.printWorkingDirectory();
+
+    logger.debug("Resolved working directory: " + resolvedPath);
   }
 
   /**
@@ -76,11 +102,53 @@ public class FTPClient {
    */
   public void delete(String path) throws IOException {
     String resolvedPath = resolveRemotePath(path);
+    logger.debug("Deleting remote file: " + resolvedPath);
     clientImpl.deleteFile(resolvedPath);
+    checkForPositiveCompletionAndLogErrors("Error deleting file.", false);
+    logger.debug("Remote file deleted: " + resolvedPath);
+  }
+
+  /**
+   * Takes a file path and creates all intermediate directories if they do not yet exist.
+   * @param filePath relative to the path provided in the constructor and including the file name
+   * @throws IOException
+   */
+  protected void createRemotePathIfNotExists(String filePath) throws IOException {
+    String[] subPath = filePath.split(remoteSeparator);
+    try {
+    for (int i = 0 ; i < subPath.length - 1; i++){
+      if (subPath[i].isEmpty() ) {
+        continue;
+      }
+      boolean exists = clientImpl.changeWorkingDirectory(subPath[i]);
+      if (!exists) {
+        logger.debug("Remote directory does not exist: " + clientImpl.printWorkingDirectory() + remoteSeparator + subPath[i]);
+        clientImpl.makeDirectory(subPath[i]);
+        clientImpl.changeWorkingDirectory(subPath[i]);
+        logger.debug("Made remote directory: " + clientImpl.printWorkingDirectory());
+      }
+    }} catch (IOException e) {
+      throw new IOException("Error creating remote path: " + filePath, e);
+    } finally {
+      clientImpl.changeWorkingDirectory(this.resolvedPath);
+    }
+  }
+
+  protected boolean remotePathExists(String path) throws IOException {
+    boolean output;
+    try {
+      output = clientImpl.changeWorkingDirectory(path);
+    } finally {
+      clientImpl.changeWorkingDirectory(this.resolvedPath);
+    }
+    return output;
   }
 
   private String resolveRemotePath(String path) {
-    return String.join("", this.path, path);
+    if (path.startsWith(remoteSeparator)) {
+      throw new IllegalArgumentException("Absolute remote path is not permitted. Path: " + path);
+    }
+    return String.join(remoteSeparator, path);
   }
 
   /**
@@ -90,21 +158,59 @@ public class FTPClient {
    */
   public void upload(String source, String path) throws IOException {
     String resolvedPath = resolveRemotePath(path);
+    logger.debug("Uploading file to remote path: " + resolvedPath);
+    createRemotePathIfNotExists(path);
+
     FileInputStream localStream = new FileInputStream(source);
     clientImpl.setFileType(FTP.BINARY_FILE_TYPE);
     clientImpl.enterLocalPassiveMode();
     clientImpl.storeFile( resolvedPath, localStream);
     localStream.close();
 
+    checkForPositiveCompletionAndLogErrors("Error uploading file.", false);
+    logger.debug("Remote file uploaded: " + resolvedPath);
+  }
+
+  private void checkForPositiveCompletionAndLogErrors(String localErrorMessage, boolean disconnectOnError) throws IOException {
     int reply = clientImpl.getReplyCode();
 
-    if(!FTPReply.isPositiveCompletion(reply)) {
+    if (FTPReply.isPositiveCompletion(reply)) {
-      throw new IOException("Error uploading file. Reply code: " + reply);
+      return;
     }
+
+    String remoteErrorMessage = clientImpl.getReplyString();
+    if (disconnectOnError) {
+      clientImpl.disconnect();
+    }
+    throw new IOException(localErrorMessage + " Reply code: " + reply + " Message: " + remoteErrorMessage);
+
+
   }
 
   public void disconnect() throws IOException {
     clientImpl.disconnect();
   }
 
+
+  public static void main(String[] args) throws IOException, FHIRException {
+    FTPClient ftp = new FTPClient(getNamedParam(args, "-upload-server"), getNamedParam(args, "-upload-path"), getNamedParam(args, "-upload-user"), getNamedParam(args, "-upload-password"));
+    ftp.connect();
+    ftp.upload("/Users/grahamegrieve/temp/test.xml", "testing/test.xml");
+    ftp.delete("testing/test.xml");
+    ftp.disconnect();
+  }
+
+  private static String getNamedParam(String[] args, String param) {
+    boolean found = false;
+    for (String a : args) {
+      if (found)
+        return a;
+      if (a.equals(param)) {
+        found = true;
+      }
+    }
+    return null;
+  }
+
+  
 }

org.hl7.fhir.utilities/src/test/java/Base64Tests.java

@@ -0,0 +1,27 @@
+import org.junit.jupiter.api.Test;
+
+import org.hl7.fhir.utilities.Base64;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class Base64Tests {
+  @Test
+  public void testIsArrayByteBase64() {
+    assertFalse(Base64.isBase64(new byte[] { Byte.MIN_VALUE }));
+    assertFalse(Base64.isBase64(new byte[] { -125 }));
+    assertFalse(Base64.isBase64(new byte[] { -10 }));
+    assertFalse(Base64.isBase64(new byte[] { 0 }));
+    assertFalse(Base64.isBase64(new byte[] { 64, Byte.MAX_VALUE }));
+    assertFalse(Base64.isBase64(new byte[] { Byte.MAX_VALUE }));
+
+    assertTrue(Base64.isBase64(new byte[] { 'A' }));
+
+    assertFalse(Base64.isBase64(new byte[] { 'A', Byte.MIN_VALUE }));
+
+    assertTrue(Base64.isBase64(new byte[] { 'A', 'Z', 'a' }));
+    assertTrue(Base64.isBase64(new byte[] { '/', '=', '+' }));
+
+    assertFalse(Base64.isBase64(new byte[] { '$' }));
+  }
+}

org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/FTPClientTest.java

@@ -3,13 +3,14 @@ package org.hl7.fhir.utilities;
 
 import org.apache.commons.io.IOUtils;
 import org.hl7.fhir.utilities.tests.ResourceLoaderTests;
-import org.jetbrains.annotations.NotNull;
+
 import org.junit.jupiter.api.*;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.mockftpserver.fake.FakeFtpServer;
 import org.mockftpserver.fake.UserAccount;
 import org.mockftpserver.fake.filesystem.*;
 
-import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
@@ -31,6 +32,7 @@ public class FTPClientTest implements ResourceLoaderTests {
   public static final String DUMMY_FILE_TO_UPLOAD = "dummyFileToUpload";
   public static final int FAKE_FTP_PORT = 8021;
   public static final String DUMMY_FILE_CONTENT = "Dummy file content\nMore content\n";
+  public static final String LOCALHOST = "localhost";
 
 
   FakeFtpServer fakeFtpServer;
@@ -69,8 +71,6 @@ public class FTPClientTest implements ResourceLoaderTests {
   }
 
   public void setupFakeFtpServer() throws IOException {
-
-
     fakeFtpServer = new FakeFtpServer();
     fakeFtpServer.setServerControlPort(FAKE_FTP_PORT);
     fakeFtpServer.addUserAccount(new UserAccount(DUMMY_USER, DUMMY_PASSWORD, fakeFtpDirectory.toFile().getAbsolutePath()));
@@ -108,6 +108,21 @@ public class FTPClientTest implements ResourceLoaderTests {
     fakeFtpServer.stop();
   }
 
+  @ParameterizedTest
+  @CsvSource({"/", "/test", "/test", "/test/", "/test1/test2", "/test1/test2", "test", "test/", "test1/test2"})
+  public void testValidRelativePaths(String path) {
+    FTPClient client = new FTPClient("localhost", path, DUMMY_USER, DUMMY_PASSWORD);
+    assertTrue(path.length() == client.getPath().length() || path.length() + 1 == client.getPath().length());
+    assertTrue(client.getPath().startsWith(path));
+    assertTrue(client.getPath().endsWith("/"));
+  }
+
+  @Test
+  public void testEmptyRelativePath() {
+    FTPClient client = new FTPClient("localhost", "", DUMMY_USER, DUMMY_PASSWORD);
+    assertEquals("", client.getPath());
+  }
+
   @Test
  public void testDelete() throws IOException {
 
@@ -120,9 +135,8 @@ public class FTPClientTest implements ResourceLoaderTests {
     assertFalse(fakeFtpServer.getFileSystem().exists(deleteFilePath));
  }
 
-  @NotNull
   private static FTPClient connectToFTPClient() throws IOException {
-    FTPClient client = new FTPClient("localhost", FAKE_FTP_PORT, RELATIVE_PATH_1 + "/", DUMMY_USER, DUMMY_PASSWORD);
+    FTPClient client = new FTPClient(LOCALHOST, FAKE_FTP_PORT,  RELATIVE_PATH_1, DUMMY_USER, DUMMY_PASSWORD);
     client.connect();
     return client;
   }
@@ -130,7 +144,7 @@ public class FTPClientTest implements ResourceLoaderTests {
   @Test
  public void testDelete2() throws IOException {
 
-    FTPClient client = connectToFTPClient2();
+    FTPClient client = connectToFTPClient();
 
     String deleteFilePath = dummyFileToDeletePath.toFile().getAbsolutePath();
     assertTrue(fakeFtpServer.getFileSystem().exists(deleteFilePath));
@@ -140,12 +154,6 @@ public class FTPClientTest implements ResourceLoaderTests {
     client.disconnect();
  }
 
-  @NotNull
-  private static FTPClient connectToFTPClient2() throws IOException {
-    FTPClient client = new FTPClient("localhost", FAKE_FTP_PORT, RELATIVE_PATH_1, DUMMY_USER, DUMMY_PASSWORD);
-    client.connect();
-    return client;
-  }
 
   @Test
   public void testUpload() throws IOException {
@@ -153,17 +161,58 @@ public class FTPClientTest implements ResourceLoaderTests {
     FTPClient client = connectToFTPClient();
 
     String uploadFilePath = dummyUploadedFilePath.toFile().getAbsolutePath();
-
     assertFalse(fakeFtpServer.getFileSystem().exists(uploadFilePath));
 
     client.upload(dummyFileToUploadPath.toFile().getAbsolutePath(), RELATIVE_PATH_2 + "/" + DUMMY_FILE_TO_UPLOAD);
 
-    assertTrue(fakeFtpServer.getFileSystem().exists(uploadFilePath));
+    assertUploadedFileCorrect(uploadFilePath);
+  }
 
-    FileEntry fileEntry=  (FileEntry)fakeFtpServer.getFileSystem().getEntry(uploadFilePath);
+  private void assertUploadedFileCorrect(String uploadedFilePath) throws IOException {
+    assertTrue(fakeFtpServer.getFileSystem().exists(uploadedFilePath));
+    FileEntry fileEntry = (FileEntry)fakeFtpServer.getFileSystem().getEntry(uploadedFilePath);
+    assertNotNull(fileEntry);
     InputStream inputStream = fileEntry.createInputStream();
     byte[] bytes = IOUtils.toByteArray(inputStream);
     String actualContent = new String(bytes, StandardCharsets.UTF_8);
     assertEquals(DUMMY_FILE_CONTENT,actualContent);
   }
+
+  @Test
+  public void testCreateRemotePathDoesntExist() throws IOException {
+    FTPClient client = connectToFTPClient();
+
+    Path newPath1 = relativePath2.resolve("newPath1");
+    Path newPath2 = newPath1.resolve("newPath2");
+
+    assertFalse(fakeFtpServer.getFileSystem().exists(newPath1.toFile().getAbsolutePath()));
+    assertFalse(fakeFtpServer.getFileSystem().exists(newPath2.toFile().getAbsolutePath()));
+
+    client.createRemotePathIfNotExists(RELATIVE_PATH_2 + "/newPath1/newPath2/newFile.txt");
+
+    assertTrue(fakeFtpServer.getFileSystem().exists(newPath1.toFile().getAbsolutePath()));
+    assertTrue(fakeFtpServer.getFileSystem().exists(newPath2.toFile().getAbsolutePath()));
+
+  }
+
+  @Test
+  public void testUploadWherePathDoesntExist() throws IOException {
+    Path newPath1 = relativePath2.resolve("newPath1");
+    Path newPath2 = newPath1.resolve("newPath2");
+
+    FTPClient client = connectToFTPClient();
+
+    Path uploadFilePath = newPath2.resolve(DUMMY_FILE_TO_UPLOAD);
+    assertFalse(fakeFtpServer.getFileSystem().exists(uploadFilePath.toFile().getAbsolutePath()));
+
+    assertFalse(fakeFtpServer.getFileSystem().exists(newPath1.toFile().getAbsolutePath()));
+    assertFalse(fakeFtpServer.getFileSystem().exists(newPath2.toFile().getAbsolutePath()));
+
+    client.upload(dummyFileToUploadPath.toFile().getAbsolutePath(),RELATIVE_PATH_2 + "/newPath1/newPath2/" + DUMMY_FILE_TO_UPLOAD);
+
+    assertTrue(fakeFtpServer.getFileSystem().exists(newPath1.toFile().getAbsolutePath()));
+    assertTrue(fakeFtpServer.getFileSystem().exists(newPath2.toFile().getAbsolutePath()));
+
+    assertUploadedFileCorrect(uploadFilePath.toFile().getAbsolutePath());
+  }
 }

pom.xml

@@ -391,6 +391,29 @@
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-deploy-plugin</artifactId>
             </plugin>
+
+            <plugin>
+                <groupId>org.codehaus.mojo</groupId>
+                <artifactId>animal-sniffer-maven-plugin</artifactId>
+                <version>1.22</version>
+                <configuration>
+                    <signature>
+                        <groupId>net.sf.androidscents.signature</groupId>
+                        <artifactId>android-api-level-26</artifactId>
+                        <version>8.0.0_r2</version>
+                    </signature>
+                    <failOnError>false</failOnError>
+                </configuration>
+                <executions>
+                    <execution>
+                        <id>check-android-26-compliance</id>
+                        <phase>test</phase>
+                        <goals>
+                            <goal>check</goal>
+                        </goals>
+                    </execution>
+                </executions>
+            </plugin>
         </plugins>
     </build>