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.

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

feat: implement integer type to comply with spec #98

Merged
merged 7 commits into from
Aug 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions serde_json_path/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- **fixed**: edge case where `.` in regexes for `match` and `search` functions was matching `\r\n` properly ([#92])
- **breaking**: added `regex` feature flag that gates regex functions `match` and `search`
- Feature is enabled by default, but if you have `default-features = false` you'll need to explicitly add it to retain access to these functions
- **breaking**(`serde_json_path_core`): ensure integers used as indices are within the [valid range for I-JSON][i-json-range] ([#98])

[#92]: https://github.com/hiltontj/serde_json_path/pull/92
[#98]: https://github.com/hiltontj/serde_json_path/pull/98
[i-json-range]: https://www.rfc-editor.org/rfc/rfc9535.html#section-2.1-4.1

# 0.6.7 (3 March 2024)

Expand Down
15 changes: 9 additions & 6 deletions serde_json_path/src/parser/primitive/int.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use nom::{
combinator::{map_res, opt, recognize},
sequence::tuple,
};
use serde_json_path_core::spec::integer::Integer;

use crate::parser::PResult;

Expand Down Expand Up @@ -37,19 +38,21 @@ pub(crate) fn parse_int_string(input: &str) -> PResult<&str> {
}

#[cfg_attr(feature = "trace", tracing::instrument(level = "trace", parent = None, ret, err))]
pub(crate) fn parse_int(input: &str) -> PResult<isize> {
map_res(parse_int_string, |i_str| i_str.parse::<isize>())(input)
pub(crate) fn parse_int(input: &str) -> PResult<Integer> {
map_res(parse_int_string, |i_str| i_str.parse())(input)
}

#[cfg(test)]
mod tests {
use serde_json_path_core::spec::integer::Integer;

use crate::parser::primitive::int::parse_int;

#[test]
fn parse_integers() {
assert_eq!(parse_int("0"), Ok(("", 0)));
assert_eq!(parse_int("10"), Ok(("", 10)));
assert_eq!(parse_int("-10"), Ok(("", -10)));
assert_eq!(parse_int("010"), Ok(("10", 0)));
assert_eq!(parse_int("0"), Ok(("", Integer::from_i64_unchecked(0))));
assert_eq!(parse_int("10"), Ok(("", Integer::from_i64_unchecked(10))));
assert_eq!(parse_int("-10"), Ok(("", Integer::from_i64_unchecked(-10))));
assert_eq!(parse_int("010"), Ok(("10", Integer::from_i64_unchecked(0))));
}
}
10 changes: 8 additions & 2 deletions serde_json_path/src/parser/segment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,10 @@ mod tests {
use test_log::test;

use nom::combinator::all_consuming;
use serde_json_path_core::spec::selector::{index::Index, name::Name, slice::Slice, Selector};
use serde_json_path_core::spec::{
integer::Integer,
selector::{index::Index, name::Name, slice::Slice, Selector},
};

use super::{
parse_child_long_hand, parse_child_segment, parse_descendant_segment,
Expand Down Expand Up @@ -194,7 +197,10 @@ mod tests {
let (_, sk) = parse_child_long_hand(r#"['name',10,0:3]"#).unwrap();
let s = sk.as_long_hand().unwrap();
assert_eq!(s[0], Selector::Name(Name::from("name")));
assert_eq!(s[1], Selector::Index(Index(10)));
assert_eq!(
s[1],
Selector::Index(Index(Integer::from_i64_unchecked(10)))
);
assert_eq!(
s[2],
Selector::ArraySlice(Slice::new().with_start(0).with_end(3))
Expand Down
9 changes: 6 additions & 3 deletions serde_json_path/src/parser/selector/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,10 @@ pub(crate) fn parse_selector(input: &str) -> PResult<Selector> {

#[cfg(test)]
mod tests {
use serde_json_path_core::spec::selector::{name::Name, slice::Slice};
use serde_json_path_core::spec::{
integer::Integer,
selector::{name::Name, slice::Slice},
};

use super::{parse_selector, parse_wildcard_selector, Index, Selector};

Expand All @@ -84,11 +87,11 @@ mod tests {
fn all_selectors() {
{
let (_, s) = parse_selector("0").unwrap();
assert_eq!(s, Selector::Index(Index(0)));
assert_eq!(s, Selector::Index(Index(Integer::from_i64_unchecked(0))));
}
{
let (_, s) = parse_selector("10").unwrap();
assert_eq!(s, Selector::Index(Index(10)));
assert_eq!(s, Selector::Index(Index(Integer::from_i64_unchecked(10))));
}
{
let (_, s) = parse_selector("'name'").unwrap();
Expand Down
6 changes: 3 additions & 3 deletions serde_json_path/src/parser/selector/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@ use nom::{
combinator::{map, opt},
sequence::{preceded, separated_pair, terminated},
};
use serde_json_path_core::spec::selector::slice::Slice;
use serde_json_path_core::spec::{integer::Integer, selector::slice::Slice};

use crate::parser::{primitive::int::parse_int, PResult};

#[cfg_attr(feature = "trace", tracing::instrument(level = "trace", parent = None, ret, err))]
fn parse_int_space_after(input: &str) -> PResult<isize> {
fn parse_int_space_after(input: &str) -> PResult<Integer> {
terminated(parse_int, multispace0)(input)
}

#[cfg_attr(feature = "trace", tracing::instrument(level = "trace", parent = None, ret, err))]
fn parse_int_space_before(input: &str) -> PResult<isize> {
fn parse_int_space_before(input: &str) -> PResult<Integer> {
preceded(multispace0, parse_int)(input)
}

Expand Down
5 changes: 5 additions & 0 deletions serde_json_path_core/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

# Unreleased

- **breaking**: ensure integers used as indices are within the [valid range for I-JSON][i-json-range] ([#98])

[#98]: https://github.com/hiltontj/serde_json_path/pull/98
[i-json-range]: https://www.rfc-editor.org/rfc/rfc9535.html#section-2.1-4.1

# 0.1.6 (3 March 2024)

- **testing**: support tests for non-determinism in compliance test suite ([#85])
Expand Down
145 changes: 145 additions & 0 deletions serde_json_path_core/src/spec/integer.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
//! Representation of itegers in the JSONPath specification
//!
//! The JSONPath specification defines some rules for integers used in query strings (see [here][spec]).
//!
//! [spec]: https://www.rfc-editor.org/rfc/rfc9535.html#name-overview

use std::{
num::{ParseIntError, TryFromIntError},
str::FromStr,
};

/// An integer for internet JSON ([RFC7493][ijson])
///
/// The value must be within the range [-(2<sup>53</sup>)+1, (2<sup>53</sup>)-1]).
///
/// [ijson]: https://www.rfc-editor.org/rfc/rfc7493#section-2.2
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Default)]
pub struct Integer(i64);

/// The maximum allowed value, 2^53 - 1
const MAX: i64 = 9_007_199_254_740_992 - 1;
/// The minimum allowed value (-2^53) + 1
const MIN: i64 = -9_007_199_254_740_992 + 1;

impl Integer {
fn try_new(value: i64) -> Result<Self, IntegerError> {
if !(MIN..=MAX).contains(&value) {
Err(IntegerError::OutOfBounds)
} else {
Ok(Self(value))
}
}

fn check(&self) -> bool {
(MIN..=MAX).contains(&self.0)
}

/// Produce an [`Integer`] with the value 0
pub fn zero() -> Self {
Self(0)
}

/// Get an [`Integer`] from an `i64`
///
/// This is intended for initializing an integer with small, non-zero numbers.
///
/// # Panics
///
/// This will panic if the inputted value is out of the valid range
/// [-(2<sup>53</sup>)+1, (2<sup>53</sup>)-1]).
pub fn from_i64_unchecked(value: i64) -> Self {
Copy link
Contributor

@Marcono1234 Marcono1234 Aug 4, 2024

Choose a reason for hiding this comment

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

Would it make sense to additionally offer conversion functions (or From) for i32? i32 is probably somewhat common and can always be converted to Integer without an error.

Maybe also for u32?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Certainly, I have no issues with extending the API. Since it is public, as a result of being in core, then doing so would be useful.

Self::try_new(value).expect("value is out of the valid range")
}

/// Take the absolute value, producing `None` if the resulting value is outside
/// the valid range [-(2<sup>53</sup>)+1, (2<sup>53</sup>)-1]).
pub fn checked_abs(mut self) -> Option<Self> {
self.0 = self.0.checked_abs()?;
self.check().then_some(self)
}
Comment on lines +55 to +60
Copy link
Contributor

@Marcono1234 Marcono1234 Aug 4, 2024

Choose a reason for hiding this comment

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

Can this actually ever return None? The Integer limits don't permit i64::MIN so I think self.0.checked_abs() always returns Some.
And for the value range of Integer MIN.abs() == MAX, so self.check() should always be true.

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is a great point. I think this can change to just return Self


/// Add the two values, producing `None` if the resulting value is outside the
/// valid range [-(2<sup>53</sup>)+1, (2<sup>53</sup>)-1]).
pub fn checked_add(mut self, rhs: Self) -> Option<Self> {
self.0 = self.0.checked_add(rhs.0)?;
self.check().then_some(self)
}
Comment on lines +64 to +67
Copy link
Contributor

@Marcono1234 Marcono1234 Aug 4, 2024

Choose a reason for hiding this comment

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

Out of curiosity (applies to all the mutating methods): Is this pattern of both mutating self and returning self that common (when the struct is not a 'builder')? It might be a bit error-prone, e.g. users might not expect (or notice) that self is mutated. Should instead a new Integer be returned, and self not be mutated?

Also, I am not sure if this approach of first doing self.0 = ... and then calling self.check() is safe. If the caller ignores the result, or continues to use the Integer afterwards, it now contains an invalid value.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, this is definitely a foot gun. I don't think it is a problem from where Integer is used in serde_json_path, but I will definitely change this.

Since these changes are not released yet I can make breaking changes to the API of Integer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, my comment was wrong. Since it is taking mut self and not &mut self it was actually behaving as expected since Integer is Copy, and actually ended up creating a copy. But the code might still be a bit confusing then.


/// Subtract the `rhs` from `self`, producing `None` if the resulting value is
/// outside the valid range [-(2<sup>53</sup>)+1, (2<sup>53</sup>)-1]).
pub fn checked_sub(mut self, rhs: Self) -> Option<Self> {
self.0 = self.0.checked_sub(rhs.0)?;
self.check().then_some(self)
}

/// Multiply the two values, producing `None` if the resulting value is outside
/// the valid range [-(2<sup>53</sup>)+1, (2<sup>53</sup>)-1]).
pub fn checked_mul(mut self, rhs: Self) -> Option<Self> {
self.0 = self.0.checked_mul(rhs.0)?;
self.check().then_some(self)
}
}

impl TryFrom<i64> for Integer {
type Error = IntegerError;

fn try_from(value: i64) -> Result<Self, Self::Error> {
Self::try_new(value)
}
}

impl TryFrom<usize> for Integer {
type Error = IntegerError;

fn try_from(value: usize) -> Result<Self, Self::Error> {
i64::try_from(value)
.map_err(|_| IntegerError::OutOfBounds)
.and_then(Self::try_new)
}
}

impl FromStr for Integer {
type Err = IntegerError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
s.parse::<i64>().map_err(Into::into).and_then(Self::try_new)
}
}

impl std::fmt::Display for Integer {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.0)
}
}

impl TryFrom<Integer> for usize {
type Error = TryFromIntError;

fn try_from(value: Integer) -> Result<Self, Self::Error> {
Self::try_from(value.0)
}
}

impl PartialEq<i64> for Integer {
fn eq(&self, other: &i64) -> bool {
self.0.eq(other)
}
}

impl PartialOrd<i64> for Integer {
fn partial_cmp(&self, other: &i64) -> Option<std::cmp::Ordering> {
self.0.partial_cmp(other)
}
}

/// An error for the [`Integer`] type
#[derive(Debug, thiserror::Error)]
pub enum IntegerError {
/// The provided value was outside the valid range [-(2**53)+1, (2**53)-1])
#[error("the provided integer was outside the valid range, see https://www.rfc-editor.org/rfc/rfc9535.html#section-2.1-4.1")]
OutOfBounds,
/// Integer parsing error
#[error(transparent)]
Parse(#[from] ParseIntError),
}
1 change: 1 addition & 0 deletions serde_json_path_core/src/spec/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Types representing the IETF JSONPath Standard
pub mod functions;
pub mod integer;
pub mod query;
pub mod segment;
pub mod selector;
14 changes: 6 additions & 8 deletions serde_json_path_core/src/spec/selector/index.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
//! Index selectors in JSONPath
use serde_json::Value;

use crate::{node::LocatedNode, path::NormalizedPath, spec::query::Queryable};
use crate::{
node::LocatedNode,
path::NormalizedPath,
spec::{integer::Integer, query::Queryable},
};

/// For selecting array elements by their index
///
/// Can use negative indices to index from the end of an array
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub struct Index(pub isize);
pub struct Index(pub Integer);

impl std::fmt::Display for Index {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
Expand Down Expand Up @@ -65,9 +69,3 @@ impl Queryable for Index {
}
}
}

impl From<isize> for Index {
fn from(i: isize) -> Self {
Self(i)
}
}
Loading