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

Refactor Ranges so that they are easier to use #190

Merged
merged 6 commits into from
Jul 6, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
227 changes: 98 additions & 129 deletions ion-schema/src/constraint.rs

Large diffs are not rendered by default.

45 changes: 45 additions & 0 deletions ion-schema/src/ion_extension.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
use ion_rs::element::{Element, Value};
use ion_rs::external::bigdecimal::BigDecimal;
use ion_rs::{Decimal, Int};
use num_traits::ToPrimitive;
use std::str::FromStr;

/// Trait for adding extensions to [`Element`] that are useful for implementing Ion Schema.
pub(crate) trait ElementExtensions {
/// Returns the value as an `usize` if it is an Ion Int that can be represented as such.
fn as_usize(&self) -> Option<usize>;
/// Returns the value as an `u64` if it is an Ion Int that can be represented as such.
fn as_u64(&self) -> Option<u64>;
/// Returns the value as a [`Decimal`] if it is any numeric Ion value that can be represented as a [`Decimal`].
fn any_number_as_decimal(&self) -> Option<Decimal>;
}
impl ElementExtensions for Element {
fn as_usize(&self) -> Option<usize> {
match self.value() {
Value::Int(Int::I64(i)) => i.to_usize(),
Value::Int(Int::BigInt(i)) => i.to_usize(),
_ => None,
}
}
Comment on lines +20 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking concern: for these conversions, note that it will not be possible to tell the difference between "The Element was not an integer" and "The Element was an integer that couldn't fit in a usize."

Something I plan to eventually add to Element is a series of expect_TYPE() -> IonResult<T> methods so the caller can receive a more detailed error if things go wrong. It also makes it easy to propagate errors with ? where needed.

Also: the next release of ion-rs makes Int and UInt into opaque types with From and TryFrom implementations for the various integer types. Nothing to do for the moment though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non-blocking concern: for these conversions, note that it will not be possible to tell the difference between "The Element was not an integer" and "The Element was an integer that couldn't fit in a usize."

I think that scenario is unlikely to be a problem—it will only occur if people are using numbers that are too large for u64 and usize as arguments to constraints in their ISL. We can revisit it once the expect_TYPE() functions are available. For now, I'll make the documentation a little more explicit.

fn as_u64(&self) -> Option<u64> {
match self.value() {
Value::Int(Int::I64(i)) => i.to_u64(),
Value::Int(Int::BigInt(i)) => i.to_u64(),
_ => None,
}
}
fn any_number_as_decimal(&self) -> Option<Decimal> {
match self.value() {
Value::Int(Int::I64(i)) => Some(Decimal::from(*i)),
Value::Int(Int::BigInt(i)) => Some(Decimal::from(BigDecimal::from(i.clone()))),
popematt marked this conversation as resolved.
Show resolved Hide resolved
Value::Float(f) => BigDecimal::from_str(&format!(
popematt marked this conversation as resolved.
Show resolved Hide resolved
"{f:.PRECISION$e}",
PRECISION = f64::MANTISSA_DIGITS as usize
))
.ok()
.map(|it| it.into()),
Value::Decimal(d) => Some(d.clone()),
_ => None,
}
}
}
292 changes: 96 additions & 196 deletions ion-schema/src/isl/isl_constraint.rs

Large diffs are not rendered by default.

1,094 changes: 0 additions & 1,094 deletions ion-schema/src/isl/isl_range.rs

This file was deleted.

70 changes: 26 additions & 44 deletions ion-schema/src/isl/isl_type_reference.rs
Original file line number Diff line number Diff line change
@@ -1,25 +1,27 @@
use crate::ion_extension::ElementExtensions;
use crate::isl::isl_import::{IslImport, IslImportType};
use crate::isl::isl_range::{Range, RangeType};
use crate::isl::isl_type::IslTypeImpl;
use crate::isl::ranges::{Limit, UsizeRange};
use crate::isl::IslVersion;
use crate::isl::WriteToIsl;
use crate::isl_require;
use crate::result::{
invalid_schema_error, invalid_schema_error_raw, unresolvable_schema_error, IonSchemaResult,
};
use crate::system::{PendingTypes, TypeId, TypeStore};
use crate::type_reference::{TypeReference, VariablyOccurringTypeRef};
use crate::types::TypeDefinitionImpl;
use ion_rs::element::Element;
use ion_rs::element::{Element, Value};
use ion_rs::{IonType, IonWriter};

