Skip to content

Commit

Permalink
fix default slice start in parser (#559)
Browse files Browse the repository at this point in the history 
According to the spec the start of a slice when not explicitly specified
depends on the sign of the step. The current implementation always used
FromStart(0), which is correct for forward steps, but for backward steps
it's supposed to be starting from the end - FromEnd(1).

This affects only `rsonpath-syntax`.
  • Loading branch information
@V0ldek
V0ldek authored Oct 26, 2024
1 parent ac85aee commit 094ae20
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 9 deletions.
44 changes: 42 additions & 2 deletions crates/rsonpath-syntax/src/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,8 @@ impl From<JsonPathQueryBuilder> for JsonPathQuery {
/// ```
pub struct SliceBuilder {
inner: Slice,
/// We need to track if start is explicit because the default depends on step sign.
start_was_explicitly_given: bool,
}

impl SliceBuilder {
Expand All @@ -432,13 +434,15 @@ impl SliceBuilder {
pub fn new() -> Self {
Self {
inner: Slice::default(),
start_was_explicitly_given: false,
}
}

/// Set the start of the [`Slice`].
#[inline]
pub fn with_start<N: Into<JsonInt>>(&mut self, start: N) -> &mut Self {
self.inner.start = start.into().into();
self.start_was_explicitly_given = true;
self
}

Expand All @@ -462,15 +466,23 @@ impl SliceBuilder {
#[inline]
#[must_use]
pub fn to_slice(&mut self) -> Slice {
if !self.start_was_explicitly_given {
if self.inner.step.is_forward() {
self.inner.start = Slice::DEFAULT_START_FORWARDS;
} else {
self.inner.start = Slice::default_start_backwards();
}
}

self.inner.clone()
}
}

impl From<SliceBuilder> for Slice {
#[inline]
#[must_use]
fn from(value: SliceBuilder) -> Self {
value.inner
fn from(mut value: SliceBuilder) -> Self {
value.to_slice()
}
}

Expand Down Expand Up @@ -805,3 +817,31 @@ impl From<LogicalExprBuilder> for LogicalExpr {
value.current
}
}

#[cfg(test)]
mod tests {
use super::SliceBuilder;
use crate::{Index, Slice, Step};

#[test]
fn slice_builder_default_start_forward() {
let mut builder = SliceBuilder::new();
builder.with_end(3).with_step(4);
let slice: Slice = builder.into();

assert_eq!(slice.start(), Index::FromStart(0.into()));
assert_eq!(slice.end(), Some(Index::FromStart(3.into())));
assert_eq!(slice.step(), Step::Forward(4.into()));
}

#[test]
fn slice_builder_default_start_backward() {
let mut builder = SliceBuilder::new();
builder.with_end(3).with_step(-4);
let slice: Slice = builder.into();

assert_eq!(slice.start(), Index::FromEnd(1.try_into().unwrap()));
assert_eq!(slice.end(), Some(Index::FromStart(3.into())));
assert_eq!(slice.step(), Step::Backward(4.try_into().unwrap()));
}
}
13 changes: 10 additions & 3 deletions crates/rsonpath-syntax/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,8 +542,13 @@ pub struct Slice {
}

impl Slice {
const DEFAULT_START: Index = Index::FromStart(num::JsonUInt::ZERO);
const DEFAULT_STEP: Step = Step::Forward(num::JsonUInt::ONE);
pub(crate) const DEFAULT_START_FORWARDS: Index = Index::FromStart(num::JsonUInt::ZERO);
/// This is not const because the required NonZeroU64::MIN is from Rust 1.70.
#[inline(always)]
pub(crate) fn default_start_backwards() -> Index {
Index::FromEnd(1.try_into().expect("const 1 is nonzero"))
}
pub(crate) const DEFAULT_STEP: Step = Step::Forward(num::JsonUInt::ONE);

/// Create a new [`Slice`] from given bounds and step.
#[inline(always)]
Expand Down Expand Up @@ -1180,7 +1185,9 @@ impl Display for Step {
impl Display for Slice {
#[inline]
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
if self.start != Self::DEFAULT_START {
if (self.step.is_forward() && self.start != Self::DEFAULT_START_FORWARDS)
|| (self.step.is_backward() && self.start != Self::default_start_backwards())
{
write!(f, "{}", self.start)?;
}
write!(f, ":")?;
Expand Down
16 changes: 12 additions & 4 deletions crates/rsonpath-syntax/src/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,11 @@ fn slice_selector(q: &str) -> IResult<&str, Selector, InternalParseError> {
};
}

// Fixup the bounds - if start was not given and step is negative, the default must be reversed.
if slice.step.is_backward() && opt_start.is_none() {
slice.start = crate::Slice::default_start_backwards();
}

Ok((rest, Selector::Slice(slice)))
}

Expand Down Expand Up @@ -1020,8 +1025,8 @@ mod tests {
#[test_case("-3:-4:-5", Index::FromEnd(3.try_into().unwrap()), Some(Index::FromEnd(4.try_into().unwrap())), Step::Backward(5.try_into().unwrap()); "test m3cm4cm5")]
#[test_case(":4:5", Index::FromStart(0.into()), Some(Index::FromStart(4.into())), Step::Forward(5.into()); "test c4c5")]
#[test_case(":-4:5", Index::FromStart(0.into()), Some(Index::FromEnd(4.try_into().unwrap())), Step::Forward(5.into()); "test cm4c5")]
#[test_case(":4:-5", Index::FromStart(0.into()), Some(Index::FromStart(4.into())), Step::Backward(5.try_into().unwrap()); "test c4cm5")]
#[test_case(":-4:-5", Index::FromStart(0.into()), Some(Index::FromEnd(4.try_into().unwrap())), Step::Backward(5.try_into().unwrap()); "test cm4cm5")]
#[test_case(":4:-5", Index::FromEnd(1.try_into().unwrap()), Some(Index::FromStart(4.into())), Step::Backward(5.try_into().unwrap()); "test c4cm5")]
#[test_case(":-4:-5", Index::FromEnd(1.try_into().unwrap()), Some(Index::FromEnd(4.try_into().unwrap())), Step::Backward(5.try_into().unwrap()); "test cm4cm5")]
#[test_case("3::5", Index::FromStart(3.into()), None, Step::Forward(5.into()); "test 3cc5")]
#[test_case("-3::5", Index::FromEnd(3.try_into().unwrap()), None, Step::Forward(5.into()); "test m3cc5")]
#[test_case("3::-5", Index::FromStart(3.into()), None, Step::Backward(5.try_into().unwrap()); "test 3ccm5")]
Expand All @@ -1039,9 +1044,12 @@ mod tests {
#[test_case(":4", Index::FromStart(0.into()), Some(Index::FromStart(4.into())), Step::Forward(1.into()); "test c4")]
#[test_case(":-4", Index::FromStart(0.into()), Some(Index::FromEnd(4.try_into().unwrap())), Step::Forward(1.into()); "test cm4")]
#[test_case("::5", Index::FromStart(0.into()), None, Step::Forward(5.into()); "test cc5")]
#[test_case("::-5", Index::FromStart(0.into()), None, Step::Backward(5.try_into().unwrap()); "test ccm5")]
#[test_case("::-5", Index::FromEnd(1.try_into().unwrap()), None, Step::Backward(5.try_into().unwrap()); "test ccm5")]
#[test_case("::", Index::FromStart(0.into()), None, Step::Forward(1.into()); "test cc")]
fn full_positive_slice(input: &str, exp_start: Index, exp_end: Option<Index>, exp_step: Step) {
#[test_case("::-1", Index::FromEnd(1.try_into().unwrap()), None, Step::Backward(1.try_into().unwrap()); "test ccm1")]
#[test_case("0::-1", Index::FromStart(0.into()), None, Step::Backward(1.try_into().unwrap()); "test 0ccm1")]
#[test_case("0:0:-1", Index::FromStart(0.into()), Some(Index::FromStart(0.into())), Step::Backward(1.try_into().unwrap()); "test 0c0cm1")]
fn slice(input: &str, exp_start: Index, exp_end: Option<Index>, exp_step: Step) {
let (rest, selector) = super::slice_selector(input).expect("should parse");
assert_eq!("", rest);
match selector {
Expand Down

0 comments on commit 094ae20

Please sign in to comment.