diff --git a/ion-schema-tests-runner/src/lib.rs b/ion-schema-tests-runner/src/lib.rs index b0ac2f7..83eabe4 100644 --- a/ion-schema-tests-runner/src/lib.rs +++ b/ion-schema-tests-runner/src/lib.rs @@ -73,7 +73,7 @@ struct MacroArgs { /// /// ion_schema_tests!( /// // The root directory of a test suite. -/// root = "../ion-schema-tests/ion_schema_1_0/", +/// root = "ion-schema-tests/ion_schema_1_0/", /// // Optional, a list of tests to mark as `#[ignore]`. /// // Strings can be test names or regexes that match test names. /// ignored( diff --git a/ion-schema-tests-runner/tests/ion-schema-tests-2-0.rs b/ion-schema-tests-runner/tests/ion-schema-tests-2-0.rs index ca26e99..bcfc17f 100644 --- a/ion-schema-tests-runner/tests/ion-schema-tests-2-0.rs +++ b/ion-schema-tests-runner/tests/ion-schema-tests-2-0.rs @@ -5,17 +5,8 @@ ion_schema_tests!( // Support for ISL 2.0 is not completely implemented yet, so some tests are ignored. ignored( "imports", - "schema::*", - "null_or::*", - "constraints::contains", "constraints::ordered_elements", "constraints::precision", "constraints::regex::value_should_be_invalid_for_type_regex_unescaped_newline__2_", // https://github.com/amazon-ion/ion-rust/issues/399 - "constraints::timestamp_precision", - // following tests are related to: https://github.com/amazon-ion/ion-rust/pull/553 - "constraints::valid_values_ranges::value_should_be_valid_for_type_valid_values_range_timestamp_known_offset__12_", - "constraints::valid_values_ranges::value_should_be_valid_for_type_valid_values_range_timestamp_known_offset__13_", - "constraints::valid_values_ranges::value_should_be_valid_for_type_valid_values_range_timestamp_unknown_offset__12_", - "constraints::valid_values_ranges::value_should_be_valid_for_type_valid_values_range_timestamp_unknown_offset__13_" ) ); diff --git a/ion-schema/src/constraint.rs b/ion-schema/src/constraint.rs index 4a0c77b..12dd6cf 100644 --- a/ion-schema/src/constraint.rs +++ b/ion-schema/src/constraint.rs @@ -1175,14 +1175,21 @@ impl ConstraintValidator for ContainsConstraint { // Create a peekable iterator for given sequence let values: Vec = match &value { IonSchemaElement::SingleElement(element) => { - match element.as_sequence() { - None => { + match element.value() { + Value::List(ion_sequence) | Value::SExp(ion_sequence) => { + ion_sequence.elements().map(|a| a.to_owned()).collect() + } + Value::Struct(ion_struct) => ion_struct + .fields() + .map(|(name, value)| value.to_owned()) + .collect(), + _ => { // return Violation if value is not an Ion sequence return Err(Violation::new( "contains", ViolationCode::TypeMismatched, &format!( - "expected list/sexp found {}", + "expected list/sexp/struct/document found {}", if element.is_null() { format!("{element}") } else { @@ -1192,7 +1199,6 @@ impl ConstraintValidator for ContainsConstraint { ion_path, )); } - Some(ion_sequence) => ion_sequence.elements().map(|a| a.to_owned()).collect(), } } IonSchemaElement::Document(document) => document.to_owned(), diff --git a/ion-schema/src/isl/isl_constraint.rs b/ion-schema/src/isl/isl_constraint.rs index 6eeefd1..010d4e2 100644 --- a/ion-schema/src/isl/isl_constraint.rs +++ b/ion-schema/src/isl/isl_constraint.rs @@ -583,6 +583,10 @@ impl IslConstraintImpl { )); } + if !value.annotations().is_empty() { + return invalid_schema_error("contains list can not have any annotations"); + } + let values: Vec = value .as_sequence() .unwrap() diff --git a/ion-schema/src/isl/isl_type.rs b/ion-schema/src/isl/isl_type.rs index 343dded..b7ffb73 100644 --- a/ion-schema/src/isl/isl_type.rs +++ b/ion-schema/src/isl/isl_type.rs @@ -165,7 +165,14 @@ impl IslTypeImpl { "type names must be a symbol with defined text", )) } - Some(name) => Some(name.to_owned()), + Some(name) => { + if !name_element.annotations().is_empty() { + return Err(invalid_schema_error_raw( + "type names must be a non null and unannotated symbol with defined text", + )); + } + Some(name.to_owned()) + } }, None => { return Err(invalid_schema_error_raw( diff --git a/ion-schema/src/isl/isl_type_reference.rs b/ion-schema/src/isl/isl_type_reference.rs index 80d3912..89dec2f 100644 --- a/ion-schema/src/isl/isl_type_reference.rs +++ b/ion-schema/src/isl/isl_type_reference.rs @@ -338,7 +338,9 @@ impl IslTypeRefImpl { } IslTypeRefImpl::TypeImport(isl_import_type, type_ref_modifier) => { // verify if the inline import type already exists in the type_store - match type_store.get_type_id_by_name(isl_import_type.type_name()) { + match type_store + .get_defined_type_id_or_imported_type_id_by_name(isl_import_type.type_name()) + { None => unresolvable_schema_error(format!( "inline import type: {} does not exists", isl_import_type.type_name() diff --git a/ion-schema/src/isl/ranges.rs b/ion-schema/src/isl/ranges.rs index 4338d3c..9925133 100644 --- a/ion-schema/src/isl/ranges.rs +++ b/ion-schema/src/isl/ranges.rs @@ -132,7 +132,24 @@ pub type TimestampRange = base::Range; impl RangeValidation for TimestampRange {} pub type TimestampPrecisionRange = base::Range; -impl RangeValidation for TimestampPrecisionRange {} +impl RangeValidation for TimestampPrecisionRange { + fn is_empty(start: &Limit, end: &Limit) -> bool { + match (start, end) { + (Limit::Inclusive(lower), Limit::Inclusive(upper)) => lower > upper, + (Limit::Exclusive(lower), Limit::Inclusive(upper)) + | (Limit::Inclusive(lower), Limit::Exclusive(upper)) => lower >= upper, + (Limit::Exclusive(lower), Limit::Exclusive(upper)) => { + let start_value = lower.int_value(); + let end_value = upper.int_value(); + + // Checking for e.g. range::[exclusive::1, exclusive::2] which is empty. + let adjusted_lower = start_value + 1; + adjusted_lower >= end_value + } + _ => false, + } + } +} // usize does not implement Into // TODO: Remove after https://github.com/amazon-ion/ion-rust/issues/573 is released diff --git a/ion-schema/src/isl/util.rs b/ion-schema/src/isl/util.rs index 1f2fa36..82bb15b 100644 --- a/ion-schema/src/isl/util.rs +++ b/ion-schema/src/isl/util.rs @@ -119,6 +119,21 @@ impl TimestampPrecision { TimestampPrecision::OtherFractionalSeconds(i) => format!("fractional second (10e{i})"), } } + + pub(crate) fn int_value(&self) -> i64 { + use TimestampPrecision::*; + match self { + Year => -4, + Month => -3, + Day => -2, + Minute => -1, + Second => 0, + Millisecond => 3, + Microsecond => 6, + Nanosecond => 9, + OtherFractionalSeconds(scale) => *scale, + } + } } impl TryFrom<&str> for TimestampPrecision { @@ -129,7 +144,7 @@ impl TryFrom<&str> for TimestampPrecision { "year" => TimestampPrecision::Year, "month" => TimestampPrecision::Month, "day" => TimestampPrecision::Day, - "minute" | "hour" => TimestampPrecision::Minute, + "minute" => TimestampPrecision::Minute, "second" => TimestampPrecision::Second, "millisecond" => TimestampPrecision::Millisecond, "microsecond" => TimestampPrecision::Microsecond, @@ -145,30 +160,8 @@ impl TryFrom<&str> for TimestampPrecision { impl PartialOrd for TimestampPrecision { fn partial_cmp(&self, other: &TimestampPrecision) -> Option { - use TimestampPrecision::*; - let self_value = match self { - Year => -4, - Month => -3, - Day => -2, - Minute => -1, - Second => 0, - Millisecond => 3, - Microsecond => 6, - Nanosecond => 9, - OtherFractionalSeconds(scale) => *scale, - }; - - let other_value = match other { - Year => -4, - Month => -3, - Day => -2, - Minute => -1, - Second => 0, - Millisecond => 3, - Microsecond => 6, - Nanosecond => 9, - OtherFractionalSeconds(scale) => *scale, - }; + let self_value = self.int_value(); + let other_value = other.int_value(); Some(self_value.cmp(&other_value)) } diff --git a/ion-schema/src/system.rs b/ion-schema/src/system.rs index f76e195..2684edf 100644 --- a/ion-schema/src/system.rs +++ b/ion-schema/src/system.rs @@ -32,9 +32,7 @@ use crate::schema::Schema; use crate::types::{BuiltInTypeDefinition, Nullability, TypeDefinitionImpl, TypeDefinitionKind}; use crate::{is_isl_version_marker, is_reserved_word, UserReservedFields}; use ion_rs::element::{Annotations, Element}; -use ion_rs::types::IonType::Struct; use ion_rs::IonType; -use regex::Regex; use std::collections::{HashMap, HashSet}; use std::io::ErrorKind; use std::sync::Arc; @@ -178,8 +176,8 @@ impl PendingTypes { return match self.ids_by_name.get(import_type_name) { Some(id) => self.types_by_id[*id] .to_owned() - .and_then(|type_def| match type_def { - TypeDefinitionKind::Named(named_type_def) => Some(Ok(named_type_def)), + .map(|type_def| match type_def { + TypeDefinitionKind::Named(named_type_def) => Ok(named_type_def), TypeDefinitionKind::Anonymous(_) => { unreachable!( "The TypeDefinition for the imported type '{}' was Anonymous.", @@ -193,24 +191,26 @@ impl PendingTypes { ) } }), - None => match type_store.get_type_id_by_name(import_type_name) { - Some(id) => match type_store.types_by_id[*id].to_owned() { - TypeDefinitionKind::Named(named_type_def) => Some(Ok(named_type_def)), - TypeDefinitionKind::Anonymous(_) => { - unreachable!( - "The TypeDefinition for the imported type '{}' was Anonymous.", - import_type_name - ) - } - TypeDefinitionKind::BuiltIn(_) => { - unreachable!( - "The TypeDefinition for the imported type '{}' was a builtin type.", - import_type_name - ) - } - }, - None => None, - }, + None => { + match type_store.get_defined_type_id_or_imported_type_id_by_name(import_type_name) { + Some(id) => match type_store.types_by_id[*id].to_owned() { + TypeDefinitionKind::Named(named_type_def) => Some(Ok(named_type_def)), + TypeDefinitionKind::Anonymous(_) => { + unreachable!( + "The TypeDefinition for the imported type '{}' was Anonymous.", + import_type_name + ) + } + TypeDefinitionKind::BuiltIn(_) => { + unreachable!( + "The TypeDefinition for the imported type '{}' was a builtin type.", + import_type_name + ) + } + }, + None => None, + } + } }; } @@ -335,7 +335,9 @@ impl PendingTypes { ) -> Option { match self.ids_by_name.get(name) { Some(id) => Some(*id + type_store.types_by_id.len()), - None => type_store.get_type_id_by_name(name).copied(), + None => type_store + .get_defined_type_id_or_imported_type_id_by_name(name) + .copied(), } } @@ -351,6 +353,11 @@ impl PendingTypes { if let Some(exists) = self.ids_by_name.get(name) { return exists.to_owned() + type_store.types_by_id.len(); } + + if let Some(exists) = type_store.imported_type_ids_by_name.get(name) { + return exists.to_owned(); + } + match type_store.update_deferred_type_def(type_def.to_owned(), name) { None => { let type_id = type_id - type_store.types_by_id.len(); @@ -404,7 +411,8 @@ impl PendingTypes { if let Some(exists) = self.ids_by_name.get(&name) { return exists.to_owned() + type_store.types_by_id.len(); } - if let Some(exists) = type_store.get_type_id_by_name(&name) { + if let Some(exists) = type_store.get_defined_type_id_or_imported_type_id_by_name(&name) + { return exists.to_owned(); } } @@ -562,12 +570,23 @@ impl TypeStore { /// Provides the [`TypeId`] associated with given name if it exists in the [`TypeStore`] as a type /// defined within schema (This doesn't include built-in types); Otherwise returns None - pub(crate) fn get_type_id_by_name(&self, name: &str) -> Option<&TypeId> { + pub(crate) fn get_defined_type_id_or_imported_type_id_by_name( + &self, + name: &str, + ) -> Option<&TypeId> { self.ids_by_name .get(name) .or_else(|| self.imported_type_ids_by_name.get(name)) } + /// Provides the [`Type`] associated with given name if it exists in the [`TypeStore`] as a type + /// defined within schema (This doesn't include built-in types and imported types); Otherwise returns None + pub(crate) fn get_type_def_by_name(&self, name: &str) -> Option<&TypeDefinitionKind> { + self.ids_by_name + .get(name) + .and_then(|id| self.types_by_id.get(*id)) + } + /// Provides the [`TypeId`] associated with given type name if it exists in the [`TypeStore`] /// Otherwise returns None pub(crate) fn get_builtin_type_id(&self, type_name: &str) -> Option { @@ -757,6 +776,7 @@ impl Resolver { let mut found_header = false; let mut found_footer = false; + let mut found_type_definition = false; let mut found_isl_version_marker = false; for value in elements { @@ -779,6 +799,26 @@ impl Resolver { }; found_isl_version_marker = true; } else if annotations.contains("schema_header") { + if isl_version == IslVersion::V2_0 { + if found_type_definition { + return invalid_schema_error( + "The schema header must come before top level type definitions", + ); + } + + if found_header { + return invalid_schema_error( + "Schema must only contain a single schema header", + ); + } + + if annotations.len() > 1 { + return invalid_schema_error( + "schema header must not have any other annotations then `schema_header`", + ); + } + } + found_header = true; // if we didn't find an isl version marker before finding a schema header @@ -818,6 +858,13 @@ impl Resolver { } // load types for schema else if annotations.contains("type") { + found_type_definition = true; + + if isl_version == IslVersion::V2_0 && annotations.len() > 1 { + return invalid_schema_error( + "Top level types definitions must not have any other annotations then `type`", + ); + } // if we didn't find an isl version marker before finding a type definition // then isl version will be defaulted to be ISL 1.0 if !found_isl_version_marker { @@ -856,13 +903,17 @@ impl Resolver { else if annotations.contains("schema_footer") { found_footer = true; if isl_version == IslVersion::V2_0 { + if annotations.len() > 1 { + return invalid_schema_error( + "schema footer must not have any other annotations then `schema_footer`", + ); + } let schema_footer = try_to!(value.as_struct()); isl_user_reserved_fields.validate_field_names_in_footer(schema_footer)?; } } else { // open content - if isl_version == IslVersion::V2_0 - && value.ion_type() == IonType::Symbol + if value.ion_type() == IonType::Symbol && !value.is_null() && is_isl_version_marker(value.as_text().unwrap()) { @@ -1079,41 +1130,6 @@ impl Resolver { } unresolvable_schema_error("Unable to load ISL model: ".to_owned() + id) } - - // This is a helper method that returns the ISL version for given schema content - // It returns an error for an ISL version that matches the version marker but doesn't match the existing ISL versions. - fn find_isl_version( - &self, - schema_content: &Vec, - isl_version_marker: Regex, - ) -> IonSchemaResult { - for value in schema_content { - // if find a type definition or a schema header before finding any version marker then this is ISL 1.0 - if value.ion_type() == Struct - && (value.annotations().contains("type") - || value.annotations().contains("schema_header")) - { - // default ISL 1.0 version will be returned - break; - } - // verify if value is an ISL version marker and if it has valid format - if value.ion_type() == IonType::Symbol - && isl_version_marker.is_match(value.as_text().unwrap()) - { - // This implementation supports Ion Schema 1.0 and Ion Schema 2.0 - return match value.as_text().unwrap() { - "$ion_schema_1_0" => Ok(IslVersion::V1_0), - "$ion_schema_2_0" => Ok(IslVersion::V2_0), - _ => invalid_schema_error(format!( - "Unsupported Ion Schema Language version: {value}" - )), - }; - } - } - - // default ISL version 1.0 if no version marker is found - Ok(IslVersion::V1_0) - } } /// Provides functions for instantiating instances of [`Schema`]. diff --git a/ion-schema/src/types.rs b/ion-schema/src/types.rs index 0a57b64..540c830 100644 --- a/ion-schema/src/types.rs +++ b/ion-schema/src/types.rs @@ -3,7 +3,7 @@ use crate::ion_path::IonPath; use crate::isl::isl_constraint::IslConstraintImpl; use crate::isl::isl_type::IslTypeImpl; use crate::isl::IslVersion; -use crate::result::{IonSchemaResult, ValidationResult}; +use crate::result::{invalid_schema_error, IonSchemaResult, ValidationResult}; use crate::system::{PendingTypes, TypeId, TypeStore}; use crate::violation::{Violation, ViolationCode}; use crate::IonSchemaElement; @@ -262,6 +262,14 @@ impl TypeDefinitionKind { )) } + /// Provides a boolean that represents whether this type is deferred type or not + pub fn is_deferred_type_def(&self) -> bool { + match self { + TypeDefinitionKind::Named(type_def) => type_def.is_deferred_type_def, + _ => false, + } + } + /// Creates an anonymous [`TypeDefinitionKind`] using the [`Constraint`]s defined within it pub fn anonymous>>(constraints: A) -> TypeDefinitionKind { TypeDefinitionKind::Anonymous(TypeDefinitionImpl::new(None, constraints.into(), None)) @@ -449,9 +457,17 @@ impl TypeDefinitionImpl { // parses an isl_type to a TypeDefinition let type_name = isl_type.name(); - // add parent information for named type - if type_name.is_some() { - pending_types.add_parent(type_name.to_owned().unwrap(), type_store); + if let Some(type_name) = type_name { + if let Some(type_def) = type_store.get_type_def_by_name(type_name) { + if isl_version == IslVersion::V2_0 && !type_def.is_deferred_type_def() { + return invalid_schema_error(format!( + "The schema document can not have two type definitions with same name: {}", + type_name + )); + } + } + // add parent information for named type + pending_types.add_parent(type_name.to_owned(), type_store); } // add this unresolved type to context for type_id