846: Change settings behavior r=LegendreM a=MarinPostma

partially implements #824.

Returning the field distribution for all know fields is more complicated that anticipated, see https://github.com/meilisearch/MeiliSearch/issues/824#issuecomment-657656561

If we decide to to it anyway, and find a reasonable solution, I will make another PR.

fix #853 by resetting displayed and searchable attributes to wildcard when attributes are set to `[]` in the all settings route. @curquiza @bidoubiwa can you confirm me that this is the expected behavior?

Co-authored-by: mpostma <postma.marin@protonmail.com>
This commit is contained in:
bors[bot] 2020-07-16 14:31:06 +00:00 committed by GitHub
commit 7dc628965c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 272 additions and 590 deletions

View file

@ -1,6 +1,7 @@
use actix_web::{web, HttpResponse};
use actix_web_macros::{delete, get, post};
use meilisearch_core::settings::{Settings, SettingsUpdate, UpdateState, DEFAULT_RANKING_RULES};
use meilisearch_schema::Schema;
use std::collections::{BTreeMap, BTreeSet, HashSet};
use crate::error::{Error, ResponseError};
@ -24,8 +25,6 @@ pub fn services(cfg: &mut web::ServiceConfig) {
.service(get_displayed)
.service(update_displayed)
.service(delete_displayed)
.service(get_accept_new_fields)
.service(update_accept_new_fields)
.service(get_attributes_for_faceting)
.service(delete_attributes_for_faceting)
.service(update_attributes_for_faceting);
@ -108,23 +107,8 @@ async fn get_all(
_ => vec![],
};
println!("{:?}", attributes_for_faceting);
let searchable_attributes = schema.as_ref().map(|s| {
s.indexed_name()
.iter()
.map(|s| s.to_string())
.collect::<Vec<String>>()
});
let displayed_attributes = schema.as_ref().map(|s| {
s.displayed_name()
.iter()
.map(|s| s.to_string())
.collect::<HashSet<String>>()
});
let accept_new_fields = schema.map(|s| s.accept_new_fields());
let searchable_attributes = schema.as_ref().map(get_indexed_attributes);
let displayed_attributes = schema.as_ref().map(get_displayed_attributes);
let settings = Settings {
ranking_rules: Some(Some(ranking_rules)),
@ -133,7 +117,6 @@ async fn get_all(
displayed_attributes: Some(displayed_attributes),
stop_words: Some(Some(stop_words)),
synonyms: Some(Some(synonyms)),
accept_new_fields: Some(accept_new_fields),
attributes_for_faceting: Some(Some(attributes_for_faceting)),
};
@ -158,7 +141,6 @@ async fn delete_all(
displayed_attributes: UpdateState::Clear,
stop_words: UpdateState::Clear,
synonyms: UpdateState::Clear,
accept_new_fields: UpdateState::Clear,
attributes_for_faceting: UpdateState::Clear,
};
@ -326,7 +308,7 @@ async fn get_searchable(
let reader = data.db.main_read_txn()?;
let schema = index.main.schema(&reader)?;
let searchable_attributes: Option<Vec<String>> =
schema.map(|s| s.indexed_name().iter().map(|i| i.to_string()).collect());
schema.as_ref().map(get_indexed_attributes);
Ok(HttpResponse::Ok().json(searchable_attributes))
}
@ -396,8 +378,7 @@ async fn get_displayed(
let schema = index.main.schema(&reader)?;
let displayed_attributes: Option<HashSet<String>> =
schema.map(|s| s.displayed_name().iter().map(|i| i.to_string()).collect());
let displayed_attributes = schema.as_ref().map(get_displayed_attributes);
Ok(HttpResponse::Ok().json(displayed_attributes))
}
@ -450,52 +431,6 @@ async fn delete_displayed(
Ok(HttpResponse::Accepted().json(IndexUpdateResponse::with_id(update_id)))
}
#[get(
"/indexes/{index_uid}/settings/accept-new-fields",
wrap = "Authentication::Private"
)]
async fn get_accept_new_fields(
data: web::Data<Data>,
path: web::Path<IndexParam>,
) -> Result<HttpResponse, ResponseError> {
let index = data
.db
.open_index(&path.index_uid)
.ok_or(Error::index_not_found(&path.index_uid))?;
let reader = data.db.main_read_txn()?;
let schema = index.main.schema(&reader)?;
let accept_new_fields = schema.map(|s| s.accept_new_fields());
Ok(HttpResponse::Ok().json(accept_new_fields))
}
#[post(
"/indexes/{index_uid}/settings/accept-new-fields",
wrap = "Authentication::Private"
)]
async fn update_accept_new_fields(
data: web::Data<Data>,
path: web::Path<IndexParam>,
body: web::Json<Option<bool>>,
) -> Result<HttpResponse, ResponseError> {
let index = data
.db
.open_index(&path.index_uid)
.ok_or(Error::index_not_found(&path.index_uid))?;
let settings = Settings {
accept_new_fields: Some(body.into_inner()),
..Settings::default()
};
let settings = settings.to_update().map_err(Error::bad_request)?;
let update_id = data.db.update_write(|w| index.settings_update(w, settings))?;
Ok(HttpResponse::Accepted().json(IndexUpdateResponse::with_id(update_id)))
}
#[get(
"/indexes/{index_uid}/settings/attributes-for-faceting",
wrap = "Authentication::Private"
@ -577,3 +512,25 @@ async fn delete_attributes_for_faceting(
Ok(HttpResponse::Accepted().json(IndexUpdateResponse::with_id(update_id)))
}
fn get_indexed_attributes(schema: &Schema) -> Vec<String> {
if schema.is_indexed_all() {
["*"].iter().map(|s| s.to_string()).collect()
} else {
schema.indexed_name()
.iter()
.map(|s| s.to_string())
.collect()
}
}
fn get_displayed_attributes(schema: &Schema) -> HashSet<String> {
if schema.is_displayed_all() {
["*"].iter().map(|s| s.to_string()).collect()
} else {
schema.displayed_name()
.iter()
.map(|s| s.to_string())
.collect()
}
}

