Skip to content

Commit

Permalink
Review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
@cmnbroad
cmnbroad committed Apr 8, 2024
1 parent 3f0d79d commit fd2f330
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 61 deletions.
32 changes: 23 additions & 9 deletions src/main/java/htsjdk/beta/io/bundle/Bundle.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -34,13 +30,15 @@
* <p>
* 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<BundleResource>, 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<String, BundleResource> resources = new HashMap<>(); // content type -> resource
// content type -> resource. Note that the order of resources is not preserved when round tripping
// through JSON.
private final Map<String, BundleResource> resources = new LinkedHashMap<>(); // content type -> resource
private final String primaryContentType;

/**
Expand All @@ -55,7 +53,9 @@ public Bundle(final String primaryContentType, final Collection<BundleResource>
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)) {
Expand Down Expand Up @@ -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<BundleResource> bundle1Set = new HashSet<>(bundle1.getResources());
final HashSet<BundleResource> bundle2Set = new HashSet<>(bundle2.getResources());
return bundle1Set.equals(bundle2Set);
}
}
60 changes: 24 additions & 36 deletions src/main/java/htsjdk/beta/io/bundle/BundleJSON.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<String> 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<String> 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
Expand Down Expand Up @@ -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<Bundle> bundles) {
public static String toJSON(final List<Bundle> 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)
Expand Down Expand Up @@ -120,35 +112,34 @@ public static <T extends IOPath> 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<Bundle> bundles = toBundleCollection(jsonString, ioPathConstructor);
final List<Bundle> 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<Bundle> from a jsonString, using a custom class that implements {@link IOPath} for all
* Create a List<Bundle> 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<Bundle>
* @return List<Bundle>
* @param <T> IOPath-derived class to use for IOPathResources
*/
public static <T extends IOPath> Collection<Bundle> toBundleCollection(
public static <T extends IOPath> List<Bundle> toBundleList(
final String jsonString,
final Function<String, T> ioPathConstructor) {
ValidationUtils.nonEmpty(jsonString, "json bundle string");
Expand Down Expand Up @@ -177,20 +168,20 @@ public static <T extends IOPath> Collection<Bundle> 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<Bundle> from a jsonString.
* Create a List<Bundle> 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<Bundle>} created from a Collection of jsonStrings
* @return a {@link List<Bundle>} created from a Collection of jsonStrings
*/
public static Collection<Bundle> toBundleCollection(final String jsonString) {
return toBundleCollection(jsonString, HtsPath::new);
public static List<Bundle> toBundleList(final String jsonString) {
return toBundleList(jsonString, HtsPath::new);
}

private static <T extends IOPath> Bundle toBundle(
Expand All @@ -213,23 +204,20 @@ private static <T extends IOPath> Bundle toBundle(

final String primaryContentType = getRequiredPropertyAsString(jsonObject, JSON_PROPERTY_PRIMARY);
final Collection<BundleResource> 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 <T extends IOPath> IOPathResource toBundleResource(
private static <T extends IOPath> IOPathResource toBundleResource(
final String contentType,
final JSONObject jsonObject,
final Function<String, T> ioPathConstructor) {
final String format = jsonObject.optString(JSON_PROPERTY_FORMAT, null);
return new IOPathResource(
ioPathConstructor.apply(getRequiredPropertyAsString(jsonObject, JSON_PROPERTY_PATH)),
contentType,
format == null ? null : format);
format);
}
private static <T extends IOPath> Collection<BundleResource> toBundleResources(
final JSONObject jsonResources,
Expand Down
13 changes: 6 additions & 7 deletions src/test/java/htsjdk/beta/io/bundle/BundleJSONTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -202,8 +201,8 @@ public void testAcceptSingleBundleJSONAsCollection(
final String jsonString,
final String primaryKey,
final List<BundleResource> resources) {
final Collection<Bundle> expectedBundleCollection = Arrays.asList(new Bundle(primaryKey, resources));
final Collection<Bundle> actualBundleCollection = BundleJSON.toBundleCollection(jsonString);
final List<Bundle> expectedBundleCollection = Arrays.asList(new Bundle(primaryKey, resources));
final List<Bundle> actualBundleCollection = BundleJSON.toBundleList(jsonString);
Assert.assertEquals(actualBundleCollection, expectedBundleCollection);
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -454,19 +453,19 @@ public void testRoundTripJSONCollection(
final String jsonString,
final List<Bundle> bundles) {
// create a bundle collection from the input JSON, make sure if equals the test collection
final Collection<Bundle> bundlesFromJSON = BundleJSON.toBundleCollection(jsonString);
final List<Bundle> 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<Bundle> bundlesFromRoundtripJSON = BundleJSON.toBundleCollection(actualJSONString);
final List<Bundle> 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;
Expand Down
6 changes: 6 additions & 0 deletions src/test/java/htsjdk/beta/io/bundle/BundleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;

// Example JSON :
Expand Down Expand Up @@ -84,4 +85,9 @@ public void testResourceIterator() {
}
}

@Test(expectedExceptions = IllegalArgumentException.class)
public void testRejectEmptyBundle() {
new Bundle(BundleResourceType.ALIGNED_READS, Collections.EMPTY_LIST);
}

}
12 changes: 3 additions & 9 deletions src/test/java/htsjdk/beta/plugin/reads/ReadsBundleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -138,8 +134,7 @@ public void testReadWriteRoundTrip(
final String jsonString,
final ReadsBundle<IOPath> expectedReadsBundle) {
final ReadsBundle<IOPath> 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());
}
Expand All @@ -152,8 +147,7 @@ public void testGetReadsBundleFromPath(
IOPathUtils.writeStringToPath(jsonFilePath, jsonString);
final ReadsBundle<IOPath> 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());
}
Expand Down

0 comments on commit fd2f330

Please sign in to comment.