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

Implement partials compilation #2212

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Conversation

tstirrat15
Copy link
Contributor

@tstirrat15 tstirrat15 commented Jan 28, 2025

Description

This implements partials support in the composable schema language.

Changes

Gonna annotate.

Testing

Review. See that tests pass. Suggest any missing tests.

@github-actions github-actions bot added area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) area/dependencies Affects dependencies labels Jan 28, 2025
@tstirrat15 tstirrat15 force-pushed the implement-partials-compilation branch 5 times, most recently from 0cec82e to 9e66052 Compare January 30, 2025 23:04
@tstirrat15 tstirrat15 marked this pull request as ready for review January 30, 2025 23:04
@tstirrat15 tstirrat15 requested a review from a team as a code owner January 30, 2025 23:04
@github-actions github-actions bot removed the area/dependencies Affects dependencies label Jan 30, 2025
@tstirrat15 tstirrat15 force-pushed the implement-partials-compilation branch 3 times, most recently from c3d13b3 to 9e735d8 Compare January 30, 2025 23:20
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 annotations

go.mod Outdated
@@ -1,6 +1,6 @@
module github.com/authzed/spicedb

go 1.23.1
go 1.23.5
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 may not actually be necessary; I changed it when I thought it was a fix for vulnerability issues. I can revert if desired.

Comment on lines -89 to -101
type compilationContext struct {
// The set of definition names that we've seen as we compile.
// If these collide we throw an error.
existingNames *mapz.Set[string]
// The global set of files we've visited in the import process.
// If these collide we short circuit, preventing duplicate imports.
globallyVisitedFiles *mapz.Set[string]
// The set of files that we've visited on a particular leg of the recursion.
// This allows for detection of circular imports.
// NOTE: This depends on an assumption that a depth-first search will always
// find a cycle, even if we're otherwise marking globally visited nodes.
locallyVisitedFiles *mapz.Set[string]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were previously values that we passed through the tree, because the import logic invoked compilation. That isn't possible with partials since a partial can be defined in one file and used in another, so among other changes, this PR makes the import logic operate on the DSL tree as an intermediate step.

return compileImpl(schema, cctx, prefix, opts...)
}