View file

@ -112,7 +112,6 @@ impl Server {
"longitude",
"tags",
],
"acceptNewFields": false,
});
server.update_all_settings(body).await;
@ -426,16 +425,6 @@ impl Server {
self.delete_request_async(&url).await
}
pub async fn get_accept_new_fields(&mut self) -> (Value, StatusCode) {
let url = format!("/indexes/{}/settings/accept-new-fields", self.uid);
self.get_request(&url).await
}
pub async fn update_accept_new_fields(&mut self, body: Value) {
let url = format!("/indexes/{}/settings/accept-new-fields", self.uid);
self.post_request_async(&url, body).await;
}
pub async fn get_synonyms(&mut self) -> (Value, StatusCode) {
let url = format!("/indexes/{}/settings/synonyms", self.uid);
self.get_request(&url).await

View file

@ -1195,15 +1195,21 @@ async fn search_with_differents_attributes_8() {
#[actix_rt::test]
async fn test_faceted_search_valid() {
let mut server = common::Server::test_server().await;
// set facetting attributes before adding documents
let mut server = common::Server::with_uid("test");
server.create_index(json!({ "uid": "test" })).await;
// simple tests on attributes with string value
let body = json!({
"attributesForFaceting": ["color"]
});
server.update_all_settings(body).await;
let dataset = include_bytes!("assets/test_set.json");
let body: Value = serde_json::from_slice(dataset).unwrap();
server.add_or_update_multiple_documents(body).await;
// simple tests on attributes with string value
let query = json!({
"q": "a",
"facetFilters": ["color:green"]
@ -1357,8 +1363,8 @@ async fn test_faceted_search_invalid() {
test_post_get_search!(server, query, |response, status_code| {
assert_eq!(status_code, 400);
assert_eq!(response["errorCode"], "invalid_facet");
});
assert_eq!(response["errorCode"], "invalid_facet");
});
let body = json!({
"attributesForFaceting": ["color", "tags"]
@ -1373,7 +1379,7 @@ async fn test_faceted_search_invalid() {
test_post_get_search!(server, query, |response, status_code| {
assert_eq!(status_code, 400);
assert_eq!(response["errorCode"], "invalid_facet");
assert_eq!(response["errorCode"], "invalid_facet");
});
// [[]]
let query = json!({
@ -1383,7 +1389,7 @@ async fn test_faceted_search_invalid() {
test_post_get_search!(server, query, |response, status_code| {
assert_eq!(status_code, 400);
assert_eq!(response["errorCode"], "invalid_facet");
assert_eq!(response["errorCode"], "invalid_facet");
});
// ["color:green", []]
@ -1394,7 +1400,7 @@ async fn test_faceted_search_invalid() {
test_post_get_search!(server, query, |response, status_code| {
assert_eq!(status_code, 400);
assert_eq!(response["errorCode"], "invalid_facet");
assert_eq!(response["errorCode"], "invalid_facet");
});
// too much depth
@ -1406,7 +1412,7 @@ async fn test_faceted_search_invalid() {
test_post_get_search!(server, query, |response, status_code| {
assert_eq!(status_code, 400);
assert_eq!(response["errorCode"], "invalid_facet");
assert_eq!(response["errorCode"], "invalid_facet");
});
// [["color:green", ["color:blue"]]]
@ -1417,7 +1423,7 @@ async fn test_faceted_search_invalid() {
test_post_get_search!(server, query, |response, status_code| {
assert_eq!(status_code, 400);
assert_eq!(response["errorCode"], "invalid_facet");
assert_eq!(response["errorCode"], "invalid_facet");
});
// "color:green"
@ -1428,7 +1434,7 @@ async fn test_faceted_search_invalid() {
test_post_get_search!(server, query, |response, status_code| {
assert_eq!(status_code, 400);
assert_eq!(response["errorCode"], "invalid_facet");
assert_eq!(response["errorCode"], "invalid_facet");
});
}

View file

@ -41,7 +41,6 @@ async fn search_with_settings_basic() {
],
"stopWords": null,
"synonyms": null,
"acceptNewFields": false,
});
server.update_all_settings(config).await;
@ -122,7 +121,6 @@ async fn search_with_settings_stop_words() {
],
"stopWords": ["ea"],
"synonyms": null,
"acceptNewFields": false,
});
server.update_all_settings(config).await;
@ -206,7 +204,6 @@ async fn search_with_settings_synonyms() {
"exercitation"
]
},
"acceptNewFields": false,
});
server.update_all_settings(config).await;
@ -286,7 +283,6 @@ async fn search_with_settings_ranking_rules() {
],
"stopWords": null,
"synonyms": null,
"acceptNewFields": false,
});
server.update_all_settings(config).await;
@ -369,7 +365,6 @@ async fn search_with_settings_searchable_attributes() {
"exercitation"
]
},
"acceptNewFields": false,
});
server.update_all_settings(config).await;
@ -435,7 +430,6 @@ async fn search_with_settings_displayed_attributes() {
],
"stopWords": null,
"synonyms": null,
"acceptNewFields": false,
});
server.update_all_settings(config).await;
@ -502,7 +496,6 @@ async fn search_with_settings_searchable_attributes_2() {
],
"stopWords": null,
"synonyms": null,
"acceptNewFields": false,
});
server.update_all_settings(config).await;

