Skip to content

chore: bring in spicedb and update compilation#635

Open
tstirrat15 wants to merge 4 commits intomainfrom
tstirrat/bring-in-new-schemadsl
Open

chore: bring in spicedb and update compilation#635
tstirrat15 wants to merge 4 commits intomainfrom
tstirrat/bring-in-new-schemadsl

Conversation

@tstirrat15
Copy link
Contributor

@tstirrat15 tstirrat15 commented Mar 2, 2026

Description

This PR is part of unifying the composable schema package into the tooling for a better developer experience. The key changes are:

  1. Dependency update — Updates SpiceDB dependency, bringing in a newer version of the schema compilation library.
  2. Decoder refactor (internal/decode/decoder.go) — Significant rewrite of the schema decoder, along with updated tests. This simplifies the decoding logic.
  3. Validate command simplification (internal/cmd/validate.go) — Major simplification of the validation logic, cutting roughly half the code. Tests were also reworked
  4. New tests — Added 8 new .zed and .yaml test files for edge cases like:
    - Import errors in composable schemas
    - Nonexistent imports
    - Nested composable schemas
    - External schemas with escaping/errors

Testing

Unit tests

@tstirrat15 tstirrat15 force-pushed the tstirrat/bring-in-new-schemadsl branch from 610e2cc to 1fb916b Compare March 11, 2026 19:42
@tstirrat15 tstirrat15 marked this pull request as draft March 11, 2026 19:42
@tstirrat15
Copy link
Contributor Author

tstirrat15 commented Mar 11, 2026

Current TODOs and notes:

  • Merge feat: update DevContext and LSP to support composable schemas spicedb#2965 and then go get that SHA to make this reference that code.
  • Validation tests are failing. My best guess is that one if not all of the failures have to do with the DirFS() that's being handed to the DevContext in the validate command - it needs to match the dir of the validation file, not the invocation dir.
  • Need to check whether validations against just schemas are pointing their warnings at the correct lines (this should be captured in a test, but should also be validated)
  • It needs a test of a schema that imports another schema and there's a validation warning/error in that other schema to see whether it points at the correct file and line within that file
  • This should be hand-tested against a few things as well - validation files in the example repo, some composable schema stuff, etc.

@miparnisari miparnisari force-pushed the tstirrat/bring-in-new-schemadsl branch from 6dc6f3f to 47ee676 Compare March 17, 2026 02:37
@miparnisari miparnisari marked this pull request as ready for review March 17, 2026 02:42
@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2026

Codecov Report

❌ Patch coverage is 86.02941% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.13%. Comparing base (082eee0) to head (32e38db).

Files with missing lines Patch % Lines
internal/decode/decoder.go 82.05% 10 Missing and 4 partials ⚠️
internal/cmd/validate.go 92.00% 3 Missing and 1 partial ⚠️
internal/cmd/import.go 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #635      +/-   ##
==========================================
+ Coverage   41.65%   42.13%   +0.48%     
==========================================
  Files          38       38              
  Lines        6127     6045      -82     
==========================================
- Hits         2552     2547       -5     
+ Misses       3319     3243      -76     
+ Partials      256      255       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@miparnisari miparnisari force-pushed the tstirrat/bring-in-new-schemadsl branch from 47ee676 to 32e38db Compare March 17, 2026 06:16
From pastebin:
zed validate https://pastebin.com/8qU45rVK

From a devtools instance:
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI devtools was deprecated

```
--fail-on-warn treat warnings as errors during validation
--force-color force color code output even in non-tty environments
--schema-type string force validation according to specific schema syntax ("", "composable", "standard")
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI big change: removing the flag

command: []string{"zed", "validate"},
expectFlagErrorCalled: true,
flagErrorContains: "requires at least 1 arg(s), only received 0",
expectUsageContains: "zed validate <validation_file_or_schema_file> [flags]",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know who would want to validate more than one schema file at once, but 🤷‍♀️

Schema: parsed.Schema.Schema,
Relationships: tuples,
})
}, development.WithSourceFS(filesystem), development.WithRootFileName(filepath.Base(filename)))
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this is what allows us to validate composable schemas


func outputDeveloperErrorsWithLineOffset(sb *strings.Builder, validateContents []byte, devErrors []*devinterface.DeveloperError, lineOffset int, sourceFS fs.FS) {
for _, devErr := range devErrors {
// If the error has a Path, read the contents from that file instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this is what lets zed know which file contained error so it can print the contents of the context of the error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants