Skip to content

Commit

Permalink
Bugfix sort order query string in elastic search api. (#5490)
Browse files Browse the repository at this point in the history
The sort order was parsed using the generated from_str_name method.
The spec of this method probably changed at one point, and we ended up
requiring sort order to be uppercase.

This PR changes the behavior to follow what we do when we parse a json
string, and adds unit tests.

Closes #5491
  • Loading branch information
fulmicoton authored Oct 16, 2024
1 parent 4c1e228 commit e08eb9f
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ use std::fmt;
use std::str::FromStr;
use std::time::Duration;

use quickwit_proto::search::SortOrder;
use quickwit_query::BooleanOperand;
use quickwit_search::SearchError;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -232,10 +231,16 @@ pub struct DeleteQueryParams {
pub timeout: Option<String>,
}

/// Parses a string as if it was a json value string.
fn parse_str_like_json<T: serde::de::DeserializeOwned>(s: &str) -> Option<T> {
let json_value = serde_json::Value::String(s.to_string());
serde_json::from_value::<T>(json_value).ok()
}

// Parse a single sort field parameter from ES sort query string parameter.
fn parse_sort_field_str(sort_field_str: &str) -> Result<SortField, SearchError> {
if let Some((field, order_str)) = sort_field_str.split_once(':') {
let order = SortOrder::from_str_name(order_str).ok_or_else(|| {
let order = parse_str_like_json(order_str).ok_or_else(|| {
SearchError::InvalidArgument(format!(
"invalid sort order `{}`. expected `asc` or `desc`",
field
Expand Down Expand Up @@ -375,3 +380,49 @@ impl fmt::Display for SuggestMode {
}
}
}

#[cfg(test)]
mod tests {

use quickwit_proto::search::SortOrder;

use super::*;

#[derive(Deserialize, PartialEq, Eq, Debug)]
#[serde(rename_all = "snake_case")]
enum TestEnum {
FirstItem,
SecondItem,
}

#[test]
fn test_parse_str_like_json() {
assert_eq!(
parse_str_like_json::<TestEnum>("first_item").unwrap(),
TestEnum::FirstItem
);
assert!(parse_str_like_json::<TestEnum>("FirstItem").is_none());
}

#[test]
fn test_sort_order_qs() {
let sort_order_qs = parse_sort_field_str("timestamp:desc").unwrap();
assert_eq!(
sort_order_qs,
SortField {
field: "timestamp".to_string(),
order: SortOrder::Desc,
date_format: None
}
);
let sort_order_qs = parse_sort_field_str("timestamp:asc").unwrap();
assert_eq!(
sort_order_qs,
SortField {
field: "timestamp".to_string(),
order: SortOrder::Asc,
date_format: None
}
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,21 @@ expected:
actor:
id: 10791502
---
# Checking that passing the sort params as a query string works.
params:
sort: "actor.id:desc"
q: "*"
size: 1
expected:
hits:
total:
value: 100
relation: eq
hits:
- _source:
actor:
id: 10791502
---
json:
size: 1
query:
Expand Down Expand Up @@ -113,4 +128,3 @@ expected:
- _source:
actor:
id: 1762355

0 comments on commit e08eb9f

Please sign in to comment.