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

Update compiler to consume local imports #2116

Merged
merged 1 commit into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/lint.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
name: "Lint"
on: # yamllint disable-line rule:truthy
on: # yamllint disable-line rule:truthy
Copy link
Member

Choose a reason for hiding this comment

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

Why this change and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Blech

push:
branches:
- "!dependabot/*"
Expand All @@ -20,7 +20,7 @@ jobs:
- name: "Check Licenses"
uses: "authzed/actions/go-license-check@main"
with:
ignore: "buf.build" # Has no license information
ignore: "buf.build" # Has no license information

go-lint:
name: "Lint Go"
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/security.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
name: "Security"
on: # yamllint disable-line rule:truthy
on: # yamllint disable-line rule:truthy
push:
branches:
- "!dependabot/*"
Expand All @@ -16,7 +16,7 @@ env:
jobs:
codeql:
name: "CodeQL Analyze"
if: "${{ github.event_name == 'pull_request' }}" # workaround to https://github.com/github/codeql-action/issues/1537
if: "${{ github.event_name == 'pull_request' }}" # workaround to https://github.com/github/codeql-action/issues/1537
runs-on: "buildjet-8vcpu-ubuntu-2204"
timeout-minutes: "${{ (matrix.language == 'swift' && 120) || 360 }}"
permissions:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
---
schema: >-
definition user {}

Expand Down
21 changes: 21 additions & 0 deletions pkg/composableschemadsl/compiler/compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/authzed/spicedb/pkg/composableschemadsl/dslshape"
"github.com/authzed/spicedb/pkg/composableschemadsl/input"
"github.com/authzed/spicedb/pkg/composableschemadsl/parser"
"github.com/authzed/spicedb/pkg/genutil/mapz"
core "github.com/authzed/spicedb/pkg/proto/core/v1"
)

