TypeNotDefined error on a minimal schema w/ built-in types

[ericox]

Before opening, please confirm:

• I have searched for duplicate or closed issues.
• I have read the guide for submitting bug reports.
• I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.

Bug Category

Schemas and Validation

Describe the bug

A valid schema fails with a TypeNotDefined error when building a raw schema fragment.

Expected behavior

The schema fragment should be constructed w/o panicking.

Reproduction steps

1. create a Cedar schema file on local disk with one entity

entity User {
   id: String,
    name: String,
 }

2. run the code snipped below in a main().

Code Snippet

Given a schema with a single entity definition

entity User {
    id: String,
    name: String,
};

Loading a schema from a file,

  let buf = build_buffer();
  let mut u = Unstructured::new(&buf);
  let fragment = json_schema::Fragment::<RawName>::from_cedarschema_file(
        File::open(Path::new("data/quip.cedarschema")).unwrap(),
        Extensions::all_available(),
    )
    .unwrap()
    .0;
    let schema = Schema::from_raw_schemafrag(fragment, SETTINGS.clone(), &mut u);

panics on the Schema::from_raw_schemafrag(fragment, SETTINGS.clone(), &mut u) call with the following stack trace

thread 'main' panicked at /Users/emcx/cedar-spec/cedar-policy-generators/src/schema.rs:731:18:
called `Result::unwrap()` on an `Err` value: TypeNotDefined(TypeNotDefinedError(NonEmpty { head: ConditionalName { possibilities: NonEmpty { head: InternalName { id: Id("String"), path: [], loc: Some(Loc { span: SourceSpan { offset: SourceOffset(79), length: 6 }, src: "// Entities\n// All enums are modeled as just String here\nentity User {\n    id: String,\n    name: String,\n};" }) }, tail: [] }, reference_type: CommonOrEntity, raw: RawName(InternalName { id: Id("String"), path: [], loc: Some(Loc { span: SourceSpan { offset: SourceOffset(79), length: 6 }, src: "// Entities\n// All enums are modeled as just String here\nentity User {\n    id: String,\n    name: String,\n};" }) }) }, tail: [] }))
stack backtrace:
   0: rust_begin_unwind
             at /rustc/90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf/library/core/src/panicking.rs:74:14
   2: core::result::unwrap_failed
             at /rustc/90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf/library/core/src/result.rs:1700:5
   3: core::result::Result<T,E>::unwrap
             at /Users/emcx/.rustup/toolchains/1.82.0-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/result.rs:1104:23
   4: cedar_policy_generators::schema::Schema::from_raw_nsdef
             at /Users/emcx/cedar-spec/cedar-policy-generators/src/schema.rs:728:13
   5: cedar_policy_generators::schema::Schema::from_raw_schemafrag
             at /Users/emcx/cedar-spec/cedar-policy-generators/src/schema.rs:814:25
   6: avp_query_data_gen::main
             at ./src/main.rs:58:18
   7: core::ops::function::FnOnce::call_once
             at /Users/emcx/.rustup/toolchains/1.82.0-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5

Log output

// Put your output below this line

Additional configuration

No response

Operating System

No response

Additional information and screenshots

No response

[john-h-kastner-aws]

Thanks for the report. I can reproduce this locally.

My current theory is that this was introduced by cedar-policy/cedar#1060 and specifically relates to the new EntityOrCommon variant, but I don't yet know what the appropriate fix is.

We didn't notice this ourselves because, as far as I can tell, this function is only use by the CLI for cedar-policy-generators which we don't currently use for anything.

[shaobo-he-aws]

I need to dive into it further but a preliminary analysis reveals that fully_qualify_type_references is invalid given cedar-policy/cedar#1060, since it can't be done until (validator-schema-)fragment-combine time, IIUC. We should remove these functions.

[cdisselkoen]

Yes, there's a lot of logic at https://github.com/cedar-policy/cedar/blob/ed35ba7e2502c202a051e73e0bb8e4129244e73f/cedar-policy-validator/src/schema.rs#L382 that should probably be done in Schema::from_raw_schemafrag() but is currently not. I don't really want to copy-paste it in, it would be better to somehow encapsulate this logic into a reusable function that is called both there and in Schema::from_raw_schemafrag().

[cdisselkoen]

Arguably it's a design flaw that cedar-policy-generators works on json_schema::NamespaceDefinition under the hood, rather than ValidatorSchema. Changing that would probably be the best resolution to this issue. Note that that's different from #75, that is, I'm talking about cedar_policy_validator::ValidatorSchema and not cedar_policy::Schema.

[cdisselkoen]

Started implementing the change to use ValidatorSchema instead of json_schema::NamespaceDefinition under the hood in cedar-policy-generators. Although I made some progress, it's definitely a larger lift and probably not something we can prioritize completing in the very near future.

[shaobo-he-aws]

I propose we remove the CLI so that others won't run into the same issue.

[john-h-kastner-aws]

Removing CLI could also fix #259

[cdisselkoen]

Reopening this -- although #507 removes the CLI which was one place folks could run into this problem, I believe the OP here is running into this problem without using the CLI. We still need one of the solutions discussed farther above