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

Fix error matching issue due to wrong error.query trimming in find_suiable_error #108

Merged
merged 1 commit into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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]