Expand Down Expand Up @@ -52,28 +53,46 @@ func (cs CompiledSchema) SourcePositionToRunePosition(source input.Source, posit
type config struct {
skipValidation bool
objectTypePrefix *string
// In an import context, this is the folder containing
// the importing schema (as opposed to imported schemas)
sourceFolder 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.

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.

Copy link
Member

Choose a reason for hiding this comment

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

Why can't you put it on the translationContext?

}

func SkipValidation() Option { return func(cfg *config) { cfg.skipValidation = true } }

// Config for the prefix attached to all definitions, such as in Serverless
// where it's `someorganization/` in front of each definition.
func ObjectTypePrefix(prefix string) ObjectPrefixOption {
return func(cfg *config) { cfg.objectTypePrefix = &prefix }
}

// Config that does not supply the prefix but requires the prefix on all objects.
func RequirePrefixedObjectType() ObjectPrefixOption {
return func(cfg *config) { cfg.objectTypePrefix = nil }
}

// Config that allows for no prefix. This is the "normal" default.
func AllowUnprefixedObjectType() ObjectPrefixOption {
return func(cfg *config) { cfg.objectTypePrefix = new(string) }
}

// Config that supplies the root source folder for compilation. Required
// for relative import syntax to work properly.
func SourceFolder(sourceFolder string) Option {
Copy link
Member

Choose a reason for hiding this comment

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

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

return func(cfg *config) { cfg.sourceFolder = sourceFolder }
}

type Option func(*config)

type ObjectPrefixOption func(*config)

// Compile compilers the input schema into a set of namespace definition protos.
func Compile(schema InputSchema, prefix ObjectPrefixOption, opts ...Option) (*CompiledSchema, error) {
names := mapz.NewSet[string]()
return compileImpl(schema, names, prefix, opts...)
}

func compileImpl(schema InputSchema, existingNames *mapz.Set[string], prefix ObjectPrefixOption, opts ...Option) (*CompiledSchema, error) {
cfg := &config{}
prefix(cfg) // required option

Expand All @@ -94,6 +113,8 @@ func Compile(schema InputSchema, prefix ObjectPrefixOption, opts ...Option) (*Co
mapper: mapper,
schemaString: schema.SchemaString,
skipValidate: cfg.skipValidation,
sourceFolder: cfg.sourceFolder,
existingNames: existingNames,
}, root)
if err != nil {
var errorWithNode errorWithNode
Expand Down
5 changes: 5 additions & 0 deletions pkg/composableschemadsl/compiler/importer-test/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Test Structure

Every folder should have a `root.zed`. This is the target for compilation.

Every folder will have an `expected.zed`, which is the output of the compilation process.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
definition user {}

definition resource {
relation viewer: user
permission view = viewer
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
from .transitive.transitive import user

definition resource {
relation viewer: user
permission view = viewer
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from .user.user import user
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
definition user {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
definition user {}

definition resource {
relation viewer: user
permission view = viewer
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
from .user.user import user

definition resource {
relation viewer: user
permission view = viewer
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
definition user {}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
definition user {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
definition user {}

definition resource {
relation viewer: user
permission view = viewer
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
from .definitions.user.user import user

definition resource {
relation viewer: user
permission view = viewer
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
definition user {}

definition resource {
relation viewer: user
permission view = viewer
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from .user import user
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
from .indirection import user

definition resource {
relation viewer: user
permission view = viewer
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
definition user {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
definition user {}

definition resource {
relation viewer: user
permission view = viewer
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
from .user import user

definition resource {
relation viewer: user
permission view = viewer
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
definition user {}
52 changes: 52 additions & 0 deletions pkg/composableschemadsl/compiler/importer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package compiler

import (
"fmt"
"os"
"path"
"path/filepath"

"github.com/rs/zerolog/log"

"github.com/authzed/spicedb/pkg/composableschemadsl/input"
"github.com/authzed/spicedb/pkg/genutil/mapz"
)

type importContext struct {
pathSegments []string
sourceFolder string
names *mapz.Set[string]
}

const SchemaFileSuffix = ".zed"

func importFile(importContext importContext) (*CompiledSchema, error) {
relativeFilepath := constructFilePath(importContext.pathSegments)
filePath := path.Join(importContext.sourceFolder, relativeFilepath)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


newSourceFolder := filepath.Dir(filePath)

var schemaBytes []byte
schemaBytes, err := os.ReadFile(filePath)
if err != nil {
return nil, fmt.Errorf("failed to read schema file: %w", err)
}
log.Trace().Str("schema", string(schemaBytes)).Str("file", filePath).Msg("read schema from file")
Copy link
Member

Choose a reason for hiding this comment

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

yes; trace is good


compiled, err := compileImpl(InputSchema{
Source: input.Source(filePath),
SchemaString: string(schemaBytes),
},
importContext.names,
AllowUnprefixedObjectType(),
SourceFolder(newSourceFolder),
)
if err != nil {
return nil, err
}
return compiled, nil
}

func constructFilePath(segments []string) string {
return path.Join(segments...) + SchemaFileSuffix
}
91 changes: 91 additions & 0 deletions pkg/composableschemadsl/compiler/importer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package compiler_test

import (
"fmt"
"os"
"path"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/authzed/spicedb/pkg/composableschemadsl/compiler"
"github.com/authzed/spicedb/pkg/composableschemadsl/generator"
"github.com/authzed/spicedb/pkg/composableschemadsl/input"
)

type importerTest struct {
name string
folder string
}

func (it *importerTest) input() string {
b, err := os.ReadFile(fmt.Sprintf("importer-test/%s/root.zed", it.folder))
if err != nil {
panic(err)
}

return string(b)
}

func (it *importerTest) relativePath() string {
return fmt.Sprintf("importer-test/%s", it.folder)
}

func (it *importerTest) expected() string {
b, err := os.ReadFile(fmt.Sprintf("importer-test/%s/expected.zed", it.folder))
if err != nil {
panic(err)
}

return string(b)
}

func (it *importerTest) writeExpected(schema string) {
err := os.WriteFile(fmt.Sprintf("importer-test/%s/expected.zed", it.folder), []byte(schema), 0o_600)
if err != nil {
panic(err)
}
}

func TestImporter(t *testing.T) {
workingDir, err := os.Getwd()
require.NoError(t, err)

importerTests := []importerTest{
{"simple local import", "simple-local"},
{"simple local import with transitive hop", "simple-local-with-hop"},
{"nested local import", "nested-local"},
{"nested local import with transitive hop", "nested-local-with-hop"},
{"nested local two layers deep import", "nested-two-layer-local"},
}

for _, test := range importerTests {
t.Run(test.name, func(t *testing.T) {
t.Parallel()

sourceFolder := path.Join(workingDir, test.relativePath())

inputSchema := test.input()

compiled, err := compiler.Compile(compiler.InputSchema{
Source: input.Source("schema"),
SchemaString: inputSchema,
}, compiler.AllowUnprefixedObjectType(),
compiler.SourceFolder(sourceFolder))
require.NoError(t, err)

generated, _, err := generator.GenerateSchema(compiled.OrderedDefinitions)
require.NoError(t, err)

if os.Getenv("REGEN") == "true" {
test.writeExpected(generated)
} else {
expected := test.expected()
if !assert.Equal(t, expected, generated, test.name) {
t.Log(generated)
}
}
})
}
}
Loading
Loading