Skip to content

Commit 7dabbe5

Browse files
rdettairdettai
authored
Fix index update endpoint panic when index_uri is the bucket root (#5711)
* Add failing unit test * Fix index update endpoint panic when index_uri is the bucket root * Simplify update endpoint to use same default URI as create
1 parent 1366486 commit 7dabbe5

4 files changed

Lines changed: 59 additions & 17 deletions

File tree

‎docs/reference/rest-api.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ Updating the doc mapping doesn't reindex existing data. Queries and results are
348348
|---------------------|--------------------|-----------------------------------------------------------------------------------------------------------------------|---------------------------------------|
349349
| `version` | `String` | Config format version, use the same as your Quickwit version. | _required_ |
350350
| `index_id` | `String` | Index ID, must be the same index as in the request URI. | _required_ |
351-
| `index_uri` | `String` | Defines where the index files are stored. Cannot be updated. | `{current_index_uri}` |
351+
| `index_uri` | `String` | Defines where the index files are stored. Cannot be updated. | `{default_index_root_uri}/{index_id}` |
352352
| `doc_mapping` | `DocMapping` | Doc mapping object as specified in the [index config docs](../configuration/index-config.md#doc-mapping). | _required_ |
353353
| `indexing_settings` | `IndexingSettings` | Indexing settings object as specified in the [index config docs](../configuration/index-config.md#indexing-settings). | |
354354
| `search_settings` | `SearchSettings` | Search settings object as specified in the [index config docs](../configuration/index-config.md#search-settings). | |

‎quickwit/quickwit-config/src/index_config/serialize.rs‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -68,21 +68,17 @@ pub fn load_index_config_from_user_config(
6868
///
6969
/// Ensures that the new configuration is valid in itself and compared to the
7070
/// current index config. If the new configuration omits some fields, the
71-
/// default values will be used, not those of the current index config. The only
72-
/// exception is the index_uri because it cannot be updated.
71+
/// default values will be used, not those of the current index config.
7372
pub fn load_index_config_update(
7473
config_format: ConfigFormat,
7574
index_config_bytes: &[u8],
75+
default_index_root_uri: &Uri,
7676
current_index_config: &IndexConfig,
7777
) -> anyhow::Result<IndexConfig> {
78-
let current_index_parent_dir = &current_index_config
79-
.index_uri
80-
.parent()
81-
.expect("index URI should have a parent");
8278
let mut new_index_config = load_index_config_from_user_config(
8379
config_format,
8480
index_config_bytes,
85-
current_index_parent_dir,
81+
default_index_root_uri,
8682
)?;
8783
ensure!(
8884
current_index_config.index_id == new_index_config.index_id,
@@ -355,10 +351,11 @@ mod test {
355351
index_id: hdfs-logs
356352
doc_mapping: {}
357353
"#;
354+
let default_root = Uri::for_test("s3://mybucket");
358355
let original_config: IndexConfig = load_index_config_from_user_config(
359356
ConfigFormat::Yaml,
360357
original_config_yaml.as_bytes(),
361-
&Uri::for_test("s3://mybucket"),
358+
&default_root,
362359
)
363360
.unwrap();
364361
{
@@ -371,6 +368,7 @@ mod test {
371368
let updated_config = load_index_config_update(
372369
ConfigFormat::Yaml,
373370
updated_config_yaml.as_bytes(),
371+
&default_root,
374372
&original_config,
375373
)
376374
.unwrap();
@@ -387,6 +385,7 @@ mod test {
387385
let updated_config = load_index_config_update(
388386
ConfigFormat::Yaml,
389387
updated_config_yaml.as_bytes(),
388+
&default_root,
390389
&original_config,
391390
)
392391
.unwrap();
@@ -403,6 +402,7 @@ mod test {
403402
let load_error = load_index_config_update(
404403
ConfigFormat::Yaml,
405404
updated_config_yaml.as_bytes(),
405+
&default_root,
406406
&original_config,
407407
)
408408
.unwrap_err();
@@ -432,10 +432,11 @@ mod test {
432432
period: 90 days
433433
schedule: daily
434434
"#;
435+
let default_root = Uri::for_test("s3://mybucket");
435436
let original_config: IndexConfig = load_index_config_from_user_config(
436437
ConfigFormat::Yaml,
437438
original_config_yaml.as_bytes(),
438-
&Uri::for_test("s3://mybucket"),
439+
&default_root,
439440
)
440441
.unwrap();
441442

@@ -452,6 +453,7 @@ mod test {
452453
let updated_config = load_index_config_update(
453454
ConfigFormat::Yaml,
454455
updated_config_yaml.as_bytes(),
456+
&default_root,
455457
&original_config,
456458
)
457459
.unwrap();
@@ -473,10 +475,11 @@ mod test {
473475
index_id: hdfs-logs
474476
doc_mapping: {}
475477
"#;
478+
let default_root = Uri::for_test("s3://mybucket");
476479
let original_config: IndexConfig = load_index_config_from_user_config(
477480
ConfigFormat::Yaml,
478481
original_config_yaml.as_bytes(),
479-
&Uri::for_test("s3://mybucket"),
482+
&default_root,
480483
)
481484
.unwrap();
482485

@@ -493,6 +496,7 @@ mod test {
493496
let updated_config = load_index_config_update(
494497
ConfigFormat::Yaml,
495498
updated_config_yaml.as_bytes(),
499+
&default_root,
496500
&original_config,
497501
)
498502
.unwrap();
@@ -513,10 +517,11 @@ mod test {
513517
type: datetime
514518
fast: true
515519
"#;
520+
let default_root = Uri::for_test("s3://mybucket");
516521
let original_config: IndexConfig = load_index_config_from_user_config(
517522
ConfigFormat::Yaml,
518523
original_config_yaml.as_bytes(),
519-
&Uri::for_test("s3://mybucket"),
524+
&default_root,
520525
)
521526
.unwrap();
522527

@@ -539,6 +544,7 @@ mod test {
539544
load_index_config_update(
540545
ConfigFormat::Yaml,
541546
updated_config_yaml.as_bytes(),
547+
&default_root,
542548
&original_config,
543549
)
544550
.expect_err("mapping changed but uid fixed should error");
@@ -556,6 +562,7 @@ mod test {
556562
load_index_config_update(
557563
ConfigFormat::Yaml,
558564
updated_config_yaml.as_bytes(),
565+
&default_root,
559566
&original_config,
560567
)
561568
.expect_err("timestamp field removed should error");
@@ -575,6 +582,7 @@ mod test {
575582
load_index_config_update(
576583
ConfigFormat::Yaml,
577584
updated_config_yaml.as_bytes(),
585+
&default_root,
578586
&original_config,
579587
)
580588
.expect_err("field required for timestamp is absent");
@@ -595,6 +603,7 @@ mod test {
595603
load_index_config_update(
596604
ConfigFormat::Yaml,
597605
updated_config_yaml.as_bytes(),
606+
&default_root,
598607
&original_config,
599608
)
600609
.expect_err("field required for default search is absent");

‎quickwit/quickwit-serve/src/index_api/index_resource.rs‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -286,13 +286,15 @@ pub async fn create_index(
286286

287287
pub fn update_index_handler(
288288
metastore: MetastoreServiceClient,
289+
node_config: Arc<NodeConfig>,
289290
) -> impl Filter<Extract = (impl warp::Reply,), Error = Rejection> + Clone {
290291
warp::path!("indexes" / String)
291292
.and(warp::put())
292293
.and(extract_config_format())
293294
.and(warp::body::content_length_limit(1024 * 1024))
294295
.and(warp::filters::body::bytes())
295296
.and(with_arg(metastore))
297+
.and(with_arg(node_config))
296298
.then(update_index)
297299
.map(log_failure("failed to update index"))
298300
.and(extract_format_from_qs())
@@ -325,6 +327,7 @@ pub async fn update_index(
325327
config_format: ConfigFormat,
326328
index_config_bytes: Bytes,
327329
metastore: MetastoreServiceClient,
330+
node_config: Arc<NodeConfig>,
328331
) -> Result<IndexMetadata, IndexServiceError> {
329332
info!(index_id = %target_index_id, "update-index");
330333

@@ -336,9 +339,13 @@ pub async fn update_index(
336339
let index_uid = current_index_metadata.index_uid.clone();
337340
let current_index_config = current_index_metadata.into_index_config();
338341

339-
let new_index_config =
340-
load_index_config_update(config_format, &index_config_bytes, &current_index_config)
341-
.map_err(IndexServiceError::InvalidConfig)?;
342+
let new_index_config = load_index_config_update(
343+
config_format,
344+
&index_config_bytes,
345+
&node_config.default_index_root_uri,
346+
&current_index_config,
347+
)
348+
.map_err(IndexServiceError::InvalidConfig)?;
342349

343350
let update_request = UpdateIndexRequest::try_from_updates(
344351
index_uid,

‎quickwit/quickwit-serve/src/index_api/rest_handler.rs‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,11 @@ pub fn index_management_handlers(
8989
// Indexes handlers.
9090
get_index_metadata_handler(index_service.metastore())
9191
.or(list_indexes_metadata_handler(index_service.metastore()))
92-
.or(create_index_handler(index_service.clone(), node_config))
93-
.or(update_index_handler(index_service.metastore()))
92+
.or(create_index_handler(
93+
index_service.clone(),
94+
node_config.clone(),
95+
))
96+
.or(update_index_handler(index_service.metastore(), node_config))
9497
.or(clear_index_handler(index_service.clone()))
9598
.or(delete_index_handler(index_service.clone()))
9699
.boxed()
@@ -1034,6 +1037,29 @@ mod tests {
10341037
.default_search_fields,
10351038
["severity_text", "body"]
10361039
);
1040+
// test with index_uri at the root of a bucket
1041+
{
1042+
let resp = warp::test::request()
1043+
.path("/indexes")
1044+
.method("POST")
1045+
.json(&true)
1046+
.body(r#"{"version": "0.7", "index_id": "hdfs-logs2", "index_uri": "s3://my-bucket", "doc_mapping": {"field_mappings":[{"name": "timestamp", "type": "i64", "fast": true, "indexed": true}]},"search_settings":{"default_search_fields":["body"]}}"#)
1047+
.reply(&index_management_handler)
1048+
.await;
1049+
let body = std::str::from_utf8(resp.body()).unwrap();
1050+
assert_eq!(resp.status(), 200, "{body}",);
1051+
}
1052+
{
1053+
let resp = warp::test::request()
1054+
.path("/indexes/hdfs-logs2")
1055+
.method("PUT")
1056+
.json(&true)
1057+
.body(r#"{"version": "0.7", "index_id": "hdfs-logs2", "index_uri": "s3://my-bucket", "doc_mapping": {"field_mappings":[{"name": "timestamp", "type": "i64", "fast": true, "indexed": true}]},"search_settings":{"default_search_fields":["severity_text", "body"]}}"#)
1058+
.reply(&index_management_handler)
1059+
.await;
1060+
let body = std::str::from_utf8(resp.body()).unwrap();
1061+
assert_eq!(resp.status(), 200, "{body}",);
1062+
}
10371063
}
10381064

10391065
#[tokio::test]

0 commit comments

Comments
 (0)