Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Throw an error on unknown fields specified in the _geo field #772

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

irevoire
Copy link
Member

@irevoire irevoire added bug Something isn't working API breaking The related changes break the milli API labels Jan 24, 2023
@irevoire irevoire requested a review from loiclec January 24, 2023 11:21
Copy link
Contributor

@loiclec loiclec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank youuuu :)

documents!({ "id" : "doggo", "_geo": { "lat": 1, "lng": 2, "doggo": "are the best" }}),
)
.unwrap_err();
insta::assert_display_snapshot!(err, @r###"The `_geo` field in the document with the id: `"\"doggo\""` contains the following unexpected fields: `{"doggo":"are the best"}`."###);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`{"doggo":"are the best"}`

This could be a bit confusing, because this object appears nowhere in the original document. I don't know what the alternative should be though, and it's just an error message, so not a big deal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I thought of displaying only a list of field name, but then I found it a bit confusing as well when it comes to more complex values 😩

I guess we’ll wait and see, if people complains about this message we’ll change it.

@irevoire
Copy link
Member Author

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 24, 2023

Build succeeded:

@bors bors bot merged commit 4e4d8df into main Jan 24, 2023
@bors bors bot deleted the error-on-extra-field-in-geo branch January 24, 2023 12:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API breaking The related changes break the milli API bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants