diff --git a/crates/squawk/src/github.rs b/crates/squawk/src/github.rs index 30c9e1c7..6bf9fa14 100644 --- a/crates/squawk/src/github.rs +++ b/crates/squawk/src/github.rs @@ -1,12 +1,13 @@ use crate::UploadToGithubArgs; use crate::config::Config; -use crate::reporter::{CheckReport, fmt_tty_violation}; +use crate::reporter::{CheckReport, fmt_github_annotations, fmt_tty_violation}; use crate::{file_finding::find_paths, reporter::check_files}; use anyhow::{Result, anyhow, bail}; use console::strip_ansi_codes; use log::info; use squawk_github::{GitHubApi, actions, app, comment_on_pr}; use squawk_linter::{Rule, Version}; +use std::io; const VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -135,6 +136,10 @@ pub fn check_and_comment_on_pr( COMMENT_HEADER, )?; + let stdout = io::stdout(); + let mut handle = stdout.lock(); + fmt_github_annotations(&mut handle, &file_results)?; + let violations: usize = file_results.iter().map(|f| f.violations.len()).sum(); if fail_on_violations && violations > 0 { @@ -257,6 +262,8 @@ SELECT 1; message: "Adding a NOT NULL field requires exclusive locks and table rewrites." .to_string(), help: Some("Make the field nullable.".to_string()), + column_end: 0, + line_end: 1, }], }]; diff --git a/crates/squawk/src/main.rs b/crates/squawk/src/main.rs index 37b27e61..9eb0d602 100644 --- a/crates/squawk/src/main.rs +++ b/crates/squawk/src/main.rs @@ -255,6 +255,8 @@ Please open an issue at https://github.com/sbdchd/squawk/issues/new with the log debug(&mut handle, &found_paths, read_stdin, &kind, opts.verbose)?; } else { let reporter = opts.reporter.unwrap_or(Reporter::Tty); + let github_annotations = std::env::var("GITHUB_ACTIONS").is_ok() + && !std::env::var("SQUAWK_DISABLE_GITHUB_ANNOTATIONS").is_ok(); let exit_code = check_and_dump_files( &mut handle, &found_paths, @@ -264,6 +266,7 @@ Please open an issue at https://github.com/sbdchd/squawk/issues/new with the log pg_version, assume_in_transaction, &reporter, + github_annotations, )?; return Ok(exit_code); } diff --git a/crates/squawk/src/reporter.rs b/crates/squawk/src/reporter.rs index a83cb089..4359894c 100644 --- a/crates/squawk/src/reporter.rs +++ b/crates/squawk/src/reporter.rs @@ -42,10 +42,14 @@ fn check_sql( for e in parse_errors { let range_start = e.range().start(); let line_col = line_index.line_col(range_start); + let range_end = e.range().end(); + let line_end = line_index.line_col(range_end); violations.push(ReportViolation { file: path.to_string(), line: line_col.line as usize, + line_end: line_end.line as usize, column: line_col.col as usize, + column_end: line_end.col as usize, level: ViolationLevel::Error, help: None, range: e.range(), @@ -56,10 +60,14 @@ fn check_sql( for e in errors { let range_start = e.text_range.start(); let line_col = line_index.line_col(range_start); + let range_end = e.text_range.end(); + let line_end = line_index.line_col(range_end); violations.push(ReportViolation { file: path.to_string(), line: line_col.line as usize, + line_end: line_end.line as usize, column: line_col.col as usize, + column_end: line_end.col as usize, range: e.text_range, help: e.help, level: ViolationLevel::Warning, @@ -158,6 +166,7 @@ pub fn check_and_dump_files( pg_version: Option, assume_in_transaction: bool, reporter: &Reporter, + github_annotations: bool, ) -> Result { let violations = check_files( path_patterns, @@ -170,7 +179,7 @@ pub fn check_and_dump_files( let ok = violations.iter().map(|x| x.violations.len()).sum::() == 0; - print_violations(f, violations, reporter)?; + print_violations(f, violations, reporter, github_annotations)?; Ok(if ok { ExitCode::SUCCESS @@ -273,6 +282,8 @@ pub struct ReportViolation { pub message: String, pub help: Option, pub rule_name: String, + pub column_end: usize, + pub line_end: usize, } fn fmt_gcc(f: &mut W, reports: &[CheckReport]) -> Result<()> { @@ -324,6 +335,30 @@ fn fmt_json(f: &mut W, reports: Vec) -> Result<()> { Ok(()) } +pub fn fmt_github_annotations(f: &mut W, reports: &[CheckReport]) -> Result<()> { + for report in reports { + for violation in &report.violations { + let level = match violation.level { + ViolationLevel::Warning => "warning", + ViolationLevel::Error => "error", + }; + + writeln!( + f, + "::{level} file={file},line={line},col={col},endLine={line_end},endColumn={col_end},title={title}::{message}", + file = violation.file, + line = violation.line + 1, + line_end = violation.line_end + 1, + col = violation.column, + col_end = violation.column_end, + title = violation.rule_name, + message = violation.message, + )?; + } + } + Ok(()) +} + #[derive(Debug)] pub struct CheckReport { pub filename: String, @@ -335,7 +370,11 @@ pub fn print_violations( writer: &mut W, reports: Vec, reporter: &Reporter, + github_annotations: bool, ) -> Result<()> { + if github_annotations { + fmt_github_annotations(writer, &reports)?; + } match reporter { Reporter::Gcc => fmt_gcc(writer, &reports), Reporter::Json => fmt_json(writer, reports), @@ -387,6 +426,7 @@ SELECT 1; &mut buff, vec![check_sql(sql, filename, &[], None, false)], &Reporter::Gcc, + false, ); assert!(res.is_ok()); @@ -400,6 +440,31 @@ SELECT 1; "###); } + #[test] + fn display_violations_tty_and_github_annotations() { + let sql = r#" + ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL; +-- multi line example +ALTER TABLE "core_foo" + ADD COLUMN "bar" + integer NOT NULL; +SELECT 1; +"#; + let filename = "main.sql"; + let mut buff = Vec::new(); + + let res = print_violations( + &mut buff, + vec![check_sql(sql, filename, &[], None, false)], + &Reporter::Tty, + true, + ); + + assert!(res.is_ok()); + // remove the color codes so tests behave in CI as they do locally + assert_snapshot!(strip_ansi_codes(&String::from_utf8_lossy(&buff))); + } + #[test] fn display_violations_tty() { let sql = r#" @@ -414,6 +479,7 @@ SELECT 1; &mut buff, vec![check_sql(sql, filename, &[], None, false)], &Reporter::Tty, + false, ); assert!(res.is_ok()); @@ -429,6 +495,7 @@ SELECT 1; &mut buff, vec![check_sql(sql, "main.sql", &[], None, false)], &Reporter::Tty, + false, ); assert!(res.is_ok()); @@ -450,10 +517,11 @@ SELECT 1; &mut buff, vec![check_sql(sql, filename, &[], None, false)], &Reporter::Json, + false, ); assert!(res.is_ok()); - assert_snapshot!(String::from_utf8_lossy(&buff), @r###"[{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field"},{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts"},{"file":"main.sql","line":1,"column":46,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int"},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field"},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts"},{"file":"main.sql","line":2,"column":40,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int"}]"###); + assert_snapshot!(String::from_utf8_lossy(&buff), @r#"[{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field","column_end":62,"line_end":1},{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts","column_end":62,"line_end":1},{"file":"main.sql","line":1,"column":46,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int","column_end":53,"line_end":1},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field","column_end":56,"line_end":2},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts","column_end":56,"line_end":2},{"file":"main.sql","line":2,"column":40,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int","column_end":47,"line_end":2}]"#); } #[test] diff --git a/crates/squawk/src/snapshots/squawk__reporter__test_check_files__check_files_invalid_syntax.snap b/crates/squawk/src/snapshots/squawk__reporter__test_check_files__check_files_invalid_syntax.snap index 3fd6484a..d9728fa9 100644 --- a/crates/squawk/src/snapshots/squawk__reporter__test_check_files__check_files_invalid_syntax.snap +++ b/crates/squawk/src/snapshots/squawk__reporter__test_check_files__check_files_invalid_syntax.snap @@ -1,5 +1,5 @@ --- -source: crates/cli/src/reporter.rs +source: crates/squawk/src/reporter.rs expression: val --- -[{"column":6,"file":"test.sql","help":null,"level":"Error","line":1,"message":"expected SEMICOLON","rule_name":"syntax-error"},{"column":7,"file":"test.sql","help":null,"level":"Error","line":1,"message":"expected command, found ERROR","rule_name":"syntax-error"}] +[{"column":6,"column_end":6,"file":"test.sql","help":null,"level":"Error","line":1,"line_end":1,"message":"expected SEMICOLON","rule_name":"syntax-error"},{"column":7,"column_end":7,"file":"test.sql","help":null,"level":"Error","line":1,"line_end":1,"message":"expected command, found ERROR","rule_name":"syntax-error"}] diff --git a/crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty_and_github_annotations.snap b/crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty_and_github_annotations.snap new file mode 100644 index 00000000..43026010 --- /dev/null +++ b/crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty_and_github_annotations.snap @@ -0,0 +1,55 @@ +--- +source: crates/squawk/src/reporter.rs +expression: "strip_ansi_codes(&String::from_utf8_lossy(&buff))" +--- +::warning file=main.sql,line=2,col=29,endLine=2,endColumn=62,title=adding-required-field::Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required. +::warning file=main.sql,line=2,col=29,endLine=2,endColumn=62,title=prefer-robust-stmts::Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through. +::warning file=main.sql,line=2,col=46,endLine=2,endColumn=53,title=prefer-bigint-over-int::Using 32-bit integer fields can result in hitting the max `int` limit. +::warning file=main.sql,line=5,col=2,endLine=6,endColumn=20,title=adding-required-field::Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required. +::warning file=main.sql,line=5,col=2,endLine=6,endColumn=20,title=prefer-robust-stmts::Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through. +::warning file=main.sql,line=6,col=4,endLine=6,endColumn=11,title=prefer-bigint-over-int::Using 32-bit integer fields can result in hitting the max `int` limit. +warning[adding-required-field]: Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required. + --> main.sql:2:30 + | +2 | ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL; + | --------------------------------- + | + = help: Make the field nullable or add a non-VOLATILE DEFAULT +warning[prefer-robust-stmts]: Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through. + --> main.sql:2:30 + | +2 | ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL; + | --------------------------------- + | +warning[prefer-bigint-over-int]: Using 32-bit integer fields can result in hitting the max `int` limit. + --> main.sql:2:47 + | +2 | ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL; + | ------- + | + = help: Use 64-bit integer values instead to prevent hitting this limit. +warning[adding-required-field]: Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required. + --> main.sql:5:3 + | +5 | / ADD COLUMN "bar" +6 | | integer NOT NULL; + | |____________________- + | + = help: Make the field nullable or add a non-VOLATILE DEFAULT +warning[prefer-robust-stmts]: Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through. + --> main.sql:5:3 + | +5 | / ADD COLUMN "bar" +6 | | integer NOT NULL; + | |____________________- + | +warning[prefer-bigint-over-int]: Using 32-bit integer fields can result in hitting the max `int` limit. + --> main.sql:6:5 + | +6 | integer NOT NULL; + | ------- + | + = help: Use 64-bit integer values instead to prevent hitting this limit. + +Find detailed examples and solutions for each rule at https://squawkhq.com/docs/rules +Found 6 issues in 1 file (checked 1 source file) diff --git a/crates/squawk/src/snapshots/squawk__reporter__test_reporter__span_offsets.snap b/crates/squawk/src/snapshots/squawk__reporter__test_reporter__span_offsets.snap index e206e57d..6dcd212a 100644 --- a/crates/squawk/src/snapshots/squawk__reporter__test_reporter__span_offsets.snap +++ b/crates/squawk/src/snapshots/squawk__reporter__test_reporter__span_offsets.snap @@ -1,5 +1,5 @@ --- -source: crates/cli/src/reporter.rs +source: crates/squawk/src/reporter.rs expression: "check_sql(sql, filename, &[], None, false)" --- CheckReport { @@ -17,6 +17,8 @@ CheckReport { "Make the field nullable or add a non-VOLATILE DEFAULT", ), rule_name: "adding-required-field", + column_end: 62, + line_end: 2, }, ReportViolation { file: "main.sql", @@ -27,6 +29,8 @@ CheckReport { message: "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.", help: None, rule_name: "prefer-robust-stmts", + column_end: 62, + line_end: 2, }, ReportViolation { file: "main.sql", @@ -39,6 +43,8 @@ CheckReport { "Use 64-bit integer values instead to prevent hitting this limit.", ), rule_name: "prefer-bigint-over-int", + column_end: 53, + line_end: 2, }, ReportViolation { file: "main.sql", @@ -51,6 +57,8 @@ CheckReport { "Make the field nullable or add a non-VOLATILE DEFAULT", ), rule_name: "adding-required-field", + column_end: 56, + line_end: 3, }, ReportViolation { file: "main.sql", @@ -61,6 +69,8 @@ CheckReport { message: "Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.", help: None, rule_name: "prefer-robust-stmts", + column_end: 56, + line_end: 3, }, ReportViolation { file: "main.sql", @@ -73,6 +83,8 @@ CheckReport { "Use 64-bit integer values instead to prevent hitting this limit.", ), rule_name: "prefer-bigint-over-int", + column_end: 47, + line_end: 3, }, ], }