Skip to content

Delete schema-based parsing for action attribtues #1194

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

Merged
merged 2 commits into from
Sep 11, 2024
Merged

Delete schema-based parsing for action attribtues #1194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
979609b
Remove schema-based-parsing for action attributes
john-h-kastner-aws Sep 10, 2024
4b8c350
Remove some dead code
john-h-kastner-aws Sep 10, 2024
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
12 changes: 6 additions & 6 deletions cedar-policy-core/src/entities.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ pub mod json;
use json::err::JsonSerializationError;

pub use json::{
schematype_of_partialvalue, schematype_of_restricted_expr, AllEntitiesNoAttrsSchema,
AttributeType, CedarValueJson, ContextJsonParser, ContextSchema, EntityJson, EntityJsonParser,
EntityTypeDescription, EntityUidJson, FnAndArg, GetSchemaTypeError, HeterogeneousSetError,
NoEntitiesSchema, NoStaticContext, Schema, SchemaType, TypeAndId,
schematype_of_restricted_expr, AllEntitiesNoAttrsSchema, AttributeType, CedarValueJson,
ContextJsonParser, ContextSchema, EntityJson, EntityJsonParser, EntityTypeDescription,
EntityUidJson, FnAndArg, GetSchemaTypeError, HeterogeneousSetError, NoEntitiesSchema,
NoStaticContext, Schema, SchemaType, TypeAndId,
};

