ahlner commented 1 year ago

Pull Request

Related issue

Fixes partially #3007

What does this PR do?

Changes function validate_geo_from_json to return a BadLatitudeAndLongitude if lat or lng is a string and not parseable to f64
implemented some unittests
Derived PartialEq for GeoError to use assert_eq! in tests

PR checklist

Please check if your PR fulfills the following requirements:

[x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
[x] Have you read the contributing guidelines?
[x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

ahlner commented 1 year ago

Hi @loiclec,

thank you for the answer. I would prefer to write a test like this benchmark: benches/indexing.rs and amend this PR with tests and a fix. Is there already a (integration) test for this functionality?

loiclec commented 1 year ago

That sounds great! An integration test would be very appreciated 😄 I don't think there is one currently. A good place to write it would be in milli/src/index.rs. You can reference https://github.com/meilisearch/meilisearch/issues/3007 in it. The integration tests usually look like this:

#[test]
fn bug_3007() { // or a more descriptive name
    let index = TempIndex::new();

    index
        .update_settings(|settings| {
            // set the index's settings
        }).unwrap();

    // add some documents. Here you wouldn't unwrap, but check the content of the error instead.
    index.add_documents(documents!({ "a" : "id"})).unwrap();
}

We like to use inline snapshot tests with insta when we can, but you don't have to use it if you don't want to.

ahlner commented 1 year ago

Hi @loiclec,

I've reworked the fix, please review again ;)

ahlner commented 1 year ago

The test "update::index_documents::tests::index_all_flavour_of_geo" is failing now. The format of the longitude ({ "id": 0, "_geo": { "lat": 31, "lng": [42] } }) is now unacceptable - validate_geo_from_json is now used and it doesn't accept this format. Either we change for this format validate_geo_from_json or the format is declared technically invalid. Your decision, I'm ready for all crimes. :D

ahlner commented 1 year ago

Another test fails now: update::index_documents::tests::geo_error

UserError(InvalidGeoField(MissingLatitude { document_id: Number(0) })) vs UserError(InvalidGeoField(MissingLatitude { document_id: String("\"0\"") }))

The first one comes from (the wrong check) extract_geo_points where the document_id is a Value::Number()

Now it comes from validate_geo_from_json where the document_id is a Value::String() from the debug-impl of DocumentId.

~~@loiclec Can we change the type in DocumentId in Value::Number a slice of u16 or whatever? It seems to be a number but is held in this case as a string.~~

I've found a more convenient way to refactor debug_id to deserialize the id in the same way as if it's loaded (and not auto-generated).

loiclec commented 1 year ago

Oh, that's interesting! As far as I can tell, it's a bug that:

{ "id": 0, "_geo": { "lat": 31, "lng": [42] } }

is still sometimes accepted (doc link). But I'll ping @gmourier to make sure, and also @irevoire to judge the impact of it on the dump feature. IMO, it's the perfect opportunity to fix it since we are going to release meilisearch v1.0 very soon :-)

Then, after you delete the whole index_all_flavour_of_geo test, the PR is ready to be merged 😄

ahlner commented 1 year ago

Ok, perfekt. There was an formatting issue, I've (hopefully) fixed that.