/// Provides public facing APIs for constructing ISL type references programmatically for ISL 1.0
pub mod v_1_0 {
use crate::isl::isl_constraint::{IslConstraint, IslConstraintImpl};
use crate::isl::isl_range::Range;
use crate::isl::isl_type::IslTypeImpl;
use crate::isl::isl_type_reference::{
IslTypeRef, IslTypeRefImpl, IslVariablyOccurringTypeRef, NullabilityModifier,
};
use crate::isl::ranges::UsizeRange;
use ion_rs::IonType;

/// Creates a named [IslTypeRef] using the name of the type referenced inside it
Expand Down Expand Up @@ -54,7 +56,7 @@ pub mod v_1_0 {
/// Creates an [IslVariablyOccurringTypeRef] using the [IslConstraint]s and [Range] referenced inside it
pub fn variably_occurring_type_ref(
type_ref: IslTypeRef,
occurs: Range,
occurs: UsizeRange,
) -> IslVariablyOccurringTypeRef {
IslVariablyOccurringTypeRef::new(type_ref, occurs)
}
Expand All @@ -63,10 +65,10 @@ pub mod v_1_0 {
/// Provides public facing APIs for constructing ISL type references programmatically for ISL 2.0
pub mod v_2_0 {
use crate::isl::isl_constraint::IslConstraint;
use crate::isl::isl_range::Range;
use crate::isl::isl_type_reference::{
v_1_0, IslTypeRef, IslTypeRefImpl, IslVariablyOccurringTypeRef, NullabilityModifier,
};
use crate::isl::ranges::UsizeRange;

/// Creates a named [IslTypeRef] using the name of the type referenced inside it
pub fn named_type_ref<A: Into<String>>(name: A) -> IslTypeRef {
Expand All @@ -89,7 +91,7 @@ pub mod v_2_0 {
/// Creates an anonymous [IslTypeRef] using the [IslConstraint]s and [Range] referenced inside it
pub fn variably_occurring_type_ref(
type_ref: IslTypeRef,
occurs: Range,
occurs: UsizeRange,
) -> IslVariablyOccurringTypeRef {
v_1_0::variably_occurring_type_ref(type_ref, occurs)
}
Expand Down Expand Up @@ -237,7 +239,7 @@ impl IslTypeRefImpl {
} else {
// for ISL 1.0 `occurs` field is a no op when used with constraints other than `fields` and `ordered_elements`.
// Although ISL 1.0 will treat this `occurs` field as no op it still has to serialize the `occurs` range to see if its a valid range.
IslVariablyOccurringTypeRef::occurs_from_ion_element(occurs_field, isl_version)?;
IslVariablyOccurringTypeRef::occurs_from_ion_element(occurs_field)?;
}
}

Expand Down Expand Up @@ -390,11 +392,11 @@ impl WriteToIsl for IslTypeRefImpl {
#[derive(Debug, Clone, PartialEq)]
pub struct IslVariablyOccurringTypeRef {
type_ref: IslTypeRefImpl,
occurs: Range,
occurs: UsizeRange,
}

impl IslVariablyOccurringTypeRef {
pub(crate) fn new(type_ref: IslTypeRef, occurs: Range) -> Self {
pub(crate) fn new(type_ref: IslTypeRef, occurs: UsizeRange) -> Self {
Self {
type_ref: type_ref.type_reference,
occurs,
Expand All @@ -404,18 +406,18 @@ impl IslVariablyOccurringTypeRef {
pub fn optional(type_ref: IslTypeRef) -> Self {
Self {
type_ref: type_ref.type_reference,
occurs: Range::optional(),
occurs: UsizeRange::zero_or_one(),
}
}

pub fn required(type_ref: IslTypeRef) -> Self {
Self {
type_ref: type_ref.type_reference,
occurs: Range::required(),
occurs: UsizeRange::new_single_value(1),
}
}

pub fn occurs(&self) -> Range {
pub fn occurs(&self) -> UsizeRange {
self.occurs.to_owned()
}

Expand All @@ -433,55 +435,35 @@ impl IslVariablyOccurringTypeRef {
true,
)?;

let occurs: Range = value
let occurs: UsizeRange = value
.as_struct()
.and_then(|s| {
s.get("occurs")
.map(|r| IslVariablyOccurringTypeRef::occurs_from_ion_element(r, isl_version))
.map(IslVariablyOccurringTypeRef::occurs_from_ion_element)
})
.unwrap_or(if constraint_name == "fields" {
Ok(Range::optional())
Ok(UsizeRange::zero_or_one())
} else {
Ok(Range::required())
Ok(UsizeRange::new_single_value(1))
})?;

isl_require!(occurs.upper() != &Limit::Closed(0usize) => "Occurs cannot be 0")?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ isl_require! is a macro I created along the lines of Kotlin's require() function to make it more concise to check ISL syntax. It takes a condition that must be true, and format args for an error message if it's false. You'll see the definition of the macro further down in result.rs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

Ok(IslVariablyOccurringTypeRef { type_ref, occurs })
}

fn occurs_from_ion_element(value: &Element, isl_version: IslVersion) -> IonSchemaResult<Range> {
use IonType::*;
fn occurs_from_ion_element(value: &Element) -> IonSchemaResult<UsizeRange> {
if value.is_null() {
return invalid_schema_error(
"expected an integer or integer range for an `occurs` constraint, found null",
);
}
match value.ion_type() {
Symbol => {
let sym = try_to!(try_to!(value.as_symbol()).text());
match sym {
"optional" => Ok(Range::optional()),
"required" => Ok(Range::required()),
_ => {
invalid_schema_error(format!(
"only optional and required symbols are supported with occurs constraint, found {sym}"
))
}
}
match value.value() {
Value::Symbol(s) if s.text() == Some("optional") => Ok(UsizeRange::zero_or_one()),
Value::Symbol(s) if s.text() == Some("required") => Ok(UsizeRange::new_single_value(1)),
Value::List(_) | Value::Int(_) => {
UsizeRange::from_ion_element(value, Element::as_usize)
}
List | Int => {
if value.ion_type() == Int && value.as_int().unwrap() <= &ion_rs::Int::I64(0) {
return invalid_schema_error("occurs constraint can not be 0");
}
Ok(Range::from_ion_element(
value,
RangeType::NonNegativeInteger,
isl_version,
)?)
}
_ => invalid_schema_error(format!(
"ion type: {:?} is not supported with occurs constraint",
value.ion_type()
)),
_ => invalid_schema_error(format!("Invalid occurs value: {value}")),
}
}

Expand Down
Loading