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

Resolve types at fragment-combine time #1060

Merged
merged 47 commits into from
Aug 2, 2024
Merged

Conversation

cdisselkoen
Copy link
Contributor

@cdisselkoen cdisselkoen commented Jul 11, 2024

Description of changes

Large change to typename resolution during schema construction.

Previously, typenames were always implicitly qualified with the current namespace. This applied to both typenames being defined, and typenames being referenced. Unfortunately, this prevented users from writing an unqualified typename Foo to refer to a definition Foo that appears in the empty namespace: the appearance of an unqualified typename Foo was assumed to refer to NS::Foo instead (if the current namespace was NS), even if there was no Foo defined in NS, causing an error. This is #579.

Now, an unqualified typename reference Foo may resolve to either NS::Foo or empty-namespace Foo (assuming NS is the current namespace), depending on which of these are defined, with NS::Foo taking priority. Importantly, this means we cannot resolve the unqualified typename reference Foo until all fragments have been combined together, because either NS::Foo or empty-namespace Foo could be defined in a different fragment.

To support this, this PR introduces a new type ConditionalName representing a name that could resolve to any of a list of candidate fully-qualified names, in a priority order, depending on which of them exist at fragment-combine time. Full typename resolution is deferred until fragment-combine time, at which point the ConditionalNames are all resolved into fully-qualified Names.

Relatedly, human-syntax processing code currently (before this PR) has its own typename-resolution algorithm, for determining when an unqualified typename refers to an entity type, a common type, a primitive type, or an extension type. For entity and common types, it only considers types defined in the current namespace, not the empty namespace, so this equally suffers from #579. However, now (in this PR) this processing also cannot be done until all fragments have been combined together, because either entity or common types in either the current or empty namespace could be defined in other fragments, and the presence of those definitions could change how the typename resolves.

This PR removes the typename-resolution code that previously applied only to the human-syntax processing. Instead, both human-syntax and JSON-syntax schemas rely on the same typename resolution algorithm, which runs at fragment-combine time. This increases consistency between the two formats and hopefully avoids a class of bugs that could appear in the future if resolution were handled differently in the two formats.

Another consequence of unifying the typename-resolution algorithms used by the human syntax and JSON syntax is that __cedar::String, __cedar::ipaddr, etc become valid aliases for String, ipaddr, etc (respectively) in the JSON syntax. (Previously, these were only valid aliases in the human syntax; they are not needed for expressiveness in the JSON syntax.). Consequently, this PR also lifts the restriction on defining common types named Long, String, etc in the JSON syntax (before this PR, those were errors). (so now, after this PR, the __cedar:: aliases may actually be needed in the JSON syntax for any JSON schema that uses the newfound capability to define a common type like Long or String.) I see no problem with any of the changes described in this paragraph, and they're all backward-compatible, but I'm calling these changes out for awareness and in case reviewers have other opinions.

Another change in this PR worth calling out, is that in the new typename resolution code, the __cedar namespace is significantly less special: at fragment-combine time, we simply add a completely normal schema fragment containing the definitions in the __cedar namespace. We also add aliases (completely normal common-type definitions) to the empty namespace that map Bool to __cedar::Bool, etc, but not overriding any user definition of Bool etc in the empty namespace (in that case, they will need to use the __cedar prefix to reference the primitive type Bool). This seems like a much better organization to me, with typename resolution not needing to special-case primitive/extension types or the __cedar namespace nearly as much.

This PR includes a large number of tests, outlined in the comments on the new test_579 module (and inspired by the discussion on #579). As of this writing, the PR contains only tests in groups A, B, and C. I believe the current code will fail tests in some of the other categories -- for instance, referring to an empty-namespace entity type in an entity parent list.

I recommend reviewing the files in this PR in a different order than GitHub displays them: start with the changes to raw_name.rs (the definitions of RawName and ConditionalName) and schema.rs (which contains the code for combining fragments into a ValidatorSchema). Then the other changes may make more sense in context.

Issue #, if available

Fixes #579 and fixes #711

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A bug fix or other functionality change requiring a patch to cedar-policy.

I confirm that this PR (choose one, and delete the other options):

  • Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).

I confirm that cedar-spec (choose one, and delete the other options):

  • Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-spec, and how you have tested that your updates are correct.)

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Copy link
Contributor

@khieta khieta left a comment

Choose a reason for hiding this comment

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

Partial review for now since I'll be OOTO tomorrow

cedar-policy-validator/src/schema/raw_name.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/schema.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/schema.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/schema.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/schema.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/schema.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/ast/name.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/err.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/err.rs Outdated Show resolved Hide resolved
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
cedar-policy-validator/src/err.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/types.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/human_schema/to_json_schema.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/human_schema/to_json_schema.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/schema.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/schema/namespace_def.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/schema/namespace_def.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/schema_file_format.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/schema_file_format.rs Outdated Show resolved Hide resolved
cedar-policy-validator/src/schema/test_579.rs Outdated Show resolved Hide resolved
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

Co-authored-by: John Kastner <130772734+john-h-kastner-aws@users.noreply.github.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
@cdisselkoen
Copy link
Contributor Author

Just pushed a major overhaul involving merging in main. This was nontrivial because of #969. As discussed with maintainers, the proposal in this PR is to use InternalName (renamed from UncheckedName) in ValidatorSchema in order to allow __cedar names to be used internally.

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
cedar-policy/CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
@cdisselkoen cdisselkoen merged commit 67bd388 into main Aug 2, 2024
19 of 20 checks passed
@cdisselkoen cdisselkoen deleted the cdisselkoen/fix-579 branch August 2, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants