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

Port expiration compiler changes into composableschemadsl #2240

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

tstirrat15
Copy link
Contributor

Description

Part of getting expiration support into composable schema.

Changes

Testing

Review. See that things are green.

@tstirrat15 tstirrat15 requested a review from a team as a code owner February 10, 2025 15:58
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Feb 10, 2025
@tstirrat15 tstirrat15 force-pushed the port-expiration-compiler-changes branch 2 times, most recently from 96f054c to 74cb714 Compare February 10, 2025 17:17
Copy link
Contributor Author

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

Add a comment

Comment on lines +364 to +378
{
"expiration trait",
`use expiration

definition document{
relation viewer: user with expiration
relation editor: user with somecaveat and expiration
}`,
`use expiration

definition document {
relation viewer: user with expiration
relation editor: user with somecaveat and expiration
}`,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this does not port in the expiration caveat tests, as we treat expiration as a reserved keyword in composable schema and treat it only as a trait.

@tstirrat15 tstirrat15 force-pushed the port-expiration-compiler-changes branch 3 times, most recently from c192152 to 22e351c Compare February 10, 2025 19:54
@github-actions github-actions bot added the area/schema Affects the Schema Language label Feb 10, 2025
@tstirrat15 tstirrat15 force-pushed the port-expiration-compiler-changes branch from 22e351c to de11d99 Compare February 10, 2025 19:58
Copy link
Contributor Author

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

See comments

@@ -111,11 +129,12 @@ func Compile(schema InputSchema, prefix ObjectPrefixOption, opts ...Option) (*Co
return nil, err
}

compiled, err := translate(translationContext{
compiled, err := translate(&translationContext{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making this a reference allows us to mutate the enabledFlags field, which allows us to detect duplicate use directives.

Comment on lines +1226 to +1238
{
"duplicate use pragmas",
withTenantPrefix,
`
use expiration
use expiration

definition simple {
relation viewer: user with expiration
}`,
`found duplicate use flag`,
[]SchemaDefinition{},
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a net-new test. We also excluded the relation with expiration caveat test from this set, since we want to reserve expiration under the new composable schema syntax.

Comment on lines +77 to +82
case dslshape.NodeTypeUseFlag:
err := translateUseFlag(tctx, topLevelNode)
if err != nil {
return nil, err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Translating the use flag is basically only for detecting duplicate flags. All of the handling/usage is down at the lexer and parser level.

Comment on lines +713 to +715
if !slices.Contains(tctx.enabledFlags, "expiration") {
return nil, typeRefNode.Errorf("expiration flag is not enabled; add `use expiration` to top of file")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also net new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a direct port.

// NOTE: a duplicate use statement should be an error at the level
// of the compiler, but should not be an error at the level
// of the parser.
{"duplicate use statement test", "duplicate_use_statement"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a parsing test to verify that multiple use statements get parsed correctly. This was mostly a debugging tool in doing this work, but also describes desired behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is asserting that duplicate use statements pass parsing. The error states happen in the compiler.

Comment on lines +17 to +21
end-rune = 37
error-message = `use` expressions must be declared before any definition
input-source = use after definition
start-rune = 38
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding more context to this parsing error.

Is it an issue that the start rune is after the end rune?

@@ -107,7 +107,7 @@ func Compile(schema InputSchema, prefix ObjectPrefixOption, opts ...Option) (*Co
return nil, err
}

compiled, err := translate(translationContext{
compiled, err := translate(&translationContext{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making the same changes to schemadsl to support erroring on duplicate use expressions.

@@ -1,28 +1,28 @@
package lexer

// FlaggableLexler wraps a lexer, automatically translating tokens based on flags, if any.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same typo changes

@tstirrat15 tstirrat15 force-pushed the port-expiration-compiler-changes branch from de11d99 to a708397 Compare February 10, 2025 23:51
Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

@tstirrat15 tstirrat15 added this pull request to the merge queue Feb 11, 2025
Merged via the queue into main with commit 344e26c Feb 11, 2025
39 checks passed
@tstirrat15 tstirrat15 deleted the port-expiration-compiler-changes branch February 11, 2025 17:45
@github-actions github-actions bot locked and limited conversation to collaborators Feb 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/schema Affects the Schema Language area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants