Skip to content

Avoid cloning enum entity type variants lists #2283

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
john-h-kastner-aws merged 3 commits into from
Apr 3, 2026
Merged

Avoid cloning enum entity type variants lists #2283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b98fc5c
Avoid clones if options for entity enum type
john-h-kastner-aws Oct 3, 2025
61ea5d4
fix wasm
john-h-kastner-aws Apr 3, 2026
c4befab
keep non-empty
john-h-kastner-aws Apr 3, 2026
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: 7 additions & 5 deletions cedar-policy-core/src/entities.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2480,7 +2480,7 @@ mod schema_based_parsing_tests {
use serde_json::json;
use smol_str::SmolStr;
use std::collections::{BTreeMap, HashSet};
use std::sync::Arc;
use std::sync::{Arc, LazyLock};

/// Mock schema impl used for most of these tests
struct MockSchema;
Expand Down Expand Up @@ -2566,7 +2566,7 @@ mod schema_based_parsing_tests {
/// Mock schema impl for the `Employee` type used in most of these tests
struct MockEmployeeDescription;
impl EntityTypeDescription for MockEmployeeDescription {
fn enum_entity_eids(&self) -> Option<NonEmpty<Eid>> {
fn enum_entity_eids(&self) -> Option<&NonEmpty<Eid>> {
None
}
fn entity_type(&self) -> EntityType {
Expand Down Expand Up @@ -3824,7 +3824,7 @@ mod schema_based_parsing_tests {

struct MockEmployeeDescription;
impl EntityTypeDescription for MockEmployeeDescription {
fn enum_entity_eids(&self) -> Option<NonEmpty<Eid>> {
fn enum_entity_eids(&self) -> Option<&NonEmpty<Eid>> {
None
}
fn entity_type(&self) -> EntityType {
Expand Down Expand Up @@ -3958,6 +3958,8 @@ mod schema_based_parsing_tests {

#[test]
fn enumerated_entities() {
static TEST_ENUM_EIDS: LazyLock<NonEmpty<Eid>> =
LazyLock::new(|| nonempty::nonempty![Eid::new("🌎"), Eid::new("🌕")]);
struct MockSchema;
struct StarTypeDescription;
impl EntityTypeDescription for StarTypeDescription {
Expand Down Expand Up @@ -3985,8 +3987,8 @@ mod schema_based_parsing_tests {
false
}

fn enum_entity_eids(&self) -> Option<NonEmpty<Eid>> {
Some(nonempty::nonempty![Eid::new("🌎"), Eid::new("🌕"),])
fn enum_entity_eids(&self) -> Option<&NonEmpty<Eid>> {
Some(&TEST_ENUM_EIDS)
}
}
impl Schema for MockSchema {
Expand Down
11 changes: 6 additions & 5 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 @@ -27,6 +27,7 @@ use err::{
UnexpectedEntityTypeError,
};
use miette::Diagnostic;
use nonempty::NonEmpty;
use smol_str::SmolStr;
use std::collections::{BTreeMap, HashMap};
use thiserror::Error;
Expand Down Expand Up @@ -229,17 +230,17 @@ impl<S: Schema> EntitySchemaConformanceChecker<'_, S> {

/// Return an [`InvalidEnumEntityError`] if `uid`'s eid is not among valid `choices`
pub fn is_valid_enumerated_entity(
choices: &[Eid],
choices: &NonEmpty<Eid>,
uid: &EntityUID,
) -> Result<(), InvalidEnumEntityError> {
choices
.iter()
.find(|id| uid.eid() == *id)
.any(|id| uid.eid() == id)
.then_some(())
.ok_or_else(|| InvalidEnumEntityError {
uid: uid.clone(),
choices: choices.to_vec(),
choices: choices.clone(),
})
.map(|_| ())
}

/// Errors returned from `validate_euid()` and friends
Expand Down Expand Up @@ -275,7 +276,7 @@ pub fn validate_euid(schema: &impl Schema, euid: &EntityUID) -> Result<(), Valid
let entity_type = euid.entity_type();
if let Some(desc) = schema.entity_type(entity_type) {
if let Some(choices) = desc.enum_entity_eids() {
is_valid_enumerated_entity(&Vec::from(choices), euid)?;
is_valid_enumerated_entity(&choices, euid)?;
}
}
if entity_type.is_action() && schema.action(euid).is_none() {
Expand Down
3 changes: 2 additions & 1 deletion cedar-policy-core/src/entities/conformance/err.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use crate::extensions::ExtensionFunctionLookupError;
use crate::impl_diagnostic_from_method_on_field;
use itertools::Itertools;
use miette::Diagnostic;
use nonempty::NonEmpty;
use smol_str::SmolStr;
use thiserror::Error;

Expand Down Expand Up @@ -334,7 +335,7 @@ pub struct InvalidEnumEntityError {
/// Entity where the error occurred
pub uid: EntityUID,
/// Name of the attribute where the error occurred
pub choices: Vec<Eid>,
pub choices: NonEmpty<Eid>,
}

impl Diagnostic for InvalidEnumEntityError {
Expand Down
4 changes: 2 additions & 2 deletions cedar-policy-core/src/entities/json/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ pub trait EntityTypeDescription {

/// Return valid EID choices if the entity type is enumerated otherwise
/// return `None`
fn enum_entity_eids(&self) -> Option<NonEmpty<Eid>>;
fn enum_entity_eids(&self) -> Option<&NonEmpty<Eid>>;
}

/// Simple type that implements `EntityTypeDescription` by expecting no
Expand Down Expand Up @@ -171,7 +171,7 @@ impl EntityTypeDescription for NullEntityTypeDescription {
fn open_attributes(&self) -> bool {
false
}
fn enum_entity_eids(&self) -> Option<NonEmpty<Eid>> {
fn enum_entity_eids(&self) -> Option<&NonEmpty<Eid>> {
None
}
}
Expand Down
24 changes: 6 additions & 18 deletions cedar-policy-core/src/tpe/request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,7 @@ impl PartialRequest {
..
} = principal_ty
{
is_valid_enumerated_entity(
&Vec::from(choices.clone().map(Eid::new)),
&uid,
)?;
is_valid_enumerated_entity(choices, &uid)?;
}
}
} else {
Expand All @@ -174,10 +171,7 @@ impl PartialRequest {
..
} = resource_ty
{
is_valid_enumerated_entity(
&Vec::from(choices.clone().map(Eid::new)),
&uid,
)?;
is_valid_enumerated_entity(choices, &uid)?;
}
}
} else {
Expand Down Expand Up @@ -407,11 +401,8 @@ impl<'s> RequestBuilder<'s> {
..
} = principal_ty
{
is_valid_enumerated_entity(
&Vec::from(choices.clone().map(Eid::new)),
candidate,
)
.map_err(RequestBuilderError::InvalidPrincipalCandidate)?;
is_valid_enumerated_entity(choices, candidate)
.map_err(RequestBuilderError::InvalidPrincipalCandidate)?;
}
self.partial_request.principal = PartialEntityUID {
ty: candidate.entity_type().clone(),
Expand Down Expand Up @@ -457,11 +448,8 @@ impl<'s> RequestBuilder<'s> {
..
} = resource_ty
{
is_valid_enumerated_entity(
&Vec::from(choices.clone().map(Eid::new)),
candidate,
)
.map_err(RequestBuilderError::InvalidResourceCandidate)?;
is_valid_enumerated_entity(choices, candidate)
.map_err(RequestBuilderError::InvalidResourceCandidate)?;
}
self.partial_request.resource = PartialEntityUID {
ty: candidate.entity_type().clone(),
Expand Down
13 changes: 7 additions & 6 deletions cedar-policy-core/src/validator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,7 @@ mod enumerated_entity_types {
};
use cool_asserts::assert_matches;
use itertools::Itertools;
use nonempty::nonempty;

use crate::validator::{
typecheck::test::test_utils::get_loc,
Expand Down Expand Up @@ -675,7 +676,7 @@ mod enumerated_entity_types {
validator.validate_policy(&template, crate::validator::ValidationMode::Strict);
assert!(warnings.collect_vec().is_empty());
assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => {
assert_eq!(err.err.choices, vec![Eid::new("foo")]);
assert_eq!(err.err.choices, nonempty![Eid::new("foo")]);
assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "fo").unwrap());
});

Expand All @@ -689,7 +690,7 @@ mod enumerated_entity_types {
validator.validate_policy(&template, crate::validator::ValidationMode::Strict);
assert!(warnings.collect_vec().is_empty());
assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => {
assert_eq!(err.err.choices, vec![Eid::new("foo")]);
assert_eq!(err.err.choices, nonempty![Eid::new("foo")]);
assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "🏈").unwrap());
});

Expand All @@ -703,7 +704,7 @@ mod enumerated_entity_types {
validator.validate_policy(&template, crate::validator::ValidationMode::Strict);
assert!(warnings.collect_vec().is_empty());
assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => {
assert_eq!(err.err.choices, vec![Eid::new("foo")]);
assert_eq!(err.err.choices, nonempty![Eid::new("foo")]);
assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "🏈").unwrap());
});

Expand All @@ -719,7 +720,7 @@ mod enumerated_entity_types {
validator.validate_policy(&template, crate::validator::ValidationMode::Strict);
assert!(warnings.collect_vec().is_empty());
assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => {
assert_eq!(err.err.choices, vec![Eid::new("foo")]);
assert_eq!(err.err.choices, nonempty![Eid::new("foo")]);
assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "🏈").unwrap());
});

Expand All @@ -735,7 +736,7 @@ mod enumerated_entity_types {
validator.validate_policy(&template, crate::validator::ValidationMode::Strict);
assert!(warnings.collect_vec().is_empty());
assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => {
assert_eq!(err.err.choices, vec![Eid::new("foo")]);
assert_eq!(err.err.choices, nonempty![Eid::new("foo")]);
assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "🏈").unwrap());
});

Expand All @@ -751,7 +752,7 @@ mod enumerated_entity_types {
validator.validate_policy(&template, crate::validator::ValidationMode::Strict);
assert!(warnings.collect_vec().is_empty());
assert_matches!(&errors.collect_vec(), [ValidationError::InvalidEnumEntity(err)] => {
assert_eq!(err.err.choices, vec![Eid::new("foo")]);
assert_eq!(err.err.choices, nonempty![Eid::new("foo")]);
assert_eq!(err.err.uid, EntityUID::with_eid_and_type("Foo", "🏈").unwrap());
});
}
Expand Down
4 changes: 2 additions & 2 deletions cedar-policy-core/src/validator/cedar_schema/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
use std::{collections::BTreeMap, iter::once};

use crate::{
ast::{Annotation, Annotations, AnyId, Id, InternalName},
ast::{Annotation, Annotations, AnyId, Eid, Id, InternalName},
parser::{Loc, Node},
};
use itertools::{Either, Itertools};
Expand Down Expand Up @@ -293,7 +293,7 @@ pub struct StandardEntityDecl {
#[derive(Debug, Clone)]
pub struct EnumEntityDecl {
pub names: NonEmpty<Node<Id>>,
pub choices: NonEmpty<Node<SmolStr>>,
pub choices: NonEmpty<Node<Eid>>,
}

/// Type definitions
Expand Down
2 changes: 1 addition & 1 deletion cedar-policy-core/src/validator/cedar_schema/fmt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ impl<N: Display> IndentedDisplay for json_schema::EntityType<N> {
" enum [{}]",
choices
.iter()
.map(|e| format!("\"{}\"", e.escape_debug()))
.map(|e| format!("\"{}\"", e.as_ref().escape_debug()))
.join(", ")
),
}
Expand Down
11 changes: 8 additions & 3 deletions cedar-policy-core/src/validator/cedar_schema/grammar.lalrpop
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use crate::parser::{Node, Loc, unescape::to_unescaped_string, cst::Ref};
use crate::ast::{Id, AnyId, Annotations};
use smol_str::SmolStr;
use smol_str::ToSmolStr;
use crate::ast::Eid;
use crate::validator::cedar_schema::ast::{
Path,
EntityDecl,
Expand Down Expand Up @@ -137,7 +138,7 @@ Entity: Node<Declaration> = {
attrs: Node::with_source_loc(ds.map(|ds| ds.unwrap_or_default()).unwrap_or_default(), Loc::new(l2..r2, Arc::clone(src))),
tags: ts,
})), Loc::new(l1..r1, Arc::clone(src))),
<l:@L> ENTITY <ets: Idents> ENUM "[" <choices: STRs> "]" ";" <r:@R> => Node::with_source_loc(Declaration::Entity(EntityDecl::Enum(EnumEntityDecl {
<l:@L> ENTITY <ets: Idents> ENUM "[" <choices: Eids> "]" ";" <r:@R> => Node::with_source_loc(Declaration::Entity(EntityDecl::Enum(EnumEntityDecl {
names: ets,
choices,
})), Loc::new(l..r, Arc::clone(src))),
Expand Down Expand Up @@ -280,6 +281,10 @@ STR: Node<SmolStr> = {
},
}

Eid: Node<Eid> = {
<s: STR> => s.map(Eid::new)
}

// Name := IDENT | STR
Name: Node<SmolStr> = {
<id: Ident> => id.map(|id| id.to_smolstr()),
Expand Down Expand Up @@ -319,8 +324,8 @@ Names: NonEmpty<Node<SmolStr>> = NonEmptyComma<Name>;
// Qualnames := Qualname {',' Qualname }
QualNames : NonEmpty<Node<QualName>> = NonEmptyComma<QualName>;

// STRs := STR {',' STR}
STRs: NonEmpty<Node<SmolStr>> = NonEmptyComma<STR>;
// Eids := Eid {',' Eid}
Eids: NonEmpty<Node<Eid>> = NonEmptyComma<Eid>;

PrincipalOrResource: Node<PR> = {
<l:@L> PRINCIPAL <r:@R> => Node::with_source_loc(PR::Principal, Loc::new(l..r, Arc::clone(src))),
Expand Down
22 changes: 13 additions & 9 deletions cedar-policy-core/src/validator/cedar_schema/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -953,9 +953,12 @@ namespace Baz {action "Foo" appliesTo {
}

mod parser_tests {
use crate::validator::cedar_schema::{
ast::{Annotated, Declaration, EntityDecl, EnumEntityDecl, Namespace},
parser::parse_schema,
use crate::{
ast::Eid,
validator::cedar_schema::{
ast::{Annotated, Declaration, EntityDecl, EnumEntityDecl, Namespace},
parser::parse_schema,
},
};
use cool_asserts::assert_matches;

Expand Down Expand Up @@ -1192,7 +1195,7 @@ mod parser_tests {
assert_matches!(&ns, [Annotated {data: Namespace { decls, ..}, ..}, ..] => {
assert_matches!(decls, [Annotated { data, .. }] => {
assert_matches!(&data.node, Declaration::Entity(EntityDecl::Enum(EnumEntityDecl { choices, ..})) => {
assert_eq!(choices.clone().map(|n| n.node), nonempty::NonEmpty::singleton("TinyTodo".into()));
assert_eq!(choices.clone().map(|n| n.node), nonempty::NonEmpty::singleton(Eid::new("TinyTodo")));
});
});
});
Expand All @@ -1207,7 +1210,7 @@ mod parser_tests {
assert_matches!(&ns, [Annotated {data: Namespace { decls, ..}, ..}, ..] => {
assert_matches!(decls, [Annotated { data, .. }] => {
assert_matches!(&data.node, Declaration::Entity(EntityDecl::Enum(EnumEntityDecl { choices, ..})) => {
assert_eq!(choices.clone().map(|n| n.node), nonempty::nonempty!["TinyTodo".into(), "GitHub".into(), "DocumentCloud".into()]);
assert_eq!(choices.clone().map(|n| n.node), nonempty::nonempty![Eid::new("TinyTodo"), Eid::new("GitHub"), Eid::new("DocumentCloud")]);
});
});
});
Expand All @@ -1221,7 +1224,7 @@ mod parser_tests {
assert_matches!(&ns, [Annotated {data: Namespace { decls, ..}, ..}] => {
assert_matches!(decls, [Annotated { data, .. }] => {
assert_matches!(&data.node, Declaration::Entity(EntityDecl::Enum(EnumEntityDecl { choices, ..})) => {
assert_eq!(choices.clone().map(|n| n.node), nonempty::NonEmpty::singleton("enum".into()));
assert_eq!(choices.clone().map(|n| n.node), nonempty::NonEmpty::singleton(Eid::new("enum")));
});
});
});
Expand All @@ -1241,12 +1244,13 @@ mod parser_tests {
}

mod translator_tests {
use crate::ast as cedar_ast;
use crate::ast::{self as cedar_ast, Eid};
use crate::extensions::Extensions;
use crate::test_utils::{expect_err, ExpectedErrorMessageBuilder};
use crate::validator::types::BoolType;
use crate::FromNormalizedStr;
use cool_asserts::assert_matches;
use nonempty::nonempty;

use crate::validator::json_schema::{EntityType, EntityTypeKind};
use crate::validator::{
Expand Down Expand Up @@ -2363,7 +2367,7 @@ mod translator_tests {
json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap();
let ns = schema.0.get(&None).unwrap();
assert_matches!(ns.entity_types.get(&"Fruits".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Enum { choices }, ..} => {
assert_eq!(Vec::from(choices.clone()), ["🍍", "🥭", "🥝"]);
assert_eq!(choices, &nonempty![Eid::new("🍍"), Eid::new("🥭"), Eid::new("🥝")]);
});

let src = r#"
Expand All @@ -2374,7 +2378,7 @@ mod translator_tests {
json_schema::Fragment::from_cedarschema_str(src, Extensions::all_available()).unwrap();
let ns = schema.0.get(&None).unwrap();
assert_matches!(ns.entity_types.get(&"enum".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Enum { choices }, ..} => {
assert_eq!(Vec::from(choices.clone()), ["enum"]);
assert_eq!(choices, &nonempty![Eid::new("enum")]);
});
}
}
Expand Down
Loading
Loading