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.

Sign up for GitHub

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

fix: supply proper location for GitHub Action format #1081

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: supply proper location for GitHub Action format #1081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23037bb
feat: Expose lint.FileLocationFromPBLocation
Feb 6, 2023
5a64cb4
fix(gh-1071): Fix file location in github output
Feb 6, 2023
4ea63a6
Fix bug from rebase
Jun 14, 2023
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
18 changes: 2 additions & 16 deletions cmd/api-linter/github_actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,8 @@ func formatGitHubActionOutput(responses []lint.Response) []byte {

fmt.Fprintf(&buf, "::error file=%s", response.FilePath)
if problem.Location != nil {
// Some findings are *line level* and only have start positions but no
// starting column. Construct a switch fallthrough to emit as many of
// the location indicators are included.
switch len(problem.Location.Span) {
case 4:
fmt.Fprintf(&buf, ",endColumn=%d", problem.Location.Span[3])
fallthrough
case 3:
fmt.Fprintf(&buf, ",endLine=%d", problem.Location.Span[2])
fallthrough
case 2:
fmt.Fprintf(&buf, ",col=%d", problem.Location.Span[1])
fallthrough
case 1:
fmt.Fprintf(&buf, ",line=%d", problem.Location.Span[0])
}
location := lint.FileLocationFromPBLocation(problem.Location, nil)
fmt.Fprintf(&buf, ",line=%d,col=%d,endLine=%d,endColumn=%d", location.Start.Line, location.Start.Column, location.End.Line, location.End.Column)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we believe that all findings contain line and column numbers? Sorry if that's obvious from other changes in the project but this caused it to nil pointer exception for me before I made the change you're removing here

Copy link
Author

Choose a reason for hiding this comment

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

This definitely depends on the logic in lint/problem.go being correct, but according to the code comment on line 128 the underlying Span should be guaranteed to have either 3 (line,startCol,endCol) or 4 (startLine,startCol,endLine,endCol) values.

I'd be curious if you have a case to reproduce that, and if it also causes problems for the YAML and JSON output? I think those should panic on main if a span is ever too short

}

// GitHub uses :: as control characters (which are also used to delimit
Expand Down
24 changes: 4 additions & 20 deletions cmd/api-linter/github_actions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,27 +53,11 @@ func TestFormatGitHubActionOutput(t *testing.T) {
Span: []int32{5, 6, 7},
},
},
{
RuleID: "line::col",
Message: "Line and column",
Location: &descriptorpb.SourceCodeInfo_Location{
Span: []int32{5, 6},
},
},
{
RuleID: "line",
Message: "Line only",
Location: &descriptorpb.SourceCodeInfo_Location{
Span: []int32{5},
},
},
},
},
},
want: `::error file=example.proto,endColumn=8,endLine=7,col=6,line=5,title=line։։col։։endLine։։endColumn::line, column, endline, and endColumn
::error file=example.proto,endLine=7,col=6,line=5,title=line։։col։։endLine::Line, column, and endline
::error file=example.proto,col=6,line=5,title=line։։col::Line and column
::error file=example.proto,line=5,title=line::Line only
want: `::error file=example.proto,line=6,col=7,endLine=8,endColumn=8,title=line։։col։։endLine։։endColumn::line, column, endline, and endColumn
::error file=example.proto,line=6,col=7,endLine=6,endColumn=7,title=line։։col։։endLine::Line, column, and endline
`,
},
{
Expand All @@ -98,8 +82,8 @@ func TestFormatGitHubActionOutput(t *testing.T) {
},
},
},
want: `::error file=example.proto,endColumn=4,endLine=3,col=2,line=1,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names
::error file=example.proto,endColumn=8,endLine=7,col=6,line=5,title=core։։naming_formats։։field_names::multi\nline\ncomment\n\nhttps://linter.aip.dev/naming_formats/field_names
want: `::error file=example.proto,line=2,col=3,endLine=4,endColumn=4,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names
::error file=example.proto,line=6,col=7,endLine=8,endColumn=8,title=core։։naming_formats։։field_names::multi\nline\ncomment\n\nhttps://linter.aip.dev/naming_formats/field_names
`,
},
{
Expand Down
32 changes: 16 additions & 16 deletions lint/problem.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,14 @@ func (p Problem) marshal() interface{} {
return struct {
Message string `json:"message" yaml:"message"`
Suggestion string `json:"suggestion,omitempty" yaml:"suggestion,omitempty"`
Location fileLocation `json:"location" yaml:"location"`
Location FileLocation `json:"location" yaml:"location"`
RuleID RuleName `json:"rule_id" yaml:"rule_id"`
RuleDocURI string `json:"rule_doc_uri" yaml:"rule_doc_uri"`
Category string `json:"category,omitempty" yaml:"category,omitempty"`
}{
p.Message,
p.Suggestion,
fileLocationFromPBLocation(loc, p.Descriptor),
FileLocationFromPBLocation(loc, p.Descriptor),
p.RuleID,
p.GetRuleURI(),
p.category,
Expand All @@ -106,35 +106,35 @@ func (p Problem) GetRuleURI() string {
return getRuleURL(string(p.RuleID), ruleURLMappings)
}

// position describes a one-based position in a source code file.
// Position describes a one-based Position in a source code file.
// They are one-indexed, as a human counts lines or columns.
type position struct {
type Position struct {
Line int `json:"line_number" yaml:"line_number"`
Column int `json:"column_number" yaml:"column_number"`
}

// fileLocation describes a location in a source code file.
// FileLocation describes a location in a source code file.
//
// Note: Positions are one-indexed, as a human counts lines or columns
// in a file.
type fileLocation struct {
Start position `json:"start_position" yaml:"start_position"`
End position `json:"end_position" yaml:"end_position"`
type FileLocation struct {
Start Position `json:"start_position" yaml:"start_position"`
End Position `json:"end_position" yaml:"end_position"`
Path string `json:"path" yaml:"path"`
}

// fileLocationFromPBLocation returns a new fileLocation object based on a
// FileLocationFromPBLocation returns a new fileLocation object based on a
// protocol buffer SourceCodeInfo_Location
func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d desc.Descriptor) fileLocation {
func FileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d desc.Descriptor) FileLocation {
// Spans are guaranteed by protobuf to have either three or four ints.
span := []int32{0, 0, 1}
if l != nil {
span = l.Span
}

var fl fileLocation
var fl FileLocation
if d != nil {
fl = fileLocation{Path: d.GetFile().GetName()}
fl = FileLocation{Path: d.GetFile().GetName()}
}

// If `span` has four ints; they correspond to
Expand All @@ -143,11 +143,11 @@ func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d desc.Descripto
// We add one because spans are zero-indexed, but not to the end column
// because we want the ending position to be inclusive and not exclusive.
if len(span) == 4 {
fl.Start = position{
fl.Start = Position{
Line: int(span[0]) + 1,
Column: int(span[1]) + 1,
}
fl.End = position{
fl.End = Position{
Line: int(span[2]) + 1,
Column: int(span[3]),
}
Expand All @@ -159,11 +159,11 @@ func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d desc.Descripto
//
// We add one because spans are zero-indexed, but not to the end column
// because we want the ending position to be inclusive and not exclusive.
fl.Start = position{
fl.Start = Position{
Line: int(span[0]) + 1,
Column: int(span[1]) + 1,
}
fl.End = position{
fl.End = Position{
Line: int(span[0]) + 1,
Column: int(span[2]),
}
Expand Down
34 changes: 34 additions & 0 deletions lint/problem_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,3 +120,37 @@ func TestProblemDescriptor(t *testing.T) {
})
}
}

func TestFileLocationFromPBLocation(t *testing.T) {
tests := []struct {
testName string
pbLocation *dpb.SourceCodeInfo_Location
expectedLocation FileLocation
}{
{
testName: "MultilineSpan",
pbLocation: &dpb.SourceCodeInfo_Location{Span: []int32{2, 0, 5, 70}},
expectedLocation: FileLocation{
Start: Position{3, 1},
End: Position{6, 70},
},
},
{
testName: "SingleLineSpan",
pbLocation: &dpb.SourceCodeInfo_Location{Span: []int32{2, 0, 42}},
expectedLocation: FileLocation{
Start: Position{3, 1},
End: Position{3, 42},
},
},
}

for _, test := range tests {
t.Run(test.testName, func(t *testing.T) {
actual := FileLocationFromPBLocation(test.pbLocation)
if actual != test.expectedLocation {
t.Errorf("Got:\n%v\nExpected\n%v", actual, test.expectedLocation)
}
})
}
}