use conformance::EntitySchemaConformanceChecker;
Expand Down Expand Up @@ -2873,7 +2873,7 @@ mod schema_based_parsing_tests {
expect_err(
&entitiesjson,
&miette::Report::new(e),
&ExpectedErrorMessageBuilder::error("error during entity deserialization")
&ExpectedErrorMessageBuilder::error("entity does not conform to the schema")
.source(r#"found action entity `Action::"update"`, but it was not declared as an action in the schema"#)
.build()
);
Expand Down Expand Up @@ -3036,7 +3036,7 @@ mod schema_based_parsing_tests {
expect_err(
&entitiesjson,
&miette::Report::new(e),
&ExpectedErrorMessageBuilder::error("error during entity deserialization")
&ExpectedErrorMessageBuilder::error("entity does not conform to the schema")
.source(r#"definition of action `Action::"view"` does not match its schema declaration"#)
.help(r#"to use the schema's definition of `Action::"view"`, simply omit it from the entities input data"#)
.build()
Expand Down
7 changes: 0 additions & 7 deletions cedar-policy-core/src/entities/conformance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,13 +257,6 @@ pub fn typecheck_restricted_expr_against_schematype(
// type error.
Ok(())
}
Err(GetSchemaTypeError::NontrivialResidual { .. }) => {
// this case is unreachable according to the invariant in the comments
// on `schematype_of_restricted_expr()`.
// Nonetheless, rather than relying on that invariant, it's safe to
// treat this case like the case above and consider this as passing.
Ok(())
}
Err(GetSchemaTypeError::HeterogeneousSet(err)) => {
Err(TypecheckError::HeterogeneousSet(err))
}
Expand Down
69 changes: 6 additions & 63 deletions cedar-policy-core/src/entities/json/entities.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use crate::ast::{BorrowedRestrictedExpr, Entity, EntityUID, PartialValue, Restri
use crate::entities::conformance::EntitySchemaConformanceChecker;
use crate::entities::{
conformance::err::{EntitySchemaConformanceError, UnexpectedEntityTypeError},
schematype_of_partialvalue, Entities, EntitiesError, GetSchemaTypeError, TCComputation,
Entities, EntitiesError, TCComputation,
};
use crate::extensions::Extensions;
use crate::jsonvalue::JsonValueWithNoDuplicateKeys;
Expand Down Expand Up @@ -79,11 +79,10 @@ pub struct EntityJsonParser<'e, 's, S: Schema = NoEntitiesSchema> {
/// Schema information about a single entity can take one of these forms:
#[derive(Debug)]
enum EntitySchemaInfo<E: EntityTypeDescription> {
/// There is no schema, i.e. we're not doing schema-based parsing
/// There is no schema, i.e. we're not doing schema-based parsing. We don't
/// have attribute type information in the schema for action entities, so
/// these are also parsed without schema-based parsing.
NoSchema,
/// The entity is an action, and here's the schema's copy of the
/// `Entity` object for it
Action(Arc<Entity>),
/// The entity is a non-action, and here's the schema's information
/// about its type
NonAction(E),
Expand Down Expand Up @@ -274,11 +273,8 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> {
None => EntitySchemaInfo::NoSchema,
Some(schema) => {
if etype.is_action() {
EntitySchemaInfo::Action(schema.action(&uid).ok_or(
JsonDeserializationError::EntitySchemaConformance(
EntitySchemaConformanceError::undeclared_action(uid.clone()),
),
)?)
// Action entities do not have attribute type information in the schema.
EntitySchemaInfo::NoSchema
} else {
EntitySchemaInfo::NonAction(schema.entity_type(etype).ok_or_else(|| {
let suggested_types = schema
Expand Down Expand Up @@ -343,59 +339,6 @@ impl<'e, 's, S: Schema> EntityJsonParser<'e, 's, S> {
};
Ok((k.clone(), rexpr))
}
EntitySchemaInfo::Action(action) => {
// We'll do schema-based parsing assuming optimistically that
// the type in the JSON is the same as the type in the schema.
// (As of this writing, the schema doesn't actually tell us
// what type each action attribute is supposed to be)
Comment on lines -346 to -350
Copy link
Contributor

@cdisselkoen cdisselkoen Sep 11, 2024

Choose a reason for hiding this comment

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

I think this is a behavior change -- suppose you have an action attribute of type ip in the schema, then with this code you're allowed to pass just "192.168.0.1" for that attribute in the entity data (ie, just a string), whereas after this code is removed, you'll need the { __extn: { ... } } form.

however, this is probably inconsequential, because most users are probably not passing actions in their entity data if they are also passing the schema, since the schema contains the action info and we already automatically pull the proper action info from the schema when constructing entities.

Copy link
Contributor Author

@john-h-kastner-aws john-h-kastner-aws Sep 11, 2024

Choose a reason for hiding this comment

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

There is a behavior change for actions with attributes, but the schema doesn't allow that currently.

let expected_val = match action.get(&k) {
// `None` indicates the attribute isn't in the schema's
// copy of the action entity
None => {
return Err(JsonDeserializationError::EntitySchemaConformance(
EntitySchemaConformanceError::action_declaration_mismatch(
uid.clone(),
),
))
}
Some(v) => v,
};
let expected_ty =
match schematype_of_partialvalue(expected_val, self.extensions) {
Ok(ty) => Ok(Some(ty)),
Err(GetSchemaTypeError::HeterogeneousSet(err)) => {
Err(JsonDeserializationError::EntitySchemaConformance(
EntitySchemaConformanceError::heterogeneous_set(
uid.clone(),
k.clone(),
err,
),
))
}
Err(GetSchemaTypeError::ExtensionFunctionLookup(err)) => {
Err(JsonDeserializationError::EntitySchemaConformance(
EntitySchemaConformanceError::extension_function_lookup(
uid.clone(),
k.clone(),
err,
),
))
}
Err(GetSchemaTypeError::UnknownInsufficientTypeInfo { .. })
| Err(GetSchemaTypeError::NontrivialResidual { .. }) => {
// In these cases, we'll just do ordinary non-schema-based parsing.
Ok(None)
}
}?;
let rexpr =
vparser.val_into_restricted_expr(v.into(), expected_ty.as_ref(), || {
JsonDeserializationErrorContext::EntityAttribute {
uid: uid.clone(),
attr: k.clone(),
}
})?;
Ok((k, rexpr))
}
})
.collect::<Result<_, JsonDeserializationError>>()?;
let is_parent_allowed = |parent_euid: &EntityUID| {
Expand Down
106 changes: 2 additions & 104 deletions cedar-policy-core/src/entities/json/schema_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,9 @@
* limitations under the License.
*/

use crate::ast::{
BorrowedRestrictedExpr, EntityType, Expr, ExprKind, Literal, PartialValue, Type, Unknown,
Value, ValueKind,
};
use crate::ast::{BorrowedRestrictedExpr, EntityType, ExprKind, Literal, Type, Unknown};
use crate::entities::Name;
use crate::extensions::{ExtensionFunctionLookupError, Extensions};
use crate::impl_diagnostic_from_expr_field;
use itertools::Itertools;
use miette::Diagnostic;
use smol_str::SmolStr;
Expand Down Expand Up @@ -285,13 +281,6 @@ pub enum GetSchemaTypeError {
#[error(transparent)]
#[diagnostic(transparent)]
UnknownInsufficientTypeInfo(#[from] UnknownInsufficientTypeInfoError),
/// Trying to compute the [`SchemaType`] of a nontrivial residual (i.e., a
/// residual which is not just a single `Unknown`). For now, we do not
/// attempt to compute the [`SchemaType`] in these cases, and just return
/// this error.
#[error(transparent)]
#[diagnostic(transparent)]
NontrivialResidual(#[from] NontrivialResidualError),
}

/// Found a set whose elements don't all have the same type. This doesn't match
Expand Down Expand Up @@ -328,22 +317,6 @@ impl std::fmt::Display for UnknownInsufficientTypeInfoError {
}
}

/// Trying to compute the [`SchemaType`] of a nontrivial residual (i.e., a
/// residual which is not just a single `Unknown`). For now, we do not
/// attempt to compute the [`SchemaType`] in these cases, and just return
/// this error.
#[derive(Debug, Error)]
#[error("cannot compute type of nontrivial residual `{residual}`")]
pub struct NontrivialResidualError {
/// Nontrivial residual which we were trying to compute the
/// [`SchemaType`] of
residual: Box<Expr>,
}

impl Diagnostic for NontrivialResidualError {
impl_diagnostic_from_expr_field!(residual);
}

/// Get the [`SchemaType`] of a restricted expression.
///
/// This isn't possible for general `Expr`s (without a request, full schema,
Expand Down Expand Up @@ -410,56 +383,8 @@ pub fn schematype_of_restricted_expr(
}
}

/// Get the [`SchemaType`] of a [`Value`].
///
/// Note that while getting the [`Type`] of a [`Value`] (with `value.type_of()`)
/// is O(1), getting the [`SchemaType`] requires recursively traversing the
/// whole `Value` and is thus O(n).
///
/// If the `Value` is a record, we can't know whether the attributes in the
/// given record are required or optional.
/// This function will return the `SchemaType` where all attributes that appear
/// in the `Value` are required, and no other attributes exist.
/// That is, this assumes that all existing attributes are required, and that no
/// other optional attributes are possible.
pub fn schematype_of_value(value: &Value) -> Result<SchemaType, HeterogeneousSetError> {
schematype_of_valuekind(&value.value)
}

/// Get the [`SchemaType`] of a [`ValueKind`].
///
/// Note that while getting the [`Type`] of a [`ValueKind`] (with `value.type_of()`)
/// is O(1), getting the [`SchemaType`] requires recursively traversing the
/// whole value and is thus O(n).
///
/// If the `ValueKind` is a record, we can't know whether the attributes in the
/// given record are required or optional.
/// This function will return the `SchemaType` where all attributes that appear
/// in the `ValueKind` are required, and no other attributes exist.
/// That is, this assumes that all existing attributes are required, and that no
/// other optional attributes are possible.
pub fn schematype_of_valuekind(value: &ValueKind) -> Result<SchemaType, HeterogeneousSetError> {
match value {
ValueKind::Lit(lit) => Ok(schematype_of_lit(lit)),
ValueKind::Set(set) => {
let element_types = set.iter().map(schematype_of_value);
schematype_of_set_elements(element_types)
}
ValueKind::Record(record) => Ok(SchemaType::Record {
attrs: record
.iter()
.map(|(k, v)| Ok((k.clone(), AttributeType::required(schematype_of_value(v)?))))
.collect::<Result<_, HeterogeneousSetError>>()?,
open_attrs: false,
}),
ValueKind::ExtensionValue(ev) => Ok(SchemaType::Extension {
name: ev.typename(),
}),
}
}

/// Get the [`SchemaType`] of a [`Literal`].
pub fn schematype_of_lit(lit: &Literal) -> SchemaType {
fn schematype_of_lit(lit: &Literal) -> SchemaType {
match lit {
Literal::Bool(_) => SchemaType::Bool,
Literal::Long(_) => SchemaType::Long,
Expand Down Expand Up @@ -497,30 +422,3 @@ fn schematype_of_set_elements<E: From<HeterogeneousSetError>>(
}
}
}

/// Get the [`SchemaType`] of a [`PartialValue`].
///
/// For some residuals, the `SchemaType` cannot be determined without evaluating
/// (or knowing more type information about the unknowns). In those cases, this
/// function returns an appropriate `GetSchemaTypeError`.
///
/// See notes on [`schematype_of_value()`].
pub fn schematype_of_partialvalue(
pvalue: &PartialValue,
extensions: &Extensions<'_>,
) -> Result<SchemaType, GetSchemaTypeError> {
match pvalue {
PartialValue::Value(v) => schematype_of_value(v).map_err(Into::into),
PartialValue::Residual(expr) => match BorrowedRestrictedExpr::new(expr) {
Ok(expr) => schematype_of_restricted_expr(expr, extensions),
Err(_) => {
// the PartialValue is a residual that isn't a valid restricted expression.
// For now we don't try to determine the type in this case.
Err(NontrivialResidualError {
residual: Box::new(expr.clone()),
}
.into())
}
},
}
}