Skip to content

Commit 6b51f6b

Browse files
rdettairdettai
committed
Fix index update endpoint panic when index_uri is the bucket root
1 parent 9a96456 commit 6b51f6b

File tree

3 files changed

+58
-21
lines changed

3 files changed

+58
-21
lines changed

‎quickwit/quickwit-common/src/uri.rs

+29-10
Original file line numberDiff line numberDiff line change
@@ -169,23 +169,30 @@ impl Uri {
169169
}
170170

171171
/// Returns the parent URI.
172+
///
173+
/// When the URI points to an object store, this method makes sure that the
174+
/// result targets to a well defined bucket, e.g `s3://` returns `None`.
175+
///
172176
/// Does not apply to PostgreSQL URIs.
173177
pub fn parent(&self) -> Option<Uri> {
178+
self.parent_unchecked().filter(|parent| {
179+
let path = parent.path();
180+
match self.protocol() {
181+
Protocol::S3 => path.components().count() >= 1,
182+
Protocol::Azure => path.components().count() >= 2,
183+
Protocol::Google => path.components().count() >= 1,
184+
_ => true,
185+
}
186+
})
187+
}
188+
189+
/// Same as [Self::parent()] but without the additional check on object stores.
190+
pub fn parent_unchecked(&self) -> Option<Uri> {
174191
if self.protocol().is_database() {
175192
return None;
176193
}
177194
let path = self.path();
178195
let protocol = self.protocol();
179-
180-
if protocol == Protocol::S3 && path.components().count() < 2 {
181-
return None;
182-
}
183-
if protocol == Protocol::Azure && path.components().count() < 3 {
184-
return None;
185-
}
186-
if protocol == Protocol::Google && path.components().count() < 2 {
187-
return None;
188-
}
189196
let parent_path = path.parent()?;
190197

191198
Some(Self {
@@ -597,11 +604,23 @@ mod tests {
597604
"ram:///foo"
598605
);
599606
assert!(Uri::for_test("s3://bucket").parent().is_none());
607+
assert_eq!(
608+
Uri::for_test("s3://bucket").parent_unchecked().unwrap(),
609+
"s3://"
610+
);
600611
assert!(Uri::for_test("s3://bucket/").parent().is_none());
612+
assert_eq!(
613+
Uri::for_test("s3://bucket/").parent_unchecked().unwrap(),
614+
"s3://"
615+
);
601616
assert_eq!(
602617
Uri::for_test("s3://bucket/foo").parent().unwrap(),
603618
"s3://bucket"
604619
);
620+
assert_eq!(
621+
Uri::for_test("s3://bucket/foo").parent_unchecked().unwrap(),
622+
"s3://bucket"
623+
);
605624
assert_eq!(
606625
Uri::for_test("s3://bucket/foo/").parent().unwrap(),
607626
"s3://bucket"

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

+18-8
Original file line numberDiff line numberDiff line change
@@ -73,12 +73,9 @@ pub fn load_index_config_from_user_config(
7373
pub fn load_index_config_update(
7474
config_format: ConfigFormat,
7575
index_config_bytes: &[u8],
76+
current_index_parent_dir: &Uri,
7677
current_index_config: &IndexConfig,
7778
) -> 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");
8279
let mut new_index_config = load_index_config_from_user_config(
8380
config_format,
8481
index_config_bytes,
@@ -355,10 +352,11 @@ mod test {
355352
index_id: hdfs-logs
356353
doc_mapping: {}
357354
"#;
355+
let default_root = Uri::for_test("s3://mybucket");
358356
let original_config: IndexConfig = load_index_config_from_user_config(
359357
ConfigFormat::Yaml,
360358
original_config_yaml.as_bytes(),
361-
&Uri::for_test("s3://mybucket"),
359+
&default_root,
362360
)
363361
.unwrap();
364362
{
@@ -371,6 +369,7 @@ mod test {
371369
let updated_config = load_index_config_update(
372370
ConfigFormat::Yaml,
373371
updated_config_yaml.as_bytes(),
372+
&default_root,
374373
&original_config,
375374
)
376375
.unwrap();
@@ -387,6 +386,7 @@ mod test {
387386
let updated_config = load_index_config_update(
388387
ConfigFormat::Yaml,
389388
updated_config_yaml.as_bytes(),
389+
&default_root,
390390
&original_config,
391391
)
392392
.unwrap();
@@ -403,6 +403,7 @@ mod test {
403403
let load_error = load_index_config_update(
404404
ConfigFormat::Yaml,
405405
updated_config_yaml.as_bytes(),
406+
&default_root,
406407
&original_config,
407408
)
408409
.unwrap_err();
@@ -432,10 +433,11 @@ mod test {
432433
period: 90 days
433434
schedule: daily
434435
"#;
436+
let default_root = Uri::for_test("s3://mybucket");
435437
let original_config: IndexConfig = load_index_config_from_user_config(
436438
ConfigFormat::Yaml,
437439
original_config_yaml.as_bytes(),
438-
&Uri::for_test("s3://mybucket"),
440+
&default_root,
439441
)
440442
.unwrap();
441443

@@ -452,6 +454,7 @@ mod test {
452454
let updated_config = load_index_config_update(
453455
ConfigFormat::Yaml,
454456
updated_config_yaml.as_bytes(),
457+
&default_root,
455458
&original_config,
456459
)
457460
.unwrap();
@@ -473,10 +476,11 @@ mod test {
473476
index_id: hdfs-logs
474477
doc_mapping: {}
475478
"#;
479+
let default_root = Uri::for_test("s3://mybucket");
476480
let original_config: IndexConfig = load_index_config_from_user_config(
477481
ConfigFormat::Yaml,
478482
original_config_yaml.as_bytes(),
479-
&Uri::for_test("s3://mybucket"),
483+
&default_root,
480484
)
481485
.unwrap();
482486

@@ -493,6 +497,7 @@ mod test {
493497
let updated_config = load_index_config_update(
494498
ConfigFormat::Yaml,
495499
updated_config_yaml.as_bytes(),
500+
&default_root,
496501
&original_config,
497502
)
498503
.unwrap();
@@ -513,10 +518,11 @@ mod test {
513518
type: datetime
514519
fast: true
515520
"#;
521+
let default_root = Uri::for_test("s3://mybucket");
516522
let original_config: IndexConfig = load_index_config_from_user_config(
517523
ConfigFormat::Yaml,
518524
original_config_yaml.as_bytes(),
519-
&Uri::for_test("s3://mybucket"),
525+
&default_root,
520526
)
521527
.unwrap();
522528

@@ -539,6 +545,7 @@ mod test {
539545
load_index_config_update(
540546
ConfigFormat::Yaml,
541547
updated_config_yaml.as_bytes(),
548+
&default_root,
542549
&original_config,
543550
)
544551
.expect_err("mapping changed but uid fixed should error");
@@ -556,6 +563,7 @@ mod test {
556563
load_index_config_update(
557564
ConfigFormat::Yaml,
558565
updated_config_yaml.as_bytes(),
566+
&default_root,
559567
&original_config,
560568
)
561569
.expect_err("timestamp field removed should error");
@@ -575,6 +583,7 @@ mod test {
575583
load_index_config_update(
576584
ConfigFormat::Yaml,
577585
updated_config_yaml.as_bytes(),
586+
&default_root,
578587
&original_config,
579588
)
580589
.expect_err("field required for timestamp is absent");
@@ -595,6 +604,7 @@ mod test {
595604
load_index_config_update(
596605
ConfigFormat::Yaml,
597606
updated_config_yaml.as_bytes(),
607+
&default_root,
598608
&original_config,
599609
)
600610
.expect_err("field required for default search is absent");

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

+11-3
Original file line numberDiff line numberDiff line change
@@ -336,9 +336,17 @@ pub async fn update_index(
336336
let index_uid = current_index_metadata.index_uid.clone();
337337
let current_index_config = current_index_metadata.into_index_config();
338338

339-
let new_index_config =
340-
load_index_config_update(config_format, &index_config_bytes, &current_index_config)
341-
.map_err(IndexServiceError::InvalidConfig)?;
339+
let current_index_parent_dir = &current_index_config
340+
.index_uri
341+
.parent_unchecked()
342+
.ok_or_else(|| IndexServiceError::Internal("index URI should have a parent".to_string()))?;
343+
let new_index_config = load_index_config_update(
344+
config_format,
345+
&index_config_bytes,
346+
&current_index_parent_dir,
347+
&current_index_config,
348+
)
349+
.map_err(IndexServiceError::InvalidConfig)?;
342350

343351
let update_request = UpdateIndexRequest::try_from_updates(
344352
index_uid,

0 commit comments

Comments
 (0)