func compileImpl(schema InputSchema, cctx compilationContext, prefix ObjectPrefixOption, opts ...Option) (*CompiledSchema, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flattening these since there's no longer an invocation in the import logic

skipValidate: cfg.skipValidation,
// NOTE: import translation is done separately so that partial references
// and definitions defined in separate files can correctly resolve.
err = translateImports(importTranslationContext{
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 the import process doesn't produce a new tree - it operates on the tree started in the root file and recursively and mutatively flattens things on. I'm not entirely sure I like the approach; I'd welcome other suggestions.

Comment on lines +135 to +144
func parseSchema(schema InputSchema) (*dslNode, input.PositionMapper, error) {
mapper := newPositionMapper(schema)
root := parser.Parse(createAstNode, schema.Source, schema.SchemaString).(*dslNode)
errs := root.FindAll(dslshape.NodeTypeError)
if len(errs) > 0 {
err := errorNodeToError(errs[0], mapper)
return nil, nil, err
}
return root, mapper, nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled this out into a separate function because the import functionality parses as a part of its work.

Comment on lines 29 to 30
compiledPartials map[string][]*core.Relation
partialWaitingRooms *mapz.MultiMap[string, *dslNode]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are for the partial compilation process. partialWaitingRooms could probably move into the translatePartials function if desired.

Copy link
Member

Choose a reason for hiding this comment

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

why is it called a waiting room?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a name i reached for. The idea is that the values of a given key are the partials whose compilation depends on the compilation of the partial named in the key, and my brain went to "waiting room" for that concept. I can change that to "unresolvedPartials" or something if that's preferable.

Comment on lines -96 to -99
case dslshape.NodeTypeImport:
compiled, err := translateImport(tctx, topLevelNode, names)
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.

This logic moved into translateImports

// NOTE: this function and the translatePartial variation differ in how they treat a missing reference.
// This function treats that as an error state, since all partials should be resolved by that point;
// the partial variant treats that as a skip.
func translateDefinitionRelationsAndPermissions(tctx translationContext, astNode *dslNode) ([]*core.Relation, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I imagine there's some deduplication to be done between this and the function below, but the point at which they differ is stuck in the middle. I'd be fine leaving this as a target for future refactoring.

Copy link
Member

Choose a reason for hiding this comment

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

pass a flag and change behavior on that bool


// Takes a parsed schema and recursively translates import syntax and replaces
// import nodes with parsed nodes from the target files
func translateImports(itctx importTranslationContext, root *dslNode) error {
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 now where the import logic is happening.

Comment on lines 842 to 823
if tctx.partialWaitingRooms.Len() != 0 {
return fmt.Errorf("could not resolve partials: [%s]. this may indicate a circular reference", strings.Join(tctx.partialWaitingRooms.Keys(), ", "))
}
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 engenders an assumption that the algorithm using this approach is correct. So far it looks correct, but I don't know that I could prove it right now.

withTenantPrefix,
`
definition simple {
...some_partial
Copy link
Member

Choose a reason for hiding this comment

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

add some tests that have concrete impls next to the partials

@@ -35,6 +35,15 @@ func (tn *dslNode) Connect(predicate string, other parser.AstNode) {
tn.children[predicate].PushBack(other)
}

// Used to preserve import order when doing import operations on AST
func (tn *dslNode) ConnectAndHoistMany(predicate string, other *list.List) {
Copy link
Member

Choose a reason for hiding this comment

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

does the order really matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't, but it means that tests pass without regeneration (which makes me feel better about not having missed something) and that the generated output matches my mental model of imports (in terms of imports being replaced by the contents of the referenced files).

Copy link
Member

Choose a reason for hiding this comment

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

Okay. Maybe name "ConnectAtFront"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be more keen to unwind it in a later refactor if we want to drop it as an optimization

Copy link
Member

Choose a reason for hiding this comment

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

ok

Comment on lines 29 to 30
compiledPartials map[string][]*core.Relation
partialWaitingRooms *mapz.MultiMap[string, *dslNode]
Copy link
Member

Choose a reason for hiding this comment

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

why is it called a waiting room?

// Do an initial pass to translate partials and add them to the
// translation context. This ensures that they're available for
// subsequent reference in definition compilation.
err := translatePartials(tctx, root)
Copy link
Member

Choose a reason for hiding this comment

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

call this collectPartials, since it isn't actually translating them into the root of the tree

// NOTE: this function and the translatePartial variation differ in how they treat a missing reference.
// This function treats that as an error state, since all partials should be resolved by that point;
// the partial variant treats that as a skip.
func translateDefinitionRelationsAndPermissions(tctx translationContext, astNode *dslNode) ([]*core.Relation, error) {
Copy link
Member

Choose a reason for hiding this comment

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

pass a flag and change behavior on that bool

path, err := importNode.GetString(dslshape.NodeImportPredicatePath)
if err != nil {
return nil, err
type importTranslationContext struct {
Copy link
Member

Choose a reason for hiding this comment

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

maybe importResolutionContext

@tstirrat15 tstirrat15 force-pushed the implement-partials-compilation branch from 9e735d8 to 88ff405 Compare January 31, 2025 20:50
@tstirrat15 tstirrat15 force-pushed the implement-partials-compilation branch from 88ff405 to 5b6840f Compare January 31, 2025 20: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

@@ -35,6 +35,15 @@ func (tn *dslNode) Connect(predicate string, other parser.AstNode) {
tn.children[predicate].PushBack(other)
}

// Used to preserve import order when doing import operations on AST
func (tn *dslNode) ConnectAndHoistMany(predicate string, other *list.List) {
Copy link
Member

Choose a reason for hiding this comment

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

ok

@tstirrat15 tstirrat15 added this pull request to the merge queue Jan 31, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 31, 2025
@tstirrat15 tstirrat15 added this pull request to the merge queue Jan 31, 2025
Merged via the queue into main with commit f5c4424 Jan 31, 2025
39 checks passed
@tstirrat15 tstirrat15 deleted the implement-partials-compilation branch January 31, 2025 22:18
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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