Repository: amzn/ion-java
Pull request #433 (2022-05-11 17:48:40)

Jar info changes
*Issue #, if available:* #432

*Description of changes:* Uses a `build.properties` file interpolated from Maven variables at build time to make project version and build time information available through `JarInfo`. This approach does not fail when multiple instances of `ion-java` are available on the classpath. 

For testing I built two different versions of `ion-java` with each with this patch applied (in `props`) or not (in `manifest`).
```plain
❯ ls props
ion-java-1.9.5-SNAPSHOT.jar  ion-java-1.9.99-SNAPSHOT.jar

❯ ls manifest
ion-java-1.9.5-SNAPSHOT.jar  ion-java-1.9.99-SNAPSHOT.jar
```

I created a test project `Hello.java` as so:
```java
import com.amazon.ion.util.JarInfo;

public class Hello {
  public static void main(String... a) {
    JarInfo info = new JarInfo();
    System.out.println("ion-java project version is: " +info.getProjectVersion());
  }
}
```

I checked all four permutations of property/manifest version mixing:
```plain
❯ mv manifest/ion-java-1.9.5-SNAPSHOT.jar props/ion-java-1.9.99-SNAPSHOT.jar mixed/

❯ mv props/ion-java-1.9.5-SNAPSHOT.jar manifest/ion-java-1.9.99-SNAPSHOT.jar backmixed/
```

```plain
❯ for i in manifest props mixed backmixed; do; printf "%10s: " "$i"; java -cp ./$i/*:. Hello; echo; done;
  manifest: Exception in thread "main" com.amazon.ion.IonException: Found multiple manifests with ion-java version info on the classpath.
	at com.amazon.ion.util.JarInfo.loadBuildProperties(JarInfo.java:114)
	at com.amazon.ion.util.JarInfo.<init>(JarInfo.java:73)
	at Hello.main(Hello.java:5)

     props: ion-java project version is: 1.9.5-SNAPSHOT

     mixed: ion-java project version is: 1.9.5-SNAPSHOT

 backmixed: ion-java project version is: 1.9.5-SNAPSHOT
```

Two manifest-using versions of `ion-java` was the only mutually incompatible solution, and in each of the other cases the first (alphabetically-listed) version of the jar won out. 

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


codecov[bot]: # [Codecov](https://codecov.io/gh/amzn/ion-java/pull/433?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#433](https://codecov.io/gh/amzn/ion-java/pull/433?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (23ba665) into [master](https://codecov.io/gh/amzn/ion-java/commit/ab41cd18e6727ad53ca354752cdefcaa374a503c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (ab41cd1) will **increase** coverage by `0.00%`.
> The diff coverage is `66.66%`.

```diff
@@            Coverage Diff            @@
##             master     #433   +/-   ##
=========================================
  Coverage     66.53%   66.54%           
+ Complexity     5419     5410    -9     
=========================================
  Files           156      156           
  Lines         22776    22762   -14     
  Branches       4107     4105    -2     
=========================================
- Hits          15155    15146    -9     
+ Misses         6260     6250   -10     
- Partials       1361     1366    +5     
```


