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

import: improve err msg #2242

Merged
merged 2 commits into from
Feb 21, 2025
Merged

Conversation

kartikaysaxena
Copy link
Contributor

Fixes authzed/zed#463, error now explicitly reports that there is something wrong with the import

@kartikaysaxena kartikaysaxena requested a review from a team as a code owner February 14, 2025 14:10
@@ -19,7 +19,7 @@ type CircularImportError struct {
func importFile(filePath string) (*dslNode, error) {
schemaBytes, err := os.ReadFile(filePath)
if err != nil {
return nil, fmt.Errorf("failed to read schema file: %w", err)
return nil, fmt.Errorf("failed to read import in schema file: %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

This is good, but we should also decorate it with the source location information from the source file(s) in which the import was used. It would require tracking them in the import context, but it should be doable.

Something like: failed to read import in schema file: %w; referenced at [...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, tested with zed, it returns terminated with errors error="parse error in simple.zed, line 2, column 1: failed to read import in schema file" on a faulty import

@tstirrat15
Copy link
Contributor

I think the idea of this was that it'd include the information about where in the source it happened:

Ideally this would say something about the import statement on line 1 of root.zed, etc.

I think it'll require turning this error: https://github.com/authzed/spicedb/blob/main/pkg/composableschemadsl/compiler/importer.go#L21-L23

into a WithSourceError like this one: https://github.com/authzed/spicedb/blob/main/pkg/composableschemadsl/compiler/translator.go#L94-L96

and then pulling that change into zed.

@kartikaysaxena
Copy link
Contributor Author

Now zed preview schema compile gives the following error on a faulty import

1:33AM ERR terminated with errors error="parse error in `simple.zed`, line 2, column 1: failed to read import in schema file"

does this look good?

Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>
Signed-off-by: Kartikay <kartikay_2101ce32@iitp.ac.in>
@tstirrat15 tstirrat15 force-pushed the import/improve-err-msg branch from bcdbe54 to 1777150 Compare February 20, 2025 22:00
Copy link
Contributor

@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.

This looks good to me now.

@tstirrat15 tstirrat15 added this pull request to the merge queue Feb 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 21, 2025
@josephschorr josephschorr added this pull request to the merge queue Feb 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 21, 2025
@tstirrat15 tstirrat15 added this pull request to the merge queue Feb 21, 2025
Merged via the queue into authzed:main with commit 26bf35e Feb 21, 2025
39 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error messaging in zed preview schema compile
3 participants