opendatalab-de / geojson-jackson

GeoJson POJOs for Jackson - serialize and deserialize objects with ease
http://blog.opendatalab.de
Apache License 2.0
263 stars 94 forks source link

Potential fail of LngLatAlt#equals(Object) since lon/lat values are rounded during serialization #23

Open etorres opened 9 years ago

etorres commented 9 years ago

Double to String conversion in LngLatAlt serializer breaks LngLatAlt#equals(Object) method due to rounded values. Please, take into account the following test where a new point is created from real longitude and latitude values, converted into JSON and read again into a Point object:

package example;

import com.fasterxml.jackson.databind.ObjectMapper;
import org.geojson.Point;

import static org.apache.commons.lang3.StringUtils.trim;
import static org.junit.Assert.fail;
import static org.hamcrest.CoreMatchers.allOf;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.MatcherAssert.assertThat;

public class JsonTest {
    @Test
    public void test() {
        try {
            Point point = new Point(-0.3762881000000107d, 39.4699075d);
            assertThat("original point is not null", point, notNullValue());

            ObjectMapper mapper = new ObjectMapper();
            assertThat("mapper is not null", mapper, notNullValue());

            String payload = mapper.writeValueAsString(point);
            assertThat("JSON is not empty", trim(payload), allOf(notNullValue(), not(equalTo(""))));

            Point point2 = mapper.readValue(payload, Point.class);
            assertThat("new point is not null", point2, notNullValue());

            assertThat("new point coincides with original", point2, equalTo(point));
        } catch (Exception e) {
            e.printStackTrace(System.err);
            fail("Test failed: " + e.getMessage());
        }
    }
}

The error message:

java.lang.AssertionError: new point coincides with original
Expected: <Point{coordinates=LngLatAlt{longitude=-0.3762881000000107, latitude=39.4699075, altitude=NaN}} GeoJsonObject{}>
     but: was <Point{coordinates=LngLatAlt{longitude=-0.3762881, latitude=39.4699075, altitude=NaN}} GeoJsonObject{}>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at example.JsonTest.test(JsonTest.java:32)
etorres commented 9 years ago

I'm posting a possible solution using a bi-function to compare coordinate values with a tolerance for nearly-equal values:

import java.util.function.BiFunction;
import static com.google.common.math.DoubleMath.fuzzyEquals;

public final class GeoJsons {

    public static final double TOLERANCE = 0.00000001d;

    public static final BiFunction<Point, Point, Boolean> POINT_FUZZY_EQUALS = (p1, p2) -> {
        if (p1 == p2) return true;
        else if (p1 == null || p2 == null) return false;
        final LngLatAlt c1 = p1.getCoordinates();
        final LngLatAlt c2 = p2.getCoordinates();
        if (c1 == c2) return true;
        else if (c1 == null || c2 == null) return false;
        return fuzzyEquals(c1.getLongitude(), c2.getLongitude(), TOLERANCE)
                && fuzzyEquals(c1.getLatitude(), c2.getLatitude(), TOLERANCE)
                && fuzzyEquals(c1.getAltitude(), c2.getAltitude(), TOLERANCE);
    };
}
twillouer commented 9 years ago

Hi,

you can take a look to AssertJ (http://joel-costigliola.github.io/assertj/) who is a better remplacement of Hamcrest.

Assertions.assertThat(12D).isEqualTo(12.1, Offset.offset(0.1))

etorres commented 9 years ago

Hi, many thanks for your response and, definitely, many thanks for the tip. I didn't know about this test library. Anyway, I think that decimal places should not be trimmed during serialization, or at least a larger precision should be allowed. BTW, in my previous message I propose a replacement for the equals method that can be used to check two points that are supposed to be nearly-equal, which is a more general solution and can be used beyond basic unit testing.

dev-shaun commented 7 years ago

Is this issue fixed?