| [Impacted Files](https://codecov.io/gh/amzn/ion-java/pull/433?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [src/com/amazon/ion/util/JarInfo.java](https://codecov.io/gh/amzn/ion-java/pull/433/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbS9hbWF6b24vaW9uL3V0aWwvSmFySW5mby5qYXZh) | `75.00% <66.66%> (+14.47%)` | :arrow_up: |
| [src/com/amazon/ion/impl/IonReaderBinaryRawX.java](https://codecov.io/gh/amzn/ion-java/pull/433/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbS9hbWF6b24vaW9uL2ltcGwvSW9uUmVhZGVyQmluYXJ5UmF3WC5qYXZh) | `79.87% <0.00%> (-0.32%)` | :arrow_down: |
| [src/com/amazon/ion/impl/BlockedBuffer.java](https://codecov.io/gh/amzn/ion-java/pull/433/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbS9hbWF6b24vaW9uL2ltcGwvQmxvY2tlZEJ1ZmZlci5qYXZh) | `50.97% <0.00%> (-0.25%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/amzn/ion-java/pull/433?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/amzn/ion-java/pull/433?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Last update [ab41cd1...23ba665](https://codecov.io/gh/amzn/ion-java/pull/433?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).

jobarr-amzn: ion-tests was a rollback! I did a `git submodule update` because I was tired of seeing it in `git status`, but I wasn't all the way up to date there. Good catch, I'll fix. 

Patch:
diff --git a/pom.xml b/pom.xml
index 4a3d696a..781b0d20 100644
--- a/pom.xml
+++ b/pom.xml
@@ -65,7 +65,17 @@
     <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
     <maven.build.timestamp.format>yyyy</maven.build.timestamp.format>
     <build.year>${maven.build.timestamp}</build.year>
-    <jdkVersion>1.6</jdkVersion>
+    <!-- As of 2022 we still need to use a parallel property to actually get formatting
+      Don't be fooled if removing the .format line below looks to have no effect- try changing it.
+      See: https://issues.apache.org/jira/browse/MRESOURCES-99
+      -->
+    <maven.build.timestamp.format>yyyy-MM-dd'T'HH:mm:ssXXX</maven.build.timestamp.format>
+    <build.time>${maven.build.timestamp}</build.time>
+    <!-- Can't target 6 from 8
+      https://stackoverflow.com/questions/48146930/is-it-possible-to-compile-project-on-java-8-for-target-java-6
+    -->
+    <jdk.source.version>6</jdk.source.version>
+    <jdk.target.version>6</jdk.target.version>
   </properties>
 
   <dependencies>
@@ -76,6 +86,13 @@
       <version>4.13.1</version>
       <scope>test</scope>
     </dependency>
+    <!-- https://mvnrepository.com/artifact/org.hamcrest/hamcrest -->
+    <dependency>
+      <groupId>org.hamcrest</groupId>
+      <artifactId>hamcrest</artifactId>
+      <version>2.2</version>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>pl.pragmatists</groupId>
       <artifactId>JUnitParams</artifactId>
@@ -85,43 +102,29 @@
   </dependencies>
 
   <build>
+    <resources>
+      <resource>
+        <!-- src/main/resources is the default resources directory but it looks like we need to list it explicitly in
+             order to set filtering=true, which does the interpolation in properties files
+             -->
+        <directory>src/main/resources</directory>
+        <filtering>true</filtering>
+      </resource>
+    </resources>
     <sourceDirectory>src</sourceDirectory>
     <testSourceDirectory>test</testSourceDirectory>
     <plugins>
+
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-compiler-plugin</artifactId>
-        <version>3.8.0</version>
+        <version>3.8.1</version>
         <configuration>
-          <source>${jdkVersion}</source>
-          <target>${jdkVersion}</target>
+          <source>${jdk.source.version}</source>
+          <target>${jdk.target.version}</target>
         </configuration>
       </plugin>
-      <plugin>
-        <!--
-        Since the format of maven.build.timestamp cannot be changed
-        during the build, define the build.time property here.
-        -->
-        <groupId>org.apache.maven.plugins</groupId>
-        <artifactId>maven-antrun-plugin</artifactId>
-        <version>1.8</version>
-        <executions>
-          <execution>
-            <phase>initialize</phase>
-            <goals>
-              <goal>run</goal>
-            </goals>
-            <configuration>
-              <exportAntProperties>true</exportAntProperties>
-              <target>
-                <tstamp>
-                  <format property="build.time" pattern="yyyy-MM-dd'T'HH:mm:ssXXX"/>
-                </tstamp>
-              </target>
-            </configuration>
-          </execution>
-        </executions>
-      </plugin>
+
       <plugin>
         <!-- Run the unit tests. -->
         <groupId>org.apache.maven.plugins</groupId>
@@ -134,6 +137,7 @@
           <argLine>-ea -Xmx3000M ${argLine}</argLine>
         </configuration>
       </plugin>
+
       <plugin>
         <groupId>org.apache.felix</groupId>
         <artifactId>maven-bundle-plugin</artifactId>
@@ -148,13 +152,11 @@
             </Export-Package>
             <!-- This can be used to retrieve the compiled JarInfo. -->
             <Main-Class>com.amazon.ion.impl._Private_CommandLine</Main-Class>
-            <!-- Add these properties to the manifest so they may be retrieved at runtime. -->
-            <Ion-Java-Build-Time>${build.time}</Ion-Java-Build-Time>
-            <Ion-Java-Project-Version>${project.version}</Ion-Java-Project-Version>
             <Automatic-Module-Name>com.amazon.ion</Automatic-Module-Name>
           </instructions>
         </configuration>
       </plugin>
+
       <!-- Code Coverage -->
       <plugin>
         <groupId>org.jacoco</groupId>
@@ -271,11 +273,9 @@
   <profiles>
     <profile>
       <id>release</id>
-      <properties>
-        <jdkVersion>1.5</jdkVersion>
-      </properties>
       <build>
         <plugins>
+
           <plugin>
             <!-- Package the source jar. -->
             <groupId>org.apache.maven.plugins</groupId>
@@ -290,6 +290,7 @@
               </execution>
             </executions>
           </plugin>
+
           <plugin>
             <!-- Package the javadoc jar. -->
             <groupId>org.apache.maven.plugins</groupId>
@@ -318,6 +319,7 @@
               </execution>
             </executions>
           </plugin>
+
           <plugin>
             <!-- GPG signing. -->
             <groupId>org.apache.maven.plugins</groupId>
@@ -333,6 +335,7 @@
               </execution>
             </executions>
           </plugin>
+
           <plugin>
             <!-- Publish the artifacts. -->
             <groupId>org.sonatype.plugins</groupId>
@@ -345,6 +348,7 @@
               <autoReleaseAfterClose>true</autoReleaseAfterClose>
             </configuration>
           </plugin>
+
         </plugins>
       </build>
     </profile>
diff --git a/src/com/amazon/ion/util/JarInfo.java b/src/com/amazon/ion/util/JarInfo.java
index 1f0ba480..061ee234 100644
--- a/src/com/amazon/ion/util/JarInfo.java
+++ b/src/com/amazon/ion/util/JarInfo.java
@@ -16,10 +16,12 @@
 package com.amazon.ion.util;
 
 import java.io.IOException;
+import java.io.InputStream;
 import java.net.URL;
 import java.util.ArrayList;
 import java.util.Enumeration;
 import java.util.List;
+import java.util.Properties;
 import java.util.jar.Attributes;
 import java.util.jar.Manifest;
 import com.amazon.ion.IonException;
@@ -31,11 +33,6 @@ import com.amazon.ion.Timestamp;
  */
 public final class JarInfo
 {
-
-    private static final String MANIFEST_FILE = "META-INF/MANIFEST.MF";
-    private static final String BUILD_TIME_ATTRIBUTE = "Ion-Java-Build-Time";
-    private static final String PROJECT_VERSION_ATTRIBUTE = "Ion-Java-Project-Version";
-
     private String ourProjectVersion;
 
     private Timestamp ourBuildTime;
@@ -49,34 +46,9 @@ public final class JarInfo
      */
     public JarInfo() throws IonException
     {
-        Enumeration<URL> manifestUrls;
-        try
-        {
-            manifestUrls = getClass().getClassLoader().getResources(MANIFEST_FILE);
-        }
-        catch (IOException e)
-        {
-            throw new IonException("Unable to load manifests.", e);
-        }
-        List<Manifest> manifests = new ArrayList<Manifest>();
-        while (manifestUrls.hasMoreElements())
-        {
-            try
-            {
-                manifests.add(new Manifest(manifestUrls.nextElement().openStream()));
-            }
-            catch (IOException e)
-            {
-                continue; // try the next manifest
-            }
-        }
-        loadBuildProperties(manifests);
+        loadBuildProperties();
     }
 
-    JarInfo(List<Manifest> manifests)
-    {
-        loadBuildProperties(manifests);
-    }
 
     /**
      * Gets the ion-java project version of this build.
@@ -101,50 +73,54 @@ public final class JarInfo
     // TODO writeTo(IonWriter)
 
     // ========================================================================
-
-    private void loadBuildProperties(List<Manifest> manifests) throws IonException
+    /**
+     * @return null but not empty string
+     */
+    private static String nonEmptyProperty(Properties props, String name)
     {
-        boolean propertiesLoaded = false;
-        for(Manifest manifest : manifests)
-        {
-            boolean success = tryLoadBuildProperties(manifest);
-            if(success && propertiesLoaded)
-            {
-                // In the event of conflicting manifests, fail instead of risking returning incorrect version info.
-                throw new IonException("Found multiple manifests with ion-java version info on the classpath.");
-            }
-            propertiesLoaded |= success;
-        }
-        if (!propertiesLoaded)
-        {
-            throw new IonException("Unable to locate manifest with ion-java version info on the classpath.");
-        }
+        String value = props.getProperty(name, "");
+        if (value.length() == 0) value = null;
+        return value;
     }
 
-    /*
-     * Returns true if the properties were loaded, otherwise false.
-     */
-    private boolean tryLoadBuildProperties(Manifest manifest)
+    private void loadBuildProperties()
+            throws IonException
     {
-        Attributes mainAttributes = manifest.getMainAttributes();
-        String projectVersion = mainAttributes.getValue(PROJECT_VERSION_ATTRIBUTE);
-        String time = mainAttributes.getValue(BUILD_TIME_ATTRIBUTE);
-
-        if (projectVersion == null || time == null)
+        String file = "/build.properties";
+        try
         {
-            return false;
-        }
+            Properties props = new Properties();
 
-        ourProjectVersion = projectVersion;
+            InputStream in = getClass().getResourceAsStream(file);
+            if (in != null)
+            {
+                try
+                {
+                    props.load(in);
+                }
+                finally
+                {
+                    in.close();
+                }
+            }
 
-        try
-        {
-            ourBuildTime = Timestamp.valueOf(time);
+            ourProjectVersion = nonEmptyProperty(props, "build.version");
+
+            String time = nonEmptyProperty(props, "build.time");
+            if (time != null)
+            {
+                try {
+                    ourBuildTime = Timestamp.valueOf(time);
+                }
+                catch (IllegalArgumentException e)
+                {
+                    // Badly formatted timestamp. Ignore it.
+                }
+            }
         }
-        catch (IllegalArgumentException e)
+        catch (IOException e)
         {
-            // Badly formatted timestamp. Ignore it.
+            throw new IonException("Unable to load " + file, e);
         }
-        return true;
     }
 }
\ No newline at end of file
diff --git a/src/main/resources/build.properties b/src/main/resources/build.properties
new file mode 100644
index 00000000..3894ceda
--- /dev/null
+++ b/src/main/resources/build.properties
@@ -0,0 +1,4 @@
+# These values are defined in pom.xml
+build.version=${pom.version}
+build.time=${build.time}
+build.name=${pom.name}


Test patch:
diff --git a/test/com/amazon/ion/util/JarInfoTest.java b/test/com/amazon/ion/util/JarInfoTest.java
index 3994ed4e..f51e1789 100644
--- a/test/com/amazon/ion/util/JarInfoTest.java
+++ b/test/com/amazon/ion/util/JarInfoTest.java
@@ -15,95 +15,31 @@
 
 package com.amazon.ion.util;
 
-import com.amazon.ion.IonException;
 import com.amazon.ion.Timestamp;
-import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 
-import java.io.ByteArrayInputStream;
-import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.List;
-import java.util.jar.Manifest;
-
-import static org.junit.Assert.*;
-
-public class JarInfoTest
-{
-    @Rule
-    public ExpectedException thrown = ExpectedException.none();
-
-    private static String getAttributes(String manifestVersion)
-    {
-        return "Manifest-Version: " + manifestVersion + "\n";
-    }
-
-    private static String getIonJavaAttributes(String buildTime, String version)
-    {
-        return getAttributes("1.0")
-                + "Ion-Java-Build-Time: " + buildTime + "\n"
-                + "Ion-Java-Project-Version: " + version + "\n";
-    }
-
-    private static Manifest getManifest(String attributes) throws IOException
-    {
-        return new Manifest(new ByteArrayInputStream(attributes.getBytes("UTF-8")));
-    }
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.matchesPattern;
 
+public class JarInfoTest {
     @Test
-    public void testSingleManifest() throws Exception
-    {
-        String expectedBuildTime = "1984T";
-        String expectedVersion = "42.0";
-
-        Manifest manifest = getManifest(getIonJavaAttributes(expectedBuildTime, expectedVersion));
-        JarInfo info = new JarInfo(Collections.singletonList(manifest));
-
-        assertEquals(expectedVersion, info.getProjectVersion());
-        assertEquals(Timestamp.valueOf(expectedBuildTime), info.getBuildTime());
+    public void getBuildTime() {
+        JarInfo info = new JarInfo();
+        Timestamp timestamp = info.getBuildTime();
+        // This test was written at the timestamp below, the loaded time should always be greater
+        Timestamp original = Timestamp.valueOf("2022-05-10T22:56:48Z");
+        assertThat(timestamp, greaterThan(original));
     }
 
     @Test
-    public void testMultipleManifests() throws Exception
-    {
-        String expectedBuildTime = "1984T";
-        String expectedVersion = "42.0";
-
-        List<Manifest> manifests = new ArrayList<Manifest>();
-        manifests.add(getManifest(getAttributes("1.0")));
-        manifests.add(getManifest(getIonJavaAttributes(expectedBuildTime, expectedVersion)));
-        manifests.add(getManifest(getAttributes("2.0")));
-
-        JarInfo info = new JarInfo(manifests);
-
-        assertEquals(expectedVersion, info.getProjectVersion());
-        assertEquals(Timestamp.valueOf(expectedBuildTime), info.getBuildTime());
-    }
-
-    @Test
-    public void testNoMatchingManifests() throws Exception
-    {
-        List<Manifest> manifests = new ArrayList<Manifest>();
-        manifests.add(getManifest(getAttributes("1.0")));
-        manifests.add(getManifest(getAttributes("2.0")));
-        manifests.add(getManifest(getAttributes("3.0")));
-
-        thrown.expect(IonException.class);
-        new JarInfo(manifests);
-    }
-
-    @Test
-    public void testConflictingManifests() throws Exception
-    {
-        List<Manifest> manifests = new ArrayList<Manifest>();
-        manifests.add(getManifest(getIonJavaAttributes("1984T", "42.0")));
-        manifests.add(getManifest(getAttributes("1.0")));
-        manifests.add(getManifest(getIonJavaAttributes("1985T", "43.0")));
-
-        thrown.expect(IonException.class);
-        new JarInfo(manifests);
+    public void getProjectVersion() {
+        JarInfo info = new JarInfo();
+        String projectVersion = info.getProjectVersion();
+        // Semantic version MAJOR.MINOR.PATCH with optional -SNAPSHOT suffix
+        // Should always match our project version
+        String projectVersionPattern = "\\d+\\.\\d+\\.\\d+(-SNAPSHOT)?";
+        assertThat(projectVersion, matchesPattern(projectVersionPattern));
     }
 
 }
\ No newline at end of file