Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Better error response for wrong datetime format in REST filter #4111

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

fix: Better error response for wrong datetime format in REST filter #4111

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c5f1fd8
fix: Better error response for wrong datetime format in REST filter
gsaivinay Apr 24, 2024
ab1810e
fix: Better error response for wrong datetime format in REST filter
gsaivinay Apr 24, 2024
d72ace0
Merge branch 'qdrant:dev' into dev
gsaivinay May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
114 changes: 112 additions & 2 deletions lib/segment/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1426,13 +1426,78 @@ impl From<Vec<IntPayloadType>> for MatchExcept {
}
}

#[derive(Debug, Deserialize, Serialize, JsonSchema, Clone, PartialEq)]
#[derive(Debug, Serialize, JsonSchema, Clone, PartialEq)]
#[serde(untagged)]
pub enum RangeInterface {
Float(Range<FloatPayloadType>),
DateTime(Range<DateTimePayloadType>),
}

impl<'de> Deserialize<'de> for RangeInterface {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
#[derive(Deserialize)]
#[serde(untagged)]
enum RangeVariants {
Float(Range<FloatPayloadType>),
DateTime(Range<String>),
}

match RangeVariants::deserialize(deserializer)? {
RangeVariants::Float(range) => Ok(RangeInterface::Float(range)),
RangeVariants::DateTime(range) => {
let lt = range
.lt
.map(|s| {
DateTimePayloadType::from_str(&s).map_err(|_| {
serde::de::Error::custom(format!(
"'{s}' is not in a supported date/time format, please use RFC 3339"
))
})
})
.transpose()?;

let gt = range
.gt
.map(|s| {
DateTimePayloadType::from_str(&s).map_err(|_| {
serde::de::Error::custom(format!(
"'{s}' is not in a supported date/time format, please use RFC 3339"
))
})
})
.transpose()?;

let gte = range
.gte
.map(|s| {
DateTimePayloadType::from_str(&s).map_err(|_| {
serde::de::Error::custom(format!(
"'{s}' is not in a supported date/time format, please use RFC 3339"
))
})
})
.transpose()?;

let lte = range
.lte
.map(|s| {
DateTimePayloadType::from_str(&s).map_err(|_| {
serde::de::Error::custom(format!(
"'{s}' is not in a supported date/time format, please use RFC 3339"
))
})
})
.transpose()?;

Ok(RangeInterface::DateTime(Range { lt, gt, gte, lte }))
}
}
}
}

/// Range filter request
#[macro_rules_attribute::macro_rules_derive(crate::common::macros::schemars_rename_generics)]
#[derive_args(< FloatPayloadType > => "Range", < DateTimePayloadType > => "DatetimeRange")]
Expand Down Expand Up @@ -1911,7 +1976,7 @@ impl NestedCondition {
}
}

#[derive(Debug, Deserialize, Serialize, JsonSchema, Clone, PartialEq)]
#[derive(Debug, Serialize, JsonSchema, Clone, PartialEq)]
#[serde(untagged)]
#[allow(clippy::large_enum_variant)]
pub enum Condition {
Expand All @@ -1929,6 +1994,51 @@ pub enum Condition {
Filter(Filter),
}

impl<'de> serde::Deserialize<'de> for Condition {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
let value = serde_json::Value::deserialize(deserializer)?;
match value {
serde_json::Value::Object(map) => {
if map.contains_key("key") {
FieldCondition::deserialize(serde_json::Value::Object(map))
.map(Condition::Field)
.map_err(serde::de::Error::custom)
} else if map.contains_key("is_empty") {
IsEmptyCondition::deserialize(serde_json::Value::Object(map))
.map(Condition::IsEmpty)
.map_err(serde::de::Error::custom)
} else if map.contains_key("is_null") {
Comment on lines +2005 to +2013
Copy link
Member

Choose a reason for hiding this comment

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

with if-else statements it will be very easy to forget to implement some parts of the deserializer if we introduce new field here, as compiler won't show any errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@generall - fair enough, I'll try to change it, do you suggest any approach?

Copy link
Member

Choose a reason for hiding this comment

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

A match statement with strict enum variants for example. Though to be honest, I don't clearly see how such approach can be used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't clearly see how such approach can be used here

yes that is true
.

strict enum variants

But isn't that requires introducing a new enum, and we might face the did not match any variant again ?

IsNullCondition::deserialize(serde_json::Value::Object(map))
.map(Condition::IsNull)
.map_err(serde::de::Error::custom)
} else if map.contains_key("has_id") {
HasIdCondition::deserialize(serde_json::Value::Object(map))
.map(Condition::HasId)
.map_err(serde::de::Error::custom)
} else if map.contains_key("nested") {
NestedCondition::deserialize(serde_json::Value::Object(map))
.map(Condition::Nested)
.map_err(serde::de::Error::custom)
} else if map.contains_key("should")
|| map.contains_key("must")
|| map.contains_key("must_not")
|| map.contains_key("min_should")
{
Filter::deserialize(serde_json::Value::Object(map))
.map(Condition::Filter)
.map_err(serde::de::Error::custom)
} else {
Err(serde::de::Error::custom("Invalid Condition format"))
}
}
_ => Err(serde::de::Error::custom("Invalid Condition format")),
}
}
}

impl Condition {
pub fn new_nested(key: JsonPath, filter: Filter) -> Self {
Self::Nested(NestedCondition {
Expand Down