diff --git a/doc/release-notes/11559-bbox-bigdecimal.md b/doc/release-notes/11559-bbox-bigdecimal.md new file mode 100644 index 00000000000..3ade655fa20 --- /dev/null +++ b/doc/release-notes/11559-bbox-bigdecimal.md @@ -0,0 +1,3 @@ +## Bug ## + +Fixed an issue where geographic bounding box coordinates with more than 5 decimal places could pass validation despite reversed order (due to single-precision float precision loss) and subsequently cause Solr indexing failures with `InvalidShapeException`. Bounding box coordinates are now parsed and compared using `BigDecimal`. diff --git a/src/main/java/edu/harvard/iq/dataverse/DatasetFieldValueValidator.java b/src/main/java/edu/harvard/iq/dataverse/DatasetFieldValueValidator.java index 74d3cbf73f0..664a3e529ea 100644 --- a/src/main/java/edu/harvard/iq/dataverse/DatasetFieldValueValidator.java +++ b/src/main/java/edu/harvard/iq/dataverse/DatasetFieldValueValidator.java @@ -6,6 +6,7 @@ package edu.harvard.iq.dataverse; import edu.harvard.iq.dataverse.DatasetFieldType.FieldType; +import java.math.BigDecimal; import java.text.ParseException; import java.text.SimpleDateFormat; import java.util.*; @@ -275,11 +276,13 @@ public static boolean validateBoundingBox(final String westLon, final String eas boolean returnVal = false; try { - Float west = verifyBoundingBoxCoordinatesWithinRange(DatasetFieldConstant.westLongitude, westLon); - Float east = verifyBoundingBoxCoordinatesWithinRange(DatasetFieldConstant.eastLongitude, eastLon); - Float north = verifyBoundingBoxCoordinatesWithinRange(DatasetFieldConstant.northLatitude, northLat); - Float south = verifyBoundingBoxCoordinatesWithinRange(DatasetFieldConstant.southLatitude, southLat); - returnVal = west <= east && south <= north; + BigDecimal west = verifyBoundingBoxCoordinatesWithinRange(DatasetFieldConstant.westLongitude, westLon); + BigDecimal east = verifyBoundingBoxCoordinatesWithinRange(DatasetFieldConstant.eastLongitude, eastLon); + BigDecimal north = verifyBoundingBoxCoordinatesWithinRange(DatasetFieldConstant.northLatitude, northLat); + BigDecimal south = verifyBoundingBoxCoordinatesWithinRange(DatasetFieldConstant.southLatitude, southLat); + // Compare with BigDecimal to avoid Float precision loss that let invalid + // high-decimal bounding boxes pass validation and then fail Solr indexing. + returnVal = west.compareTo(east) <= 0 && south.compareTo(north) <= 0; } catch (IllegalArgumentException e) { returnVal = false; } @@ -287,13 +290,25 @@ public static boolean validateBoundingBox(final String westLon, final String eas return returnVal; } - private static Float verifyBoundingBoxCoordinatesWithinRange(final String name, final String value) throws IllegalArgumentException { + private static BigDecimal verifyBoundingBoxCoordinatesWithinRange(final String name, final String value) throws IllegalArgumentException { int max = name.equals(DatasetFieldConstant.westLongitude) || name.equals(DatasetFieldConstant.eastLongitude) ? 180 : 90; int min = max * -1; - final Float returnVal = value != null ? Float.parseFloat(value) : Float.NaN; - if (returnVal.isNaN() || returnVal < min || returnVal > max) { - throw new IllegalArgumentException(String.format("Value (%s) not in range (%s-%s)", returnVal.isNaN() ? "missing" : returnVal, min, max)); + if (value == null) { + throw new IllegalArgumentException(String.format("Value (%s) not in range (%s-%s)", "missing", min, max)); + } + + final BigDecimal returnVal; + try { + returnVal = new BigDecimal(value.trim()); + } catch (NumberFormatException e) { + throw new IllegalArgumentException(String.format("Value (%s) not in range (%s-%s)", value, min, max), e); + } + + final BigDecimal minVal = BigDecimal.valueOf(min); + final BigDecimal maxVal = BigDecimal.valueOf(max); + if (returnVal.compareTo(minVal) < 0 || returnVal.compareTo(maxVal) > 0) { + throw new IllegalArgumentException(String.format("Value (%s) not in range (%s-%s)", returnVal, min, max)); } return returnVal; } diff --git a/src/main/java/edu/harvard/iq/dataverse/search/IndexServiceBean.java b/src/main/java/edu/harvard/iq/dataverse/search/IndexServiceBean.java index 8132c5e113d..20db5a6fee7 100644 --- a/src/main/java/edu/harvard/iq/dataverse/search/IndexServiceBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/search/IndexServiceBean.java @@ -52,6 +52,7 @@ import edu.harvard.iq.dataverse.util.SystemConfig; import java.io.IOException; import java.io.InputStream; +import java.math.BigDecimal; import java.sql.Timestamp; import java.text.SimpleDateFormat; import java.time.LocalDate; @@ -1373,23 +1374,31 @@ public SolrInputDocuments toSolrDocs(IndexableDataset indexableDataset, Set Float.parseFloat(westLon)) { - minWestLon=westLon; - } - if(maxEastLon==null || Float.parseFloat(maxEastLon) < Float.parseFloat(eastLon)) { - maxEastLon=eastLon; - } - if(minSouthLat==null || Float.parseFloat(minSouthLat) > Float.parseFloat(southLat)) { - minSouthLat=southLat; - } - if(maxNorthLat==null || Float.parseFloat(maxNorthLat) < Float.parseFloat(northLat)) { - maxNorthLat=northLat; - } - if (DatasetFieldValueValidator.validateBoundingBox(westLon, eastLon, northLat, southLat)) { //W, E, N, S solrInputDocument.addField(SearchFields.GEOLOCATION, "ENVELOPE(" + westLon + "," + eastLon + "," + northLat + "," + southLat + ")"); + + //Find the overall bounding box that includes all valid bounding boxes + try { + BigDecimal curWest = new BigDecimal(westLon.trim()); + BigDecimal curEast = new BigDecimal(eastLon.trim()); + BigDecimal curSouth = new BigDecimal(southLat.trim()); + BigDecimal curNorth = new BigDecimal(northLat.trim()); + + if (minWestLon == null || new BigDecimal(minWestLon.trim()).compareTo(curWest) > 0) { + minWestLon = westLon; + } + if (maxEastLon == null || new BigDecimal(maxEastLon.trim()).compareTo(curEast) < 0) { + maxEastLon = eastLon; + } + if (minSouthLat == null || new BigDecimal(minSouthLat.trim()).compareTo(curSouth) > 0) { + minSouthLat = southLat; + } + if (maxNorthLat == null || new BigDecimal(maxNorthLat.trim()).compareTo(curNorth) < 0) { + maxNorthLat = northLat; + } + } catch (NumberFormatException ignored) { + } } } } diff --git a/src/test/java/edu/harvard/iq/dataverse/DatasetFieldValueValidatorTest.java b/src/test/java/edu/harvard/iq/dataverse/DatasetFieldValueValidatorTest.java index 3221949384e..9fb44d3da2f 100644 --- a/src/test/java/edu/harvard/iq/dataverse/DatasetFieldValueValidatorTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/DatasetFieldValueValidatorTest.java @@ -174,12 +174,33 @@ public void testBoundingBoxValidity() { assertTrue(DatasetFieldValueValidator.validateBoundingBox("0", "0", "0", "0")); // invalid tests - assertTrue(!DatasetFieldValueValidator.validateBoundingBox("-180", null, "90", null)); - assertTrue(!DatasetFieldValueValidator.validateBoundingBox(null, "180", null, "90")); - assertTrue(!DatasetFieldValueValidator.validateBoundingBox("-180", "180", "90", "junk")); - assertTrue(!DatasetFieldValueValidator.validateBoundingBox("45", "40", "90", "0")); - assertTrue(!DatasetFieldValueValidator.validateBoundingBox("360", "0", "90", "-90")); - assertTrue(!DatasetFieldValueValidator.validateBoundingBox("", "", "", "")); - assertTrue(!DatasetFieldValueValidator.validateBoundingBox(null, null, null, null)); + assertFalse(DatasetFieldValueValidator.validateBoundingBox("-180", null, "90", null)); + assertFalse(DatasetFieldValueValidator.validateBoundingBox(null, "180", null, "90")); + assertFalse(DatasetFieldValueValidator.validateBoundingBox("-180", "180", "90", "junk")); + assertFalse(DatasetFieldValueValidator.validateBoundingBox("45", "40", "90", "0")); + assertFalse(DatasetFieldValueValidator.validateBoundingBox("360", "0", "90", "-90")); + assertFalse(DatasetFieldValueValidator.validateBoundingBox("", "", "", "")); + assertFalse(DatasetFieldValueValidator.validateBoundingBox(null, null, null, null)); + + // High-precision coordinates that reverse order must be rejected. + // Float comparison could treat nearly-equal 6+ decimal values as equal and + // incorrectly allow South>North / West>East boxes that later break Solr (#11559). + assertFalse(DatasetFieldValueValidator.validateBoundingBox("-71.116431", "-71.116430", "42.377000", "42.377001")); + assertFalse(DatasetFieldValueValidator.validateBoundingBox("-71.116430", "-71.116431", "42.377001", "42.377000")); + assertFalse(DatasetFieldValueValidator.validateBoundingBox("0.000001", "0.000000", "0.000001", "0.000000")); + assertFalse(DatasetFieldValueValidator.validateBoundingBox("0.000000", "0.000001", "0.000000", "0.000001")); + + // High-precision out-of-range bounds + assertFalse(DatasetFieldValueValidator.validateBoundingBox("-180.0000001", "180", "90", "-90")); + assertFalse(DatasetFieldValueValidator.validateBoundingBox("-180", "180.0000001", "90", "-90")); + assertFalse(DatasetFieldValueValidator.validateBoundingBox("-180", "180", "90.0000001", "-90")); + assertFalse(DatasetFieldValueValidator.validateBoundingBox("-180", "180", "90", "-90.0000001")); + + // Still valid with many decimals when order is correct + assertTrue(DatasetFieldValueValidator.validateBoundingBox("-71.116431", "-71.116430", "42.377001", "42.377000")); + // High-precision point bounding box (min == max) + assertTrue(DatasetFieldValueValidator.validateBoundingBox("-71.116431", "-71.116431", "42.377000", "42.377000")); + // Leading/trailing whitespace should be trimmed and accepted + assertTrue(DatasetFieldValueValidator.validateBoundingBox(" -71.116431 ", " -71.116430 ", " 42.377001 ", " 42.377000 ")); } } diff --git a/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java b/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java index 1e1aad484e4..96439baca1d 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java @@ -7531,6 +7531,150 @@ public void testGetDatasetWithTermsOfUseAndGuestbook() throws IOException, JsonP .body("data.guestbookId", equalTo(guestbook.getId().intValue())); } + @Test + public void testGeographicBoundingBoxHighPrecisionValidation() { + Response createUser = UtilIT.createRandomUser(); + String apiToken = UtilIT.getApiTokenFromResponse(createUser); + + Response createDataverse = UtilIT.createRandomDataverse(apiToken); + createDataverse.then().assertThat().statusCode(CREATED.getStatusCode()); + String dataverseAlias = UtilIT.getAliasFromResponse(createDataverse); + + Response setMetadataBlocks = UtilIT.setMetadataBlocks(dataverseAlias, JsonUtil.createArrayBuilder().add("citation").add("geospatial"), apiToken); + setMetadataBlocks.then().assertThat().statusCode(OK.getStatusCode()); + + // Negative test: Coordinates with 6+ decimal places where South > North. + // Before #11559 fix, Float.parseFloat treated 42.001001f and 42.001000f as equal (both 42.001f), + // erroneously passing validation and later breaking Solr indexing with InvalidShapeException. + JsonObjectBuilder invalidBboxDataset = createDatasetJsonWithBoundingBox( + "-71.116431", "-71.116430", "42.001001", "42.001000" + ); + Response invalidResponse = UtilIT.createDataset(dataverseAlias, invalidBboxDataset, apiToken); + invalidResponse.prettyPrint(); + invalidResponse.then().assertThat() + .statusCode(BAD_REQUEST.getStatusCode()) + .body("message", containsString("invalid coordinates")); + + // Positive test: High-precision coordinates with valid ordering (South <= North, West <= East) + JsonObjectBuilder validBboxDataset = createDatasetJsonWithBoundingBox( + "-71.116431", "-71.116430", "42.001000", "42.001001" + ); + Response validResponse = UtilIT.createDataset(dataverseAlias, validBboxDataset, apiToken); + validResponse.prettyPrint(); + validResponse.then().assertThat().statusCode(CREATED.getStatusCode()); + } + + private JsonObjectBuilder createDatasetJsonWithBoundingBox(String west, String east, String south, String north) { + return JsonUtil.createObjectBuilder() + .add("datasetVersion", JsonUtil.createObjectBuilder() + .add("metadataBlocks", JsonUtil.createObjectBuilder() + .add("citation", JsonUtil.createObjectBuilder() + .add("fields", JsonUtil.createArrayBuilder() + .add(JsonUtil.createObjectBuilder() + .add("typeName", "title") + .add("value", "Dataset with Bounding Box") + .add("typeClass", "primitive") + .add("multiple", false) + ) + .add(JsonUtil.createObjectBuilder() + .add("value", JsonUtil.createArrayBuilder() + .add(JsonUtil.createObjectBuilder() + .add("authorName", + JsonUtil.createObjectBuilder() + .add("value", "Tester, Geo") + .add("typeClass", "primitive") + .add("multiple", false) + .add("typeName", "authorName")) + ) + ) + .add("typeClass", "compound") + .add("multiple", true) + .add("typeName", "author") + ) + .add(JsonUtil.createObjectBuilder() + .add("value", JsonUtil.createArrayBuilder() + .add(JsonUtil.createObjectBuilder() + .add("datasetContactEmail", + JsonUtil.createObjectBuilder() + .add("value", "geotester@mailinator.com") + .add("typeClass", "primitive") + .add("multiple", false) + .add("typeName", "datasetContactEmail")) + ) + ) + .add("typeClass", "compound") + .add("multiple", true) + .add("typeName", "datasetContact") + ) + .add(JsonUtil.createObjectBuilder() + .add("value", JsonUtil.createArrayBuilder() + .add(JsonUtil.createObjectBuilder() + .add("dsDescriptionValue", + JsonUtil.createObjectBuilder() + .add("value", "Dataset for geospatial bbox test.") + .add("typeClass", "primitive") + .add("multiple", false) + .add("typeName", "dsDescriptionValue")) + ) + ) + .add("typeClass", "compound") + .add("multiple", true) + .add("typeName", "dsDescription") + ) + .add(JsonUtil.createObjectBuilder() + .add("value", JsonUtil.createArrayBuilder() + .add("Other") + ) + .add("typeClass", "controlledVocabulary") + .add("multiple", true) + .add("typeName", "subject") + ) + ) + ) + .add("geospatial", JsonUtil.createObjectBuilder() + .add("fields", JsonUtil.createArrayBuilder() + .add(JsonUtil.createObjectBuilder() + .add("typeName", "geographicBoundingBox") + .add("typeClass", "compound") + .add("multiple", true) + .add("value", JsonUtil.createArrayBuilder() + .add(JsonUtil.createObjectBuilder() + .add("westLongitude", + JsonUtil.createObjectBuilder() + .add("value", west) + .add("typeClass", "primitive") + .add("multiple", false) + .add("typeName", "westLongitude") + ) + .add("southLatitude", + JsonUtil.createObjectBuilder() + .add("value", south) + .add("typeClass", "primitive") + .add("multiple", false) + .add("typeName", "southLatitude") + ) + .add("eastLongitude", + JsonUtil.createObjectBuilder() + .add("value", east) + .add("typeClass", "primitive") + .add("multiple", false) + .add("typeName", "eastLongitude") + ) + .add("northLatitude", + JsonUtil.createObjectBuilder() + .add("value", north) + .add("typeClass", "primitive") + .add("multiple", false) + .add("typeName", "northLatitude") + ) + ) + ) + ) + ) + ) + )); + } + private String getSuperuserToken() { Response createResponse = UtilIT.createRandomUser(); String adminApiToken = UtilIT.getApiTokenFromResponse(createResponse); diff --git a/src/test/java/edu/harvard/iq/dataverse/search/IndexServiceBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/search/IndexServiceBeanTest.java index eda9b995db5..24aed2afad5 100644 --- a/src/test/java/edu/harvard/iq/dataverse/search/IndexServiceBeanTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/search/IndexServiceBeanTest.java @@ -35,6 +35,7 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.LinkedList; import java.util.List; @@ -43,6 +44,7 @@ import java.util.logging.Logger; import java.util.stream.Collectors; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @LocalJvmSettings @@ -115,6 +117,51 @@ public void testValidateBoundingBox() throws SolrServerException, IOException { assertTrue(!doc.get().containsKey("boundingBox")); } + @Test + public void testBoundingBoxAggregationHighPrecision() throws SolrServerException, IOException { + final IndexableDataset indexableDataset = createIndexableDataset(); + final DatasetVersion datasetVersion = indexableDataset.getDatasetVersion(); + DatasetField dsf = new DatasetField(); + DatasetFieldType dsft = new DatasetFieldType(DatasetFieldConstant.geographicBoundingBox, DatasetFieldType.FieldType.TEXT, true); + dsf.setDatasetFieldType(dsft); + + List vals = new LinkedList<>(); + // Box 1: Valid high precision + vals.add(constructBoundingBoxCompoundValue(dsf, "-71.116431", "-71.116420", "42.377010", "42.377000")); + // Box 2: Valid high precision expanding the envelope in all four directions + vals.add(constructBoundingBoxCompoundValue(dsf, "-71.116435", "-71.116415", "42.377015", "42.376990")); + // Box 3: Invalid high precision (South > North at 6th decimal) - must NOT be aggregated into boundingBox nor indexed in geolocation + vals.add(constructBoundingBoxCompoundValue(dsf, "-71.116440", "-71.116410", "42.001000", "42.001001")); + + dsf.setDatasetFieldCompoundValues(vals); + datasetVersion.getDatasetFields().add(dsf); + + final SolrInputDocuments docs = indexService.toSolrDocs(indexableDataset, null); + Optional doc = docs.getDocuments().stream().findFirst(); + assertTrue(doc.isPresent()); + + // Overall boundingBox envelope must aggregate only valid boxes with exact BigDecimal precision + assertEquals("ENVELOPE(-71.116435,-71.116415,42.377015,42.376990)", doc.get().getFieldValue(SearchFields.BOUNDING_BOX)); + + // Geolocation must contain only the 2 valid bounding boxes + Collection geolocations = doc.get().getFieldValues(SearchFields.GEOLOCATION); + assertEquals(2, geolocations.size()); + assertTrue(geolocations.contains("ENVELOPE(-71.116431,-71.116420,42.377010,42.377000)")); + assertTrue(geolocations.contains("ENVELOPE(-71.116435,-71.116415,42.377015,42.376990)")); + } + + private DatasetFieldCompoundValue constructBoundingBoxCompoundValue(DatasetField parent, String west, String east, String north, String south) { + DatasetFieldCompoundValue val = new DatasetFieldCompoundValue(); + val.setParentDatasetField(parent); + val.setChildDatasetFields(Arrays.asList( + constructBoundingBoxValue(DatasetFieldConstant.westLongitude, west), + constructBoundingBoxValue(DatasetFieldConstant.eastLongitude, east), + constructBoundingBoxValue(DatasetFieldConstant.northLatitude, north), + constructBoundingBoxValue(DatasetFieldConstant.southLatitude, south) + )); + return val; + } + private DatasetField constructBoundingBoxValue(String datasetFieldTypeName, String value) { DatasetField retVal = new DatasetField(); retVal.setDatasetFieldType(new DatasetFieldType(datasetFieldTypeName, DatasetFieldType.FieldType.TEXT, false));