Update compiler to consume local imports #2116

tstirrat15 · 2024-11-04T19:57:26Z

Description

Part of getting composable schemas implemented. This actually implements the traversal.

Notes

This is intentionally incremental, and is not intended to be the release candidate. Future work includes:

Import cycle tracking
Only including requested imports (rather than all imports)

Changes

Plumb an existingNames value through the compiler so that it can track the definitions it's already seen
Add file importer logic
Implement import logic the translator

Testing

Review. See that tests go green.

tstirrat15

A couple of thoughts

tstirrat15 · 2024-11-05T17:45:08Z

pkg/composableschemadsl/compiler/compiler.go

+	existingNames    []string
+	// In an import context, this is the folder containing
+	// the importing schema (as opposed to imported schemas)
+	sourceFolder string


I don't really like how I need to plumb this through three levels in order to get it to the next compilation context. I'd welcome recommendations on a different way to structure this.

Why can't you put it on the translationContext?

tstirrat15 · 2024-11-05T17:46:14Z

pkg/composableschemadsl/compiler/translator.go

@@ -22,6 +23,8 @@ type translationContext struct {
 	mapper           input.PositionMapper
 	schemaString     string
 	skipValidate     bool
+	existingNames    []string


Should this be a *mapz.Set up and down the stack?

josephschorr · 2024-11-05T18:31:08Z

pkg/composableschemadsl/compiler/compiler.go

+	existingNames    []string
+	// In an import context, this is the folder containing
+	// the importing schema (as opposed to imported schemas)
+	sourceFolder string


Why can't you put it on the translationContext?

josephschorr · 2024-11-05T18:32:18Z

pkg/composableschemadsl/compiler/importer.go

+	// TODO: do we want this sort of logging here?
+	log.Trace().Str("schema", string(schemaBytes)).Str("file", filepath).Msg("read schema from file")
+
+	compiled, err := Compile(InputSchema{


If you call an internal compile, then this could be passed the translationContext directly and then you wouldn't need a different context here or the need to pass it into Compile

vroldanbet · 2024-11-06T10:30:44Z

pkg/composableschemadsl/compiler/importer.go

+
+func ImportFile(importContext *ImportContext) (*CompiledSchema, error) {
+	relativeFilepath := constructFilePath(importContext.pathSegments)
+	filePath := path.Join(importContext.sourceFolder, relativeFilepath)


what happens if the imports are outside of the root context folder? is that supported, and if not, what kind of opinions/guidelines does it impose on the structure of a composable schema?

No; the eventual goal will be to support this using git-based imports

The biggest effect that I see is that you won't be able to do a relative import within the package that walks up a directory. this means that you couldn't have:

app1/app1.zed app2/app2.zed util.zed

and have both app files reference the util file. I have a feeling this will be a tripping point; it'll either have to be well-documented or something we change.

josephschorr · 2024-11-06T15:22:17Z

.github/workflows/lint.yaml

@@ -1,6 +1,6 @@
 ---
 name: "Lint"
-on: # yamllint disable-line rule:truthy
+on:  # yamllint disable-line rule:truthy


Why this change and below?

yamllint wants two spaces before an inline comment and appeared to be failing the lint check as a result.

josephschorr · 2024-11-06T15:22:57Z

pkg/composableschemadsl/compiler/importer.go

+
+const SchemaFileSuffix = ".zed"
+
+func ImportFile(importContext *ImportContext) (*CompiledSchema, error) {


Unless you see a need to export this, should we just keep this internal (i.e. lowercase)?

That works for me

josephschorr · 2024-11-06T16:33:18Z

pkg/composableschemadsl/compiler/compiler.go

@@ -68,12 +72,21 @@ func AllowUnprefixedObjectType() ObjectPrefixOption {
 	return func(cfg *config) { cfg.objectTypePrefix = new(string) }
 }

+func SourceFolder(sourceFolder string) Option {


Add a doc comment explaining what this does. Probably good to add doc comments to the other exported functions too

josephschorr · 2024-11-06T16:33:52Z

pkg/composableschemadsl/compiler/importer.go

+	"github.com/authzed/spicedb/pkg/genutil/mapz"
+)
+
+type ImportContext struct {


make this context unexported as well

josephschorr · 2024-11-06T16:34:06Z

pkg/composableschemadsl/compiler/importer.go

+
+const SchemaFileSuffix = ".zed"
+
+func importFile(importContext *ImportContext) (*CompiledSchema, error) {


why does this take in a * vs just the importContext struct itself?

Mostly me not having built an intuition yet for when you would use one vs the other.

josephschorr · 2024-11-06T16:34:16Z

pkg/composableschemadsl/compiler/importer.go

+		return nil, fmt.Errorf("failed to read schema file: %w", err)
+	}
+	// TODO: do we want this sort of logging here?
+	log.Trace().Str("schema", string(schemaBytes)).Str("file", filePath).Msg("read schema from file")


yes; trace is good

josephschorr · 2024-11-06T16:34:26Z

pkg/composableschemadsl/compiler/importer.go

+	log.Trace().Str("schema", string(schemaBytes)).Str("file", filePath).Msg("read schema from file")
+
+	compiled, err := compileImpl(InputSchema{
+		// TODO: should this point to the schema file? What is this for?


yes; it gives the file path for error messaging

josephschorr

LGTM

tstirrat15 force-pushed the update-compiler-to-consume-imports branch from ae9d114 to 8946248 Compare November 4, 2024 19:57

github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Nov 4, 2024

tstirrat15 commented Nov 5, 2024

View reviewed changes

tstirrat15 force-pushed the update-compiler-to-consume-imports branch from 0653113 to 775de22 Compare November 5, 2024 17:51

tstirrat15 marked this pull request as ready for review November 5, 2024 17:51

tstirrat15 requested a review from a team as a code owner November 5, 2024 17:51

josephschorr reviewed Nov 5, 2024

View reviewed changes

tstirrat15 force-pushed the update-compiler-to-consume-imports branch from 35995e0 to ccf4d79 Compare November 5, 2024 20:49

tstirrat15 mentioned this pull request Nov 5, 2024

Implement definition requests #2121

Open

vroldanbet reviewed Nov 6, 2024

View reviewed changes

josephschorr reviewed Nov 6, 2024

View reviewed changes

tstirrat15 force-pushed the update-compiler-to-consume-imports branch from 85172fc to 66f9d26 Compare November 6, 2024 15:35

josephschorr reviewed Nov 6, 2024

View reviewed changes

tstirrat15 force-pushed the update-compiler-to-consume-imports branch from 80aa245 to 6cba14f Compare November 6, 2024 17:53

josephschorr approved these changes Nov 6, 2024

View reviewed changes

Implement naive resolution of local imports

f8ceb8e

tstirrat15 force-pushed the update-compiler-to-consume-imports branch from 6cba14f to f8ceb8e Compare November 6, 2024 18:03

tstirrat15 enabled auto-merge November 6, 2024 18:24

tstirrat15 added this pull request to the merge queue Nov 6, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 6, 2024

tstirrat15 added this pull request to the merge queue Nov 6, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 6, 2024

tstirrat15 added this pull request to the merge queue Nov 6, 2024

Merged via the queue into main with commit 20f450c Nov 6, 2024
22 checks passed

tstirrat15 deleted the update-compiler-to-consume-imports branch November 6, 2024 19:00

github-actions bot locked and limited conversation to collaborators Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update compiler to consume local imports #2116

Update compiler to consume local imports #2116

tstirrat15 commented Nov 4, 2024

tstirrat15 left a comment

tstirrat15 Nov 5, 2024

josephschorr Nov 5, 2024

tstirrat15 Nov 5, 2024

josephschorr Nov 5, 2024

josephschorr Nov 5, 2024

josephschorr Nov 5, 2024

vroldanbet Nov 6, 2024

josephschorr Nov 6, 2024

tstirrat15 Nov 6, 2024

josephschorr Nov 6, 2024

tstirrat15 Nov 6, 2024

josephschorr Nov 6, 2024

josephschorr Nov 6, 2024

tstirrat15 Nov 6, 2024

josephschorr Nov 6, 2024

josephschorr Nov 6, 2024

josephschorr Nov 6, 2024

tstirrat15 Nov 6, 2024

josephschorr Nov 6, 2024

josephschorr Nov 6, 2024

josephschorr left a comment


		const SchemaFileSuffix = ".zed"

		func ImportFile(importContext ImportContext) (CompiledSchema, error) {

Update compiler to consume local imports #2116

Update compiler to consume local imports #2116

Conversation

tstirrat15 commented Nov 4, 2024

Description

Notes

Changes

Testing

tstirrat15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr left a comment

Choose a reason for hiding this comment