From 1b6449ef895231ec97c5a2d31fd2ea7b2b154d96 Mon Sep 17 00:00:00 2001 From: Tanner Stirrat Date: Mon, 23 Mar 2026 10:48:25 -0600 Subject: [PATCH] chore: hang filename off of ast nodes and refactor accordingly --- internal/services/shared/errors_test.go | 1 + pkg/development/devcontext.go | 6 +-- pkg/development/schema_position_mapper.go | 17 ++----- pkg/proto/core/v1/core.pb.go | 13 ++++- pkg/proto/core/v1/core_vtproto.pb.go | 47 +++++++++++++++++++ pkg/schema/errors.go | 4 +- pkg/schemadsl/compiler/development.go | 35 -------------- pkg/schemadsl/compiler/translator.go | 10 ++++ pkg/spiceerrors/common.go | 7 ++- pkg/validationfile/blocks/assertions.go | 6 +++ pkg/validationfile/blocks/errors.go | 2 + .../blocks/expectedrelations.go | 14 ++++-- pkg/validationfile/blocks/relationships.go | 4 ++ pkg/validationfile/loader.go | 1 + proto/internal/core/v1/core.proto | 1 + 15 files changed, 108 insertions(+), 60 deletions(-) diff --git a/internal/services/shared/errors_test.go b/internal/services/shared/errors_test.go index 30af8b13b7..74449dabf0 100644 --- a/internal/services/shared/errors_test.go +++ b/internal/services/shared/errors_test.go @@ -196,6 +196,7 @@ func TestRewriteError(t *testing.T) { inputError: spiceerrors.NewWithSourceError( fmt.Errorf("invalid schema definition"), "definition document {\n relation viewer: user\n}", + "source", 1, 1, ), diff --git a/pkg/development/devcontext.go b/pkg/development/devcontext.go index 1d4445d524..16d07c43e6 100644 --- a/pkg/development/devcontext.go +++ b/pkg/development/devcontext.go @@ -334,7 +334,6 @@ func getDevError(cverr error, compiled *compiler.CompiledSchema, definitionOrCav if cverr == nil || compiled == nil || definitionOrCaveat == nil { return nil } - path := compiled.GetPathToDefinitionOrPartialOrCaveat(definitionOrCaveat.GetName()) errWithSource, ok := spiceerrors.AsWithSourceError(cverr) if ok { // NOTE: zeroes are fine here to mean "unknown" @@ -354,7 +353,7 @@ func getDevError(cverr error, compiled *compiler.CompiledSchema, definitionOrCav Context: errWithSource.SourceCodeString, Line: lineNumber, Column: columnPosition, - Path: []string{path}, + Path: []string{errWithSource.FileName}, } } return &devinterface.DeveloperError{ @@ -362,7 +361,8 @@ func getDevError(cverr error, compiled *compiler.CompiledSchema, definitionOrCav Kind: devinterface.DeveloperError_SCHEMA_ISSUE, Source: devinterface.DeveloperError_SCHEMA, Context: definitionOrCaveat.GetName(), - Path: []string{path}, + // TODO: do we have any additional context here? + Path: []string{""}, } } diff --git a/pkg/development/schema_position_mapper.go b/pkg/development/schema_position_mapper.go index df8f31c4b8..f6bfe2cbf2 100644 --- a/pkg/development/schema_position_mapper.go +++ b/pkg/development/schema_position_mapper.go @@ -92,9 +92,10 @@ func (r *SchemaPositionMapper) ReferenceAtPosition(source input.Source, position if importPath, ok := r.importReferenceChain(nodeChain); ok { importSource := input.Source(importPath) return &SchemaReference{ - ReferenceType: ReferenceTypeImport, - Source: source, - Position: position, + ReferenceType: ReferenceTypeImport, + Source: source, + Position: position, + // TODO: is this the path pointed at by the import, or is it the position of the import reference itself? TargetSource: &importSource, TargetPosition: &input.Position{LineNumber: 0, ColumnPosition: 0}, Text: importPath, @@ -296,16 +297,6 @@ func (r *SchemaPositionMapper) ReferenceAtPosition(source input.Source, position return nil, nil } -// resolveTargetSource returns the input.Source for the file where a definition or caveat -// is defined. For imported definitions, this will be the imported file path. For definitions -// in the root schema, this returns the fallback source. -func (r *SchemaPositionMapper) resolveTargetSource(name string, fallback input.Source) input.Source { - if defSource := r.schema.GetPathToDefinitionOrPartialOrCaveat(name); defSource != "" { - return input.Source(defSource) - } - return fallback -} - func (r *SchemaPositionMapper) lookupCaveat(caveatName string) (*core.CaveatDefinition, bool) { c, err := r.typeSystem.GetCaveat(context.Background(), caveatName) if err != nil { diff --git a/pkg/proto/core/v1/core.pb.go b/pkg/proto/core/v1/core.pb.go index 4afc652001..e39fb1bdba 100644 --- a/pkg/proto/core/v1/core.pb.go +++ b/pkg/proto/core/v1/core.pb.go @@ -2262,6 +2262,7 @@ type SourcePosition struct { state protoimpl.MessageState `protogen:"open.v1"` ZeroIndexedLineNumber uint64 `protobuf:"varint,1,opt,name=zero_indexed_line_number,json=zeroIndexedLineNumber,proto3" json:"zero_indexed_line_number,omitempty"` ZeroIndexedColumnPosition uint64 `protobuf:"varint,2,opt,name=zero_indexed_column_position,json=zeroIndexedColumnPosition,proto3" json:"zero_indexed_column_position,omitempty"` + FileName string `protobuf:"bytes,3,opt,name=file_name,json=fileName,proto3" json:"file_name,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -2310,6 +2311,13 @@ func (x *SourcePosition) GetZeroIndexedColumnPosition() uint64 { return 0 } +func (x *SourcePosition) GetFileName() string { + if x != nil { + return x.FileName + } + return "" +} + type CaveatExpression struct { state protoimpl.MessageState `protogen:"open.v1"` // Types that are valid to be assigned to OperationOrCaveat: @@ -3233,10 +3241,11 @@ const file_core_v1_core_proto_rawDesc = "" + "\x0fsource_position\x18\x03 \x01(\v2\x17.core.v1.SourcePositionR\x0esourcePosition\"4\n" + "\x06Object\x12\x10\n" + "\fTUPLE_OBJECT\x10\x00\x12\x18\n" + - "\x14TUPLE_USERSET_OBJECT\x10\x01\"\x8a\x01\n" + + "\x14TUPLE_USERSET_OBJECT\x10\x01\"\xa7\x01\n" + "\x0eSourcePosition\x127\n" + "\x18zero_indexed_line_number\x18\x01 \x01(\x04R\x15zeroIndexedLineNumber\x12?\n" + - "\x1czero_indexed_column_position\x18\x02 \x01(\x04R\x19zeroIndexedColumnPosition\"\x9c\x01\n" + + "\x1czero_indexed_column_position\x18\x02 \x01(\x04R\x19zeroIndexedColumnPosition\x12\x1b\n" + + "\tfile_name\x18\x03 \x01(\tR\bfileName\"\x9c\x01\n" + "\x10CaveatExpression\x128\n" + "\toperation\x18\x01 \x01(\v2\x18.core.v1.CaveatOperationH\x00R\toperation\x127\n" + "\x06caveat\x18\x02 \x01(\v2\x1d.core.v1.ContextualizedCaveatH\x00R\x06caveatB\x15\n" + diff --git a/pkg/proto/core/v1/core_vtproto.pb.go b/pkg/proto/core/v1/core_vtproto.pb.go index 1a90dbb784..a0c7dcd48d 100644 --- a/pkg/proto/core/v1/core_vtproto.pb.go +++ b/pkg/proto/core/v1/core_vtproto.pb.go @@ -892,6 +892,7 @@ func (m *SourcePosition) CloneVT() *SourcePosition { r := new(SourcePosition) r.ZeroIndexedLineNumber = m.ZeroIndexedLineNumber r.ZeroIndexedColumnPosition = m.ZeroIndexedColumnPosition + r.FileName = m.FileName if len(m.unknownFields) > 0 { r.unknownFields = make([]byte, len(m.unknownFields)) copy(r.unknownFields, m.unknownFields) @@ -2382,6 +2383,9 @@ func (this *SourcePosition) EqualVT(that *SourcePosition) bool { if this.ZeroIndexedColumnPosition != that.ZeroIndexedColumnPosition { return false } + if this.FileName != that.FileName { + return false + } return string(this.unknownFields) == string(that.unknownFields) } @@ -4835,6 +4839,13 @@ func (m *SourcePosition) MarshalToSizedBufferVT(dAtA []byte) (int, error) { i -= len(m.unknownFields) copy(dAtA[i:], m.unknownFields) } + if len(m.FileName) > 0 { + i -= len(m.FileName) + copy(dAtA[i:], m.FileName) + i = protohelpers.EncodeVarint(dAtA, i, uint64(len(m.FileName))) + i-- + dAtA[i] = 0x1a + } if m.ZeroIndexedColumnPosition != 0 { i = protohelpers.EncodeVarint(dAtA, i, uint64(m.ZeroIndexedColumnPosition)) i-- @@ -6064,6 +6075,10 @@ func (m *SourcePosition) SizeVT() (n int) { if m.ZeroIndexedColumnPosition != 0 { n += 1 + protohelpers.SizeOfVarint(uint64(m.ZeroIndexedColumnPosition)) } + l = len(m.FileName) + if l > 0 { + n += 1 + l + protohelpers.SizeOfVarint(uint64(l)) + } n += len(m.unknownFields) return n } @@ -11456,6 +11471,38 @@ func (m *SourcePosition) UnmarshalVT(dAtA []byte) error { break } } + case 3: + if wireType != 2 { + return fmt.Errorf("proto: wrong wireType = %d for field FileName", wireType) + } + var stringLen uint64 + for shift := uint(0); ; shift += 7 { + if shift >= 64 { + return protohelpers.ErrIntOverflow + } + if iNdEx >= l { + return io.ErrUnexpectedEOF + } + b := dAtA[iNdEx] + iNdEx++ + stringLen |= uint64(b&0x7F) << shift + if b < 0x80 { + break + } + } + intStringLen := int(stringLen) + if intStringLen < 0 { + return protohelpers.ErrInvalidLength + } + postIndex := iNdEx + intStringLen + if postIndex < 0 { + return protohelpers.ErrInvalidLength + } + if postIndex > l { + return io.ErrUnexpectedEOF + } + m.FileName = string(dAtA[iNdEx:postIndex]) + iNdEx = postIndex default: iNdEx = preIndex skippy, err := protohelpers.Skip(dAtA[iNdEx:]) diff --git a/pkg/schema/errors.go b/pkg/schema/errors.go index 18a3bf5b9f..d9d7a81b6d 100644 --- a/pkg/schema/errors.go +++ b/pkg/schema/errors.go @@ -423,7 +423,8 @@ func NewTypeWithSourceError(wrapped error, withSource nspkg.WithSourcePosition, return asTypeError(spiceerrors.NewWithSourceError( wrapped, sourceCodeString, - sourcePosition.ZeroIndexedLineNumber+1, // +1 to make 1-indexed + sourcePosition.FileName, + sourcePosition.ZeroIndexedLineNumber+1, // +1 to make 1-indexed sourcePosition.ZeroIndexedColumnPosition+1, // +1 to make 1-indexed )) } @@ -431,6 +432,7 @@ func NewTypeWithSourceError(wrapped error, withSource nspkg.WithSourcePosition, return asTypeError(spiceerrors.NewWithSourceError( wrapped, sourceCodeString, + "", 0, 0, )) diff --git a/pkg/schemadsl/compiler/development.go b/pkg/schemadsl/compiler/development.go index 1480e278ba..903457c098 100644 --- a/pkg/schemadsl/compiler/development.go +++ b/pkg/schemadsl/compiler/development.go @@ -55,41 +55,6 @@ func (nc *NodeChain) String() string { return out.String() } -// GetPathToDefinitionOrPartialOrCaveat returns the input source for the AST node defining the given name. -// For definitions compiled from imports, this will be the imported file path (e.g. "users.zed"). -// For definitions in the root schema, this will be the root source (e.g. "schema"). -// Returns empty string if not found. -func (cs *CompiledSchema) GetPathToDefinitionOrPartialOrCaveat(name string) string { - for _, child := range cs.rootNode.GetChildren() { - nodeType := child.GetType() - var predicateName string - switch nodeType { - case dslshape.NodeTypeDefinition: - predicateName = dslshape.NodeDefinitionPredicateName - case dslshape.NodeTypeCaveatDefinition: - predicateName = dslshape.NodeCaveatDefinitionPredicateName - case dslshape.NodeTypePartial: - predicateName = dslshape.NodePartialPredicateName - default: - continue - } - - defName, err := child.GetString(predicateName) - if err != nil { - continue - } - - if defName == name { - source, err := child.GetString(dslshape.NodePredicateSource) - if err != nil { - return "" - } - return source - } - } - return "" -} - // PartialNodePosition returns the start rune position and source of the partial // definition with the given name. Returns (-1, -1, "") if not found. func (cs *CompiledSchema) PartialNodePosition(name string) (int, int) { diff --git a/pkg/schemadsl/compiler/translator.go b/pkg/schemadsl/compiler/translator.go index 0fa90d82bf..c0eeb6b16c 100644 --- a/pkg/schemadsl/compiler/translator.go +++ b/pkg/schemadsl/compiler/translator.go @@ -336,6 +336,10 @@ func getSourcePosition(dslNode *dslNode, mapper input.PositionMapper) *core.Sour return nil } + if !dslNode.Has(dslshape.NodePredicateSource) { + return nil + } + sourceRange, err := dslNode.Range(mapper) if err != nil { return nil @@ -355,9 +359,15 @@ func getSourcePosition(dslNode *dslNode, mapper input.PositionMapper) *core.Sour uintCol = 0 } + filename, err := dslNode.GetString(dslshape.NodePredicateSource) + if err != nil { + return nil + } + return &core.SourcePosition{ ZeroIndexedLineNumber: uintLine, ZeroIndexedColumnPosition: uintCol, + FileName: filename, } } diff --git a/pkg/spiceerrors/common.go b/pkg/spiceerrors/common.go index 625faef270..c817b34cca 100644 --- a/pkg/spiceerrors/common.go +++ b/pkg/spiceerrors/common.go @@ -22,6 +22,9 @@ type WithSourceError struct { // SourceCodeString is the input source code string for the error. SourceCodeString string + // FileName is the filename where the error was found. + FileName string + // LineNumber is the (1-indexed) line number of the error, or 0 if unknown. LineNumber uint64 @@ -42,8 +45,8 @@ func (err *WithSourceError) Unwrap() error { } // NewWithSourceError creates and returns a new WithSourceError. -func NewWithSourceError(err error, sourceCodeString string, oneIndexedLineNumber uint64, oneIndexedColumnPosition uint64) *WithSourceError { - return &WithSourceError{err, sourceCodeString, oneIndexedLineNumber, oneIndexedColumnPosition} +func NewWithSourceError(err error, sourceCodeString, fileName string, oneIndexedLineNumber uint64, oneIndexedColumnPosition uint64) *WithSourceError { + return &WithSourceError{err, sourceCodeString, fileName, oneIndexedLineNumber, oneIndexedColumnPosition} } // AsWithSourceError returns the error as an WithSourceError, if applicable. diff --git a/pkg/validationfile/blocks/assertions.go b/pkg/validationfile/blocks/assertions.go index 3da0c6f053..a55ce1b534 100644 --- a/pkg/validationfile/blocks/assertions.go +++ b/pkg/validationfile/blocks/assertions.go @@ -96,6 +96,8 @@ func (a *Assertion) UnmarshalYAML(node *yamlv3.Node) error { return spiceerrors.NewWithSourceError( fmt.Errorf("error parsing assertion `%s`", trimmed), trimmed, + // TODO: this should point at the filename. is there a way to get that context here? + "", line, column, ) @@ -106,6 +108,8 @@ func (a *Assertion) UnmarshalYAML(node *yamlv3.Node) error { return spiceerrors.NewWithSourceError( fmt.Errorf("error parsing relationship in assertion `%s`: %w", trimmed, err), trimmed, + // TODO: this should point at the filename. is there a way to get that context here? + "", line, column, ) @@ -120,6 +124,8 @@ func (a *Assertion) UnmarshalYAML(node *yamlv3.Node) error { return spiceerrors.NewWithSourceError( fmt.Errorf("error parsing caveat context in assertion `%s`: %w", trimmed, err), trimmed, + // TODO: this should point at the filename. is there a way to get that context here? + "", line, column, ) diff --git a/pkg/validationfile/blocks/errors.go b/pkg/validationfile/blocks/errors.go index b30ef7abd1..066e34100b 100644 --- a/pkg/validationfile/blocks/errors.go +++ b/pkg/validationfile/blocks/errors.go @@ -38,6 +38,8 @@ func convertYamlError(err error) error { return spiceerrors.NewWithSourceError( errors.New(message), source, + // TODO: this should point at the filename. is there a way to get that context here? + "", lineNumber, 0, ) diff --git a/pkg/validationfile/blocks/expectedrelations.go b/pkg/validationfile/blocks/expectedrelations.go index 564b266bf6..d70c165dce 100644 --- a/pkg/validationfile/blocks/expectedrelations.go +++ b/pkg/validationfile/blocks/expectedrelations.go @@ -72,6 +72,8 @@ func (ors *ObjectRelation) UnmarshalYAML(node *yamlv3.Node) error { return spiceerrors.NewWithSourceError( fmt.Errorf("could not parse %s: %w", ors.ObjectRelationString, err), ors.ObjectRelationString, + // TODO: this should point at the filename. is there a way to get that context here? + "", line, column, ) @@ -144,6 +146,8 @@ func (es *ExpectedSubject) UnmarshalYAML(node *yamlv3.Node) error { return spiceerrors.NewWithSourceError( subErr, subErr.SourceCodeString, + // TODO: this should point at the filename. is there a way to get that context here? + "", line+subErr.LineNumber, column+subErr.ColumnPosition, ) @@ -154,6 +158,8 @@ func (es *ExpectedSubject) UnmarshalYAML(node *yamlv3.Node) error { return spiceerrors.NewWithSourceError( onrErr, onrErr.SourceCodeString, + // TODO: this should point at the filename. is there a way to get that context here? + "", line+onrErr.LineNumber, column+onrErr.ColumnPosition, ) @@ -192,13 +198,13 @@ func (vs ValidationString) Subject() (*SubjectWithExceptions, *spiceerrors.WithS groups := vsSubjectWithExceptionsOrCaveatRegex.FindStringSubmatch(subjectStr) if len(groups) == 0 { bracketedSubjectString := "[" + subjectStr + "]" - return nil, spiceerrors.NewWithSourceError(fmt.Errorf("invalid subject: `%s`", subjectStr), bracketedSubjectString, 0, 0) + return nil, spiceerrors.NewWithSourceError(fmt.Errorf("invalid subject: `%s`", subjectStr), bracketedSubjectString, "", 0, 0) } subjectONRString := groups[slices.Index(vsSubjectWithExceptionsOrCaveatRegex.SubexpNames(), "subject_onr")] subjectONR, err := tuple.ParseSubjectONR(subjectONRString) if err != nil { - return nil, spiceerrors.NewWithSourceError(fmt.Errorf("invalid subject: `%s`: %w", subjectONRString, err), subjectONRString, 0, 0) + return nil, spiceerrors.NewWithSourceError(fmt.Errorf("invalid subject: `%s`: %w", subjectONRString, err), subjectONRString, "", 0, 0) } exceptionsString := strings.TrimSpace(groups[slices.Index(vsSubjectWithExceptionsOrCaveatRegex.SubexpNames(), "exceptions")]) @@ -216,7 +222,7 @@ func (vs ValidationString) Subject() (*SubjectWithExceptions, *spiceerrors.WithS exceptionONR, err := tuple.ParseSubjectONR(strings.TrimSpace(exceptionString)) if err != nil { - return nil, spiceerrors.NewWithSourceError(fmt.Errorf("invalid subject: `%s`: %w", exceptionString, err), exceptionString, 0, 0) + return nil, spiceerrors.NewWithSourceError(fmt.Errorf("invalid subject: `%s`: %w", exceptionString, err), exceptionString, "", 0, 0) } exceptions = append(exceptions, SubjectAndCaveat{exceptionONR, isCaveated}) @@ -244,7 +250,7 @@ func (vs ValidationString) ONRS() ([]tuple.ObjectAndRelation, *spiceerrors.WithS for _, onrString := range onrStrings { found, err := tuple.ParseONR(onrString) if err != nil { - return nil, spiceerrors.NewWithSourceError(fmt.Errorf("invalid resource and relation: `%s`: %w", onrString, err), onrString, 0, 0) + return nil, spiceerrors.NewWithSourceError(fmt.Errorf("invalid resource and relation: `%s`: %w", onrString, err), onrString, "", 0, 0) } onrs = append(onrs, found) diff --git a/pkg/validationfile/blocks/relationships.go b/pkg/validationfile/blocks/relationships.go index 439258e7d3..6886fb8cee 100644 --- a/pkg/validationfile/blocks/relationships.go +++ b/pkg/validationfile/blocks/relationships.go @@ -59,6 +59,8 @@ func (pr *ParsedRelationships) UnmarshalYAML(node *yamlv3.Node) error { return spiceerrors.NewWithSourceError( fmt.Errorf("error parsing relationship `%s`: %w", trimmed, err), trimmed, + // TODO: this should point at the filename. is there a way to get that context here? + "", errorLine, column, ) @@ -69,6 +71,8 @@ func (pr *ParsedRelationships) UnmarshalYAML(node *yamlv3.Node) error { return spiceerrors.NewWithSourceError( fmt.Errorf("found repeated relationship `%s`", trimmed), trimmed, + // TODO: this should point at the filename. is there a way to get that context here? + "", errorLine, column, ) diff --git a/pkg/validationfile/loader.go b/pkg/validationfile/loader.go index 6c0d1c6625..6788692a3e 100644 --- a/pkg/validationfile/loader.go +++ b/pkg/validationfile/loader.go @@ -259,6 +259,7 @@ func CompileSchema(schema compiler.InputSchema, cts *caveattypes.TypeSet) (*comp return nil, spiceerrors.NewWithSourceError( fmt.Errorf("error when parsing schema: %s", errWithContext.BaseMessage), errWithContext.ErrorSourceCode, + string(errWithContext.Source), uintLine+1, // source line is 0-indexed uintCol+1, // source col is 0-indexed ) diff --git a/proto/internal/core/v1/core.proto b/proto/internal/core/v1/core.proto index f4792ff377..d2ed938f6b 100644 --- a/proto/internal/core/v1/core.proto +++ b/proto/internal/core/v1/core.proto @@ -542,6 +542,7 @@ message ComputedUserset { message SourcePosition { uint64 zero_indexed_line_number = 1; uint64 zero_indexed_column_position = 2; + string file_name = 3; } message CaveatExpression {