From aa44a439b2deaec352a25dacf60605b67734e7af Mon Sep 17 00:00:00 2001 From: Chris Norman Date: Mon, 18 Mar 2024 14:38:36 -0400 Subject: [PATCH] Review comments. --- .../java/htsjdk/beta/io/bundle/Bundle.java | 32 +++++++--- .../htsjdk/beta/io/bundle/BundleJSON.java | 62 ++++++++----------- .../htsjdk/beta/plugin/reads/ReadsBundle.java | 2 +- .../htsjdk/beta/io/bundle/BundleJSONTest.java | 13 ++-- .../htsjdk/beta/io/bundle/BundleTest.java | 6 ++ .../beta/plugin/reads/ReadsBundleTest.java | 12 +--- 6 files changed, 64 insertions(+), 63 deletions(-) diff --git a/src/main/java/htsjdk/beta/io/bundle/Bundle.java b/src/main/java/htsjdk/beta/io/bundle/Bundle.java index 9efe52e45b..06ef7d12dd 100644 --- a/src/main/java/htsjdk/beta/io/bundle/Bundle.java +++ b/src/main/java/htsjdk/beta/io/bundle/Bundle.java @@ -4,11 +4,7 @@ import htsjdk.utils.ValidationUtils; import java.io.Serializable; -import java.util.Collection; -import java.util.HashMap; -import java.util.Iterator; -import java.util.Map; -import java.util.Optional; +import java.util.*; /** * An immutable collection of related resources, including a (single, required) primary resource, such as "reads", @@ -34,13 +30,15 @@ *

* Bundles that contain only serializable ({@link IOPathResource}) resources may be serialized to, and * deserialized from JSON. + * + * Note that the order of resources in a bundle is not significant, and is not guaranteed to be preserved. */ public class Bundle implements Iterable, Serializable { private static final long serialVersionUID = 1L; - // don't use LinkedHashMap here; using HashMap resolves unnatural resource ordering issues that arise - // when creating a bundle from serialized files or strings - private final Map resources = new HashMap<>(); // content type -> resource + // Note that this uses LinkedHashMap to preserve the input order of the resources, + // but that order is not preserved when round tripping through JSON. + private final Map resources = new LinkedHashMap<>(); // content type -> resource private final String primaryContentType; /** @@ -55,7 +53,9 @@ public Bundle(final String primaryContentType, final Collection ValidationUtils.validateArg(primaryContentType.length() > 0, "A non-zero length primary resource content type must be provided"); ValidationUtils.nonNull(resources, "resource collection"); - ValidationUtils.nonEmpty(resources,"resource collection"); + if (resources.isEmpty()) { + throw new IllegalArgumentException("A bundle must contain at least one resource"); + } resources.forEach(r -> { if (null != this.resources.putIfAbsent(r.getContentType(), r)) { @@ -155,4 +155,18 @@ public int hashCode() { public String toString() { return String.format("%s/%d resource(s)", primaryContentType, resources.size()); } + + // compare two bundles for equality without regard for resource order + public static boolean equalsIgnoreOrder(final Bundle bundle1, final Bundle bundle2) { + if (bundle1 == null || bundle2 == null) { + return false; + } else if (!bundle1.getPrimaryContentType().equals(bundle2.getPrimaryContentType())) { + return false; + } else if (bundle1.getResources().size() != bundle2.getResources().size()) { + return false; + } + final HashSet bundle1Set = new HashSet<>(bundle1.getResources()); + final HashSet bundle2Set = new HashSet<>(bundle2.getResources()); + return bundle1Set.equals(bundle2Set); + } } diff --git a/src/main/java/htsjdk/beta/io/bundle/BundleJSON.java b/src/main/java/htsjdk/beta/io/bundle/BundleJSON.java index c24254c8a7..1dfc5e2396 100644 --- a/src/main/java/htsjdk/beta/io/bundle/BundleJSON.java +++ b/src/main/java/htsjdk/beta/io/bundle/BundleJSON.java @@ -10,8 +10,6 @@ import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Optional; import java.util.Set; @@ -36,14 +34,8 @@ public class BundleJSON { public static final String JSON_SCHEMA_NAME = "htsbundle"; public static final String JSON_SCHEMA_VERSION = "0.1.0"; // TODO: bump this to 1.0.0 - final private static Set TOP_LEVEL_PROPERTIES = Collections.unmodifiableSet( - new HashSet<>() { - private static final long serialVersionUID = 1L; - { - add(JSON_PROPERTY_SCHEMA_NAME); - add(JSON_PROPERTY_SCHEMA_VERSION); - add(JSON_PROPERTY_PRIMARY); - }}); + final private static Set TOP_LEVEL_PROPERTIES = + Set.of(JSON_PROPERTY_SCHEMA_NAME, JSON_PROPERTY_SCHEMA_VERSION, JSON_PROPERTY_PRIMARY); /** * Serialize a bundle to a JSON string representation. All resources in the bundle must @@ -77,14 +69,14 @@ public static String toJSON(final Bundle bundle) { } /** - * Convert a (non-empty) Collection of Bundles to a JSON array string representation. - * @param bundles a Collection of Bundles to serialize to JSON - * @return a JSON string (array) representation of the collection of bundles - * @throw IllegalArgumentException if the collection is empty + * Convert a (non-empty) List of Bundles to a JSON array string representation. + * @param bundles a List of Bundles to serialize to JSON + * @return a JSON string (array) representation of the list of bundles + * @throw IllegalArgumentException if the list is empty */ - public static String toJSON(final Collection bundles) { + public static String toJSON(final List bundles) { if (bundles.isEmpty()) { - throw new IllegalArgumentException("A bundle collection must contain at least one bundle"); + throw new IllegalArgumentException("A bundle list must contain at least one bundle"); } return bundles.stream() .map(BundleJSON::toJSON) @@ -120,35 +112,34 @@ public static Bundle toBundle( return toBundle(new JSONObject(jsonString), ioPathConstructor); } catch (JSONException | UnsupportedOperationException e) { // see if the user provided a collection instead of a single bundle, and if so, present it as - // a Bundle as long as it only contains one Bundle + // a Bundle as long as it only contains one BundleResource try { - final Collection bundles = toBundleCollection(jsonString, ioPathConstructor); + final List bundles = toBundleList(jsonString, ioPathConstructor); if (bundles.size() > 1) { throw new IllegalArgumentException( - String.format("A JSON string with more than one bundle was provided but only a single Bundle is allowed", - e.getMessage(), + String.format("A JSON string with more than one bundle was provided but only a single bundle is allowed in this context (%s)", e.getMessage())); } return bundles.stream().findFirst().get(); } catch (JSONException | UnsupportedOperationException e2) { throw new IllegalArgumentException( - String.format("JSON can be interpreted neither as an individual bundle (%s) nor as a bundle collection (%s)", - e2.getMessage(), - e.getMessage()), + String.format("The JSON can be interpreted neither as an individual bundle (%s) nor as a bundle collection (%s)", + e.getMessage(), + e2.getMessage()), e); } } } /** - * Create a Collection from a jsonString, using a custom class that implements {@link IOPath} for all + * Create a List from a jsonString, using a custom class that implements {@link IOPath} for all * resources. * @param jsonString the json string must conform to the bundle schema, and may contain an array or single object * @param ioPathConstructor constructor to use to create the backing IOPath for all resources - * @return Collection + * @return List * @param IOPath-derived class to use for IOPathResources */ - public static Collection toBundleCollection( + public static List toBundleList( final String jsonString, final Function ioPathConstructor) { ValidationUtils.nonEmpty(jsonString, "json bundle string"); @@ -177,20 +168,20 @@ public static Collection toBundleCollection( e); } } - if (bundles.size() < 1) { + if (bundles.isEmpty()) { throw new IllegalArgumentException("JSON bundle collection must contain at least one bundle"); } return bundles; } /** - * Create a Collection from a jsonString. + * Create a List from a jsonString. * * @param jsonString a JSON strings that conform to the bundle schema; may be an array or single object - * @return a {@link Collection} created from a Collection of jsonStrings + * @return a {@link List} created from a Collection of jsonStrings */ - public static Collection toBundleCollection(final String jsonString) { - return toBundleCollection(jsonString, HtsPath::new); + public static List toBundleList(final String jsonString) { + return toBundleList(jsonString, HtsPath::new); } private static Bundle toBundle( @@ -213,15 +204,12 @@ private static Bundle toBundle( final String primaryContentType = getRequiredPropertyAsString(jsonObject, JSON_PROPERTY_PRIMARY); final Collection bundleResources = toBundleResources(jsonObject, ioPathConstructor); - if (bundleResources.isEmpty()) { - LOG.warn("Empty resource bundle found in: ", jsonObject.toString()); - } return new Bundle(primaryContentType, bundleResources); } catch (JSONException | UnsupportedOperationException e) { throw new IllegalArgumentException(e); } } - private static IOPathResource toBundleResource( + private static IOPathResource toBundleResource( final String contentType, final JSONObject jsonObject, final Function ioPathConstructor) { @@ -229,7 +217,7 @@ private static IOPathResource toBundleResource( return new IOPathResource( ioPathConstructor.apply(getRequiredPropertyAsString(jsonObject, JSON_PROPERTY_PATH)), contentType, - format == null ? null : format); + format); } private static Collection toBundleResources( final JSONObject jsonResources, @@ -249,7 +237,7 @@ private static Collection toBundleResources( return bundleResources; } - private static String getRequiredPropertyAsString(JSONObject jsonDocument, String propertyName) { + private static String getRequiredPropertyAsString(final JSONObject jsonDocument, final String propertyName) { final String propertyValue = jsonDocument.optString(propertyName, null); if (propertyValue == null) { throw new IllegalArgumentException( diff --git a/src/main/java/htsjdk/beta/plugin/reads/ReadsBundle.java b/src/main/java/htsjdk/beta/plugin/reads/ReadsBundle.java index 78787387a7..d6770b77ed 100644 --- a/src/main/java/htsjdk/beta/plugin/reads/ReadsBundle.java +++ b/src/main/java/htsjdk/beta/plugin/reads/ReadsBundle.java @@ -195,7 +195,7 @@ private static IOPathResource toInputResource(final String pr // Try to infer the contentType/format, i.e., READS/BAM from an IOPath. Currently this // exists purely to check for logical inconsistencies. It can detect cases that are illogical - // (an IOPath that has format CRAM, but file extension BAM), but it can't determinstically + // (an IOPath that has format CRAM, but file extension BAM), but it can't deterministically // and correctly infer the types in all cases without reproducing all the logic embedded in all the // codecs (i.e., an htsget IOPath ends in ".bam", but has format HTSGET_BAM, not BAM - detecting // that here would require parsing the entire IOPath structure, which is best left to the codecs diff --git a/src/test/java/htsjdk/beta/io/bundle/BundleJSONTest.java b/src/test/java/htsjdk/beta/io/bundle/BundleJSONTest.java index 1243debb74..ea2c65b8c0 100644 --- a/src/test/java/htsjdk/beta/io/bundle/BundleJSONTest.java +++ b/src/test/java/htsjdk/beta/io/bundle/BundleJSONTest.java @@ -12,7 +12,6 @@ import java.io.IOException; import java.io.InputStream; import java.util.Arrays; -import java.util.Collection; import java.util.List; import java.util.Optional; @@ -202,8 +201,8 @@ public void testAcceptSingleBundleJSONAsCollection( final String jsonString, final String primaryKey, final List resources) { - final Collection expectedBundleCollection = Arrays.asList(new Bundle(primaryKey, resources)); - final Collection actualBundleCollection = BundleJSON.toBundleCollection(jsonString); + final List expectedBundleCollection = Arrays.asList(new Bundle(primaryKey, resources)); + final List actualBundleCollection = BundleJSON.toBundleList(jsonString); Assert.assertEquals(actualBundleCollection, expectedBundleCollection); } @@ -334,7 +333,7 @@ public void testRejectInvalidJSON(final String jsonString, final String expected @Test(dataProvider = "invalidBundleJSON", expectedExceptions = IllegalArgumentException.class) public void testRejectInvalidJSONAsCollection(final String jsonString, final String expectedMessageFragment) { try { - BundleJSON.toBundleCollection(jsonString); + BundleJSON.toBundleList(jsonString); } catch (final IllegalArgumentException e) { Assert.assertTrue(e.getMessage().contains(expectedMessageFragment)); throw e; @@ -454,19 +453,19 @@ public void testRoundTripJSONCollection( final String jsonString, final List bundles) { // create a bundle collection from the input JSON, make sure if equals the test collection - final Collection bundlesFromJSON = BundleJSON.toBundleCollection(jsonString); + final List bundlesFromJSON = BundleJSON.toBundleList(jsonString); Assert.assertEquals(bundlesFromJSON, bundles); // now write the test collection of bundles to JSON, and then roundtrip it back to a bundle collection final String actualJSONString = BundleJSON.toJSON(bundles); - final Collection bundlesFromRoundtripJSON = BundleJSON.toBundleCollection(actualJSONString); + final List bundlesFromRoundtripJSON = BundleJSON.toBundleList(actualJSONString); Assert.assertEquals(bundlesFromRoundtripJSON, bundles); } @Test(expectedExceptions = IllegalArgumentException.class) public void testRejectEmptyCollection() { try { - BundleJSON.toBundleCollection("[]"); + BundleJSON.toBundleList("[]"); } catch (final IllegalArgumentException e) { Assert.assertTrue(e.getMessage().contains("JSON bundle collection must contain at least one bundle")); throw e; diff --git a/src/test/java/htsjdk/beta/io/bundle/BundleTest.java b/src/test/java/htsjdk/beta/io/bundle/BundleTest.java index 4ec6f67f00..38bfaa1641 100644 --- a/src/test/java/htsjdk/beta/io/bundle/BundleTest.java +++ b/src/test/java/htsjdk/beta/io/bundle/BundleTest.java @@ -7,6 +7,7 @@ import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; // Example JSON : @@ -84,4 +85,9 @@ public void testResourceIterator() { } } + @Test(expectedExceptions = IllegalArgumentException.class) + public void testRejectEmptyBundle() { + new Bundle(BundleResourceType.ALIGNED_READS, Collections.EMPTY_LIST); + } + } \ No newline at end of file diff --git a/src/test/java/htsjdk/beta/plugin/reads/ReadsBundleTest.java b/src/test/java/htsjdk/beta/plugin/reads/ReadsBundleTest.java index a697808871..04b0271f10 100644 --- a/src/test/java/htsjdk/beta/plugin/reads/ReadsBundleTest.java +++ b/src/test/java/htsjdk/beta/plugin/reads/ReadsBundleTest.java @@ -2,11 +2,7 @@ import htsjdk.HtsjdkTest; import htsjdk.beta.io.IOPathUtils; -import htsjdk.beta.io.bundle.Bundle; -import htsjdk.beta.io.bundle.BundleBuilder; -import htsjdk.beta.io.bundle.BundleJSON; -import htsjdk.beta.io.bundle.BundleResourceType; -import htsjdk.beta.io.bundle.IOPathResource; +import htsjdk.beta.io.bundle.*; import htsjdk.io.HtsPath; import htsjdk.io.IOPath; import org.testng.Assert; @@ -138,8 +134,7 @@ public void testReadWriteRoundTrip( final String jsonString, final ReadsBundle expectedReadsBundle) { final ReadsBundle bundleFromJSON = ReadsBundle.getReadsBundleFromString(jsonString); - Assert.assertEquals(bundleFromJSON, expectedReadsBundle); - Assert.assertEquals(bundleFromJSON.getPrimaryContentType(), expectedReadsBundle.getPrimaryContentType()); + Assert.assertTrue(Bundle.equalsIgnoreOrder(bundleFromJSON, expectedReadsBundle)); Assert.assertTrue(bundleFromJSON.getReads().getIOPath().isPresent()); Assert.assertEquals(bundleFromJSON.getReads().getIOPath().get(), expectedReadsBundle.getReads().getIOPath().get()); } @@ -152,8 +147,7 @@ public void testGetReadsBundleFromPath( IOPathUtils.writeStringToPath(jsonFilePath, jsonString); final ReadsBundle bundleFromPath = ReadsBundle.getReadsBundleFromPath(jsonFilePath); - Assert.assertEquals(bundleFromPath, expectedReadsBundle); - Assert.assertEquals(bundleFromPath.getPrimaryContentType(), expectedReadsBundle.getPrimaryContentType()); + Assert.assertTrue(Bundle.equalsIgnoreOrder(bundleFromPath, expectedReadsBundle)); Assert.assertTrue(bundleFromPath.getReads().getIOPath().isPresent()); Assert.assertEquals(bundleFromPath.getReads().getIOPath().get(), expectedReadsBundle.getReads().getIOPath().get()); }