Skip to content

Commit

Permalink
Fix error matching issue due to wrong error.query trimming in find_su…
Browse files Browse the repository at this point in the history
…itable_error
  • Loading branch information
surister committed Oct 24, 2024
1 parent 58765b9 commit da7b8d5
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 20 deletions.
14 changes: 2 additions & 12 deletions cratedb_sqlparse_js/cratedb_sqlparse/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import SqlBaseParser from "./generated_parser/SqlBaseParser.js";
import {CommonTokenStream, ErrorListener, InputStream, Interval, Token} from "antlr4";
import {AstBuilder} from "./AstBuilder.js";
import {Metadata} from "./models.js"

function BEGIN_DOLLAR_QUOTED_STRING_action(localctx, actionIndex) {
if (actionIndex === 0) {
this.tags.push(this.text);
Expand Down Expand Up @@ -180,16 +181,6 @@ export class Statement {
}
}

/**
*
* @param {string} string
* @returns {string}
*/
function trim(string) {
return string.replace(/^\s+|\s+$/gm, '');
}


function findSuitableError(statement, errors) {
for (const error of errors) {
let errorQuery = error.query;
Expand All @@ -198,7 +189,7 @@ function findSuitableError(statement, errors) {
errorQuery = errorQuery.substring(0, errorQuery.length - 1);
}

errorQuery = trim(errorQuery);
errorQuery = errorQuery.trimStart().trimEnd()

// If a good match error_query contains statement.query
if (errorQuery.includes(statement.query)) {
Expand Down Expand Up @@ -263,7 +254,6 @@ export function sqlparse(query, raise_exception = false) {
console.error("Could not match errors to queries, too much ambiguity, please report it opening an issue with the query.")
}


const stmtEnricher = new AstBuilder()

for (const stmt of statements) {
Expand Down
39 changes: 36 additions & 3 deletions cratedb_sqlparse_js/tests/exceptions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ test('White or special characters should not avoid exception catching', () => {
}
})

test('Missing token error should not panic', ()=> {
test('Missing token error should not panic', ()=> {
// See https://github.com/crate/cratedb-sqlparse/issues/66
sqlparse(`
CREATE TABLE t01 (
Expand All @@ -104,8 +104,7 @@ test('Missing token error should not panic', ()=> {
`)
})


test('Whitetest or special characters should not avoid exception catching', () => {
test('Special characters should not avoid exception catching', () => {
// https://github.com/crate/cratedb-sqlparse/issues/67
const stmts = [
`SELECT 1\n limit `,
Expand All @@ -117,4 +116,38 @@ test('Whitetest or special characters should not avoid exception catching', () =
let r = sqlparse(stmt)
expect(r[0].exception).toBeDefined();
}
})

test('Special query with several errors should correctly be matched regardless of spaces', () => {
// See https://github.com/crate/cratedb-sqlparse/issues/107
const stmts = [
`
SELECT A FROM tbl1 where ;
SELECT 1;
SELECT D, A FROM tbl1 WHERE;
`,

`
SELECT
A
FROM
tbl1
WHERE;
SELECT
1;
SELECT
B
FROM
tbl1
WHERE;
`
]
for (const stmt of stmts) {
const r = sqlparse(stmt)
expect(r[0].exception).toBeDefined()
expect(r[1].exception).toBeNull()
expect(r[2].exception).toBeDefined()
}
})
55 changes: 50 additions & 5 deletions cratedb_sqlparse_py/tests/test_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def test_sqlparse_collects_exceptions():
def test_sqlparse_collects_exceptions_2():
from cratedb_sqlparse import sqlparse

# Different combination of the query to validate
# Different combination of the queries to validate
r = sqlparse("""
SELEC 1;
SELECT A, B, C, D FROM tbl1;
Expand All @@ -73,7 +73,7 @@ def test_sqlparse_collects_exceptions_2():
def test_sqlparse_collects_exceptions_3():
from cratedb_sqlparse import sqlparse

# Different combination of the query to validate
# Different combination of the queries to validate
r = sqlparse("""
SELECT 1;
SELECT A, B, C, D FROM tbl1;
Expand All @@ -86,10 +86,13 @@ def test_sqlparse_collects_exceptions_3():


def test_sqlparse_catches_exception():
"""
Special characters should not stop sqlparse from creating and matching the exception.
See https://github.com/crate/cratedb-sqlparse/issues/67
"""
from cratedb_sqlparse import sqlparse

# Special characters shouldn't avoid exception catching.
# https://github.com/crate/cratedb-sqlparse/issues/67
stmts = """
SELECT 1\n limit,
SELECT 1\r limit,
Expand All @@ -101,6 +104,11 @@ def test_sqlparse_catches_exception():


def test_sqlparse_should_not_panic():
"""
Missing token ')' in this case, should not throw a Runtime Exception.
See https://github.com/crate/cratedb-sqlparse/issues/66
"""
from cratedb_sqlparse import sqlparse

sqlparse("""
Expand All @@ -110,4 +118,41 @@ def test_sqlparse_should_not_panic():
);
""")[0]

# That's it, it shouldn't raise a runtime Exception.

def test_sqlparse_match_exceptions_spaces():
"""
Regardless of spaces, errors should be correctly matched to their original statement.
See https://github.com/crate/cratedb-sqlparse/issues/107
"""
from cratedb_sqlparse import sqlparse

stmts = [
"""
SELECT A FROM tbl1 where ;
SELECT 1;
SELECT D, A FROM tbl1 WHERE;
""",
"""
SELECT
A
FROM
tbl1
WHERE;
SELECT
1;
SELECT
B
FROM
tbl1
WHERE;
""",
]

for stmt in stmts:
r = sqlparse(stmt)
assert r[0]
assert r[1]
assert r[2]

0 comments on commit da7b8d5

Please sign in to comment.