View file

@ -48,7 +48,6 @@ async fn write_all_and_delete() {
"street": ["avenue"],
},
"attributesForFaceting": ["name"],
"acceptNewFields": false,
});
server.update_all_settings(body.clone()).await;
@ -77,46 +76,11 @@ async fn write_all_and_delete() {
"exactness"
],
"distinctAttribute": null,
"searchableAttributes": [
"address",
"id",
"longitude",
"phone",
"age",
"gender",
"latitude",
"email",
"about",
"registered",
"picture",
"isActive",
"tags",
"name",
"color",
"balance"
],
"displayedAttributes": [
"address",
"id",
"longitude",
"phone",
"age",
"gender",
"latitude",
"email",
"about",
"registered",
"picture",
"isActive",
"tags",
"name",
"color",
"balance"
],
"searchableAttributes": ["*"],
"displayedAttributes": ["*"],
"stopWords": [],
"synonyms": {},
"attributesForFaceting": [],
"acceptNewFields": true,
});
assert_json_eq!(expect, response, ordered: false);
@ -168,7 +132,6 @@ async fn write_all_and_update() {
"street": ["avenue"],
},
"attributesForFaceting": ["name"],
"acceptNewFields": false,
});
server.update_all_settings(body.clone()).await;
@ -210,7 +173,6 @@ async fn write_all_and_update() {
"street": ["avenue"],
},
"attributesForFaceting": ["title"],
"acceptNewFields": false,
});
server.update_all_settings(body).await;
@ -248,7 +210,6 @@ async fn write_all_and_update() {
"street": ["avenue"],
},
"attributesForFaceting": ["title"],
"acceptNewFields": false,
});
assert_json_eq!(expected, response, ordered: false);
@ -274,12 +235,11 @@ async fn test_default_settings() {
"exactness"
],
"distinctAttribute": null,
"searchableAttributes": [],
"displayedAttributes": [],
"searchableAttributes": ["*"],
"displayedAttributes": ["*"],
"stopWords": [],
"synonyms": {},
"attributesForFaceting": [],
"acceptNewFields": true,
});
let (response, _status_code) = server.get_all_settings().await;
@ -308,16 +268,11 @@ async fn test_default_settings_2() {
"exactness"
],
"distinctAttribute": null,
"searchableAttributes": [
"id"
],
"displayedAttributes": [
"id"
],
"searchableAttributes": ["*"],
"displayedAttributes": ["*"],
"stopWords": [],
"synonyms": {},
"attributesForFaceting": [],
"acceptNewFields": true,
});
let (response, _status_code) = server.get_all_settings().await;
@ -381,7 +336,6 @@ async fn write_setting_and_update_partial() {
"road": ["street", "avenue"],
"street": ["avenue"],
},
"acceptNewFields": false,
});
server.update_all_settings(body.clone()).await;
@ -427,7 +381,6 @@ async fn write_setting_and_update_partial() {
"street": ["avenue"],
},
"attributesForFaceting": [],
"acceptNewFields": false,
});
let (response, _status_code) = server.get_all_settings().await;
@ -469,17 +422,49 @@ async fn setting_ranking_rules_dont_mess_with_other_settings() {
}
#[actix_rt::test]
async fn distinct_attribute_recorded_as_known_field() {
let mut server = common::Server::test_server().await;
let body = json!({
"distinctAttribute": "foobar",
"acceptNewFields": true
});
server.update_all_settings(body).await;
let document = json!([{"id": 9348127, "foobar": "hello", "foo": "bar"}]);
server.add_or_update_multiple_documents(document).await;
// foobar should not be added to the searchable attributes because it is already known, but "foo" should
let (response, _) = server.get_all_settings().await;
assert!(response["searchableAttributes"].as_array().unwrap().iter().any(|v| v.as_str().unwrap() == "foo"));
assert!(!response["searchableAttributes"].as_array().unwrap().iter().any(|v| v.as_str().unwrap() == "foobar"));
async fn displayed_and_searchable_attributes_reset_to_wildcard() {
let mut server = common::Server::test_server().await;
server.update_all_settings(json!({ "searchableAttributes": ["color"], "displayedAttributes": ["color"] })).await;
let (response, _) = server.get_all_settings().await;
assert_eq!(response["searchableAttributes"].as_array().unwrap()[0], "color");
assert_eq!(response["displayedAttributes"].as_array().unwrap()[0], "color");
server.delete_searchable_attributes().await;
server.delete_displayed_attributes().await;
let (response, _) = server.get_all_settings().await;
assert_eq!(response["searchableAttributes"].as_array().unwrap().len(), 1);
assert_eq!(response["displayedAttributes"].as_array().unwrap().len(), 1);
assert_eq!(response["searchableAttributes"].as_array().unwrap()[0], "*");
assert_eq!(response["displayedAttributes"].as_array().unwrap()[0], "*");
let mut server = common::Server::test_server().await;
server.update_all_settings(json!({ "searchableAttributes": ["color"], "displayedAttributes": ["color"] })).await;
let (response, _) = server.get_all_settings().await;
assert_eq!(response["searchableAttributes"].as_array().unwrap()[0], "color");
assert_eq!(response["displayedAttributes"].as_array().unwrap()[0], "color");
server.update_all_settings(json!({ "searchableAttributes": [], "displayedAttributes": [] })).await;
let (response, _) = server.get_all_settings().await;
assert_eq!(response["searchableAttributes"].as_array().unwrap().len(), 1);
assert_eq!(response["displayedAttributes"].as_array().unwrap().len(), 1);
assert_eq!(response["searchableAttributes"].as_array().unwrap()[0], "*");
assert_eq!(response["displayedAttributes"].as_array().unwrap()[0], "*");
}
#[actix_rt::test]
async fn settings_that_contains_wildcard_is_wildcard() {
let mut server = common::Server::test_server().await;
server.update_all_settings(json!({ "searchableAttributes": ["color", "*"], "displayedAttributes": ["color", "*"] })).await;
let (response, _) = server.get_all_settings().await;
assert_eq!(response["searchableAttributes"].as_array().unwrap().len(), 1);
assert_eq!(response["displayedAttributes"].as_array().unwrap().len(), 1);
assert_eq!(response["searchableAttributes"].as_array().unwrap()[0], "*");
assert_eq!(response["displayedAttributes"].as_array().unwrap()[0], "*");
}

View file

@ -1,349 +0,0 @@
use assert_json_diff::assert_json_eq;
use serde_json::json;
mod common;
#[actix_rt::test]
async fn index_new_fields_default() {
let mut server = common::Server::with_uid("movies");
let body = json!({
"uid": "movies",
"primaryKey": "id",
});
server.create_index(body).await;
// 1 - Add a document
let body = json!([{
"id": 1,
"title": "I'm a legend",
}]);
server.add_or_replace_multiple_documents(body).await;
// 2 - Get the complete document
let expected = json!({
"id": 1,
"title": "I'm a legend",
});
let (response, status_code) = server.get_document(1).await;
assert_eq!(status_code, 200);
assert_json_eq!(response, expected);
// 3 - Add a document with more fields
let body = json!([{
"id": 2,
"title": "I'm not a legend",
"description": "A bad copy of the original movie I'm a lengend"
}]);
server.add_or_replace_multiple_documents(body).await;
// 4 - Get the complete document
let expected = json!({
"id": 2,
"title": "I'm not a legend",
"description": "A bad copy of the original movie I'm a lengend"
});
let (response, status_code) = server.get_document(2).await;
assert_eq!(status_code, 200);
assert_json_eq!(response, expected);
}
#[actix_rt::test]
async fn index_new_fields_true() {
let mut server = common::Server::with_uid("movies");
let body = json!({
"uid": "movies",
"primaryKey": "id",
});
server.create_index(body).await;
// 1 - Set indexNewFields = true
server.update_accept_new_fields(json!(true)).await;
// 2 - Add a document
let body = json!([{
"id": 1,
"title": "I'm a legend",
}]);
server.add_or_replace_multiple_documents(body).await;
// 3 - Get the complete document
let expected = json!({
"id": 1,
"title": "I'm a legend",
});
let (response, status_code) = server.get_document(1).await;
assert_eq!(status_code, 200);
assert_json_eq!(response, expected);
// 4 - Add a document with more fields
let body = json!([{
"id": 2,
"title": "I'm not a legend",
"description": "A bad copy of the original movie I'm a lengend"
}]);
server.add_or_replace_multiple_documents(body).await;
// 5 - Get the complete document
let expected = json!({
"id": 2,
"title": "I'm not a legend",
"description": "A bad copy of the original movie I'm a lengend"
});
let (response, status_code) = server.get_document(2).await;
assert_eq!(status_code, 200);
assert_json_eq!(response, expected);
}
#[actix_rt::test]
async fn index_new_fields_false() {
let mut server = common::Server::with_uid("movies");
let body = json!({
"uid": "movies",
"primaryKey": "id",
});
server.create_index(body).await;
// 1 - Set indexNewFields = false
server.update_accept_new_fields(json!(false)).await;
// 2 - Add a document
let body = json!([{
"id": 1,
"title": "I'm a legend",
}]);
server.add_or_replace_multiple_documents(body).await;
// 3 - Get the complete document
let expected = json!({
"id": 1,
});
let (response, status_code) = server.get_document(1).await;
assert_eq!(status_code, 200);
assert_json_eq!(response, expected);
// 4 - Add a document with more fields
let body = json!([{
"id": 2,
"title": "I'm not a legend",
"description": "A bad copy of the original movie I'm a lengend"
}]);
server.add_or_replace_multiple_documents(body).await;
// 5 - Get the complete document
let expected = json!({
"id": 2,
});
let (response, status_code) = server.get_document(2).await;
assert_eq!(status_code, 200);
assert_json_eq!(response, expected);
}
#[actix_rt::test]
async fn index_new_fields_true_then_false() {
let mut server = common::Server::with_uid("movies");
let body = json!({
"uid": "movies",
"primaryKey": "id",
});
server.create_index(body).await;
// 1 - Set indexNewFields = true
server.update_accept_new_fields(json!(true)).await;
// 2 - Add a document
let body = json!([{
"id": 1,
"title": "I'm a legend",
}]);
server.add_or_replace_multiple_documents(body).await;
// 3 - Get the complete document
let expected = json!({
"id": 1,
"title": "I'm a legend",
});
let (response, status_code) = server.get_document(1).await;
assert_eq!(status_code, 200);
assert_json_eq!(response, expected);
// 4 - Set indexNewFields = false
server.update_accept_new_fields(json!(false)).await;
// 5 - Add a document with more fields
let body = json!([{
"id": 2,
"title": "I'm not a legend",
"description": "A bad copy of the original movie I'm a lengend"
}]);
server.add_or_replace_multiple_documents(body).await;
// 6 - Get the complete document
let expected = json!({
"id": 2,
"title": "I'm not a legend",
});
let (response, status_code) = server.get_document(2).await;
assert_eq!(status_code, 200);
assert_json_eq!(response, expected);
}
#[actix_rt::test]
async fn index_new_fields_false_then_true() {
let mut server = common::Server::with_uid("movies");
let body = json!({
"uid": "movies",
"primaryKey": "id",
});
server.create_index(body).await;
// 1 - Set indexNewFields = false
server.update_accept_new_fields(json!(false)).await;
// 2 - Add a document
let body = json!([{
"id": 1,
"title": "I'm a legend",
}]);
server.add_or_replace_multiple_documents(body).await;
// 3 - Get the complete document
let expected = json!({
"id": 1,
});
let (response, status_code) = server.get_document(1).await;
assert_eq!(status_code, 200);
assert_json_eq!(response, expected);
// 4 - Set indexNewFields = false
server.update_accept_new_fields(json!(true)).await;
// 5 - Add a document with more fields
let body = json!([{
"id": 2,
"title": "I'm not a legend",
"description": "A bad copy of the original movie I'm a lengend"
}]);
server.add_or_replace_multiple_documents(body).await;
// 6 - Get the complete document
let expected = json!({
"id": 1,
});
let (response, status_code) = server.get_document(1).await;
assert_eq!(status_code, 200);
assert_json_eq!(response, expected);
let expected = json!({
"id": 2,
"description": "A bad copy of the original movie I'm a lengend"
});
let (response, status_code) = server.get_document(2).await;
assert_eq!(status_code, 200);
assert_json_eq!(response, expected);
}
// Fix issue https://github.com/meilisearch/MeiliSearch/issues/518
#[actix_rt::test]
async fn accept_new_fields_does_not_take_into_account_the_primary_key() {
let mut server = common::Server::with_uid("movies");
// 1 - Create an index with no primary-key
let body = json!({
"uid": "movies",
});
let (response, status_code) = server.create_index(body).await;
assert_eq!(status_code, 201);
assert_eq!(response["primaryKey"], json!(null));
// 2 - Add searchable and displayed attributes as: ["title"] & Set acceptNewFields to false
let body = json!({
"searchableAttributes": ["title"],
"displayedAttributes": ["title"],
"acceptNewFields": false,
});
server.update_all_settings(body).await;
// 4 - Add a document
let body = json!([{
"id": 1,
"title": "Test",
"comment": "comment test"
}]);
server.add_or_replace_multiple_documents(body).await;
// 5 - Get settings, they should not changed
let (response, _status_code) = server.get_all_settings().await;
let expected = json!({
"rankingRules": [
"typo",
"words",
"proximity",
"attribute",
"wordsPosition",
"exactness",
],
"distinctAttribute": null,
"searchableAttributes": ["title"],
"displayedAttributes": ["title"],
"stopWords": [],
"synonyms": {},
"attributesForFaceting": [],
"acceptNewFields": false,
});
assert_json_eq!(response, expected, ordered: false);
}

View file

@ -171,6 +171,7 @@ async fn write_custom_ranking_and_index_documents() {
let expected = json!({
"id": 1,
"name": "Cherry Orr",
"color": "green"
});