Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
etorres opened this issue Aug 26, 2015 · 4 comments

Comments

@etorres
Copy link

etorres commented Aug 26, 2015

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
Copy link
Author

etorres commented Aug 26, 2015

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
Copy link
Contributor

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
Copy link
Author

etorres commented Aug 26, 2015

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
Copy link

Is this issue fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants