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

Jump to bottom

fix(sqllab): clean comments within quotes #23908

Merged
merged 4 commits into from
May 5, 2023
+70 −3
Merged

fix(sqllab): clean comments within quotes #23908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
167d6a2
fix(sqllab): clean comments within quotes
justinpark May 2, 2023
7ef533c
add comments for code detail
justinpark May 3, 2023
da2810d
fix the matching quote condition
justinpark May 3, 2023
0d1cc15
optimize forEach operation by while-loop
justinpark May 3, 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
60 changes: 59 additions & 1 deletion superset-frontend/src/SqlLab/actions/sqlLab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,12 +357,70 @@ export function fetchQueryResults(query, displayLimit) {
};
}

const quotes = '\'"`'.split('');
const quotedBlockHash = shortid.generate();
const quotedBlockMatch = new RegExp(`${quotedBlockHash}:\\d+:`, 'g');

function splitByQuotedBlock(str) {
const chunks = [];
let currentQuote = '';
let chunkStart = 0;

let i = 0;
while (i < str.length) {
const currentChar = str[i];
if (
currentQuote ? currentChar === currentQuote : quotes.includes(currentChar)
) {
let chunk;
if (currentQuote) {
chunk = str.substring(chunkStart, i + 1);
chunkStart = i + 1;
currentQuote = '';
} else {
chunk = str.substring(chunkStart, i);
chunkStart = i;
currentQuote = currentChar;
}
if (chunk) {
chunks.push(chunk);
}
}
i += 1;
}

if (chunkStart < str.length) {
const lastChunk = str.substring(chunkStart);
if (lastChunk) {
chunks.push(lastChunk);
}
}

return chunks;
}

export function cleanSqlComments(sql) {
if (!sql) return '';
// it sanitizes the following comment block groups
// group 1 -> /* */
// group 2 -> --
return sql.replace(/(--.*?$|\/\*[\s\S]*?\*\/)\n?/gm, '\n').trim();
const chunks = splitByQuotedBlock(sql);
return (
chunks
// replace quoted blocks in a hash format
.map((chunk, index) =>
quotes.includes(chunk[0]) ? `${quotedBlockHash}:${index}:` : chunk,
)
.join('')
// Clean out the commented-out blocks
.replace(/(--.*?$|\/\*[\s\S]*?\*\/)\n?/gm, '\n')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.replace(/(--.*?$|\/\*[\s\S]*?\*\/)\n?/gm, '\n')
.replace(/(--.*$|\/\*[\s\S]*\*\/)\n?/gm, '\n')

I don't think these question marks are necessary

Copy link
Member Author

@justinpark justinpark May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This ? mark covers the multiple comment group case

i.e.

/*
comments
*/
no-comments
/*
comments
*/

.replace(/(--.?$|/*[\s\S]?*/)\n?/gm, '\n') => no-comments
.replace(/(--.$|/*[\s\S]*/)\n?/gm, '\n') => '\n'

.trim()
// restore quoted block to the original value
.replace(
quotedBlockMatch,
quotedBlock => chunks[quotedBlock.match(/:\d+/)[0].substring(1)],
)
);
}

export function runQuery(query) {
Expand Down
13 changes: 11 additions & 2 deletions superset-frontend/src/SqlLab/actions/sqlLab.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,16 @@ describe('async actions', () => {
const makeRequest = () => {
const request = actions.runQuery({
...query,
sql: '/*\nSELECT * FROM\n */\nSELECT 213--, {{ds}}\n/*\n{{new_param1}}\n{{new_param2}}*/\n\nFROM table',
sql: `/*
SELECT * FROM
*/
SELECT 213--, {{ds}} "quote out"
/*
{{new_param1}}
{{new_param2}}*/

FROM table
WHERE value = '--"NULL"--' --{{test_param}}`,
});
return request(dispatch, () => initialState);
};
Expand All @@ -326,7 +335,7 @@ describe('async actions', () => {
expect(fetchMock.calls(runQueryEndpoint)).toHaveLength(1);
expect(
JSON.parse(fetchMock.calls(runQueryEndpoint)[0][1].body).sql,
).toEqual('SELECT 213\n\n\nFROM table');
).toEqual(`SELECT 213\n\n\nFROM table\nWHERE value = '--"NULL"--'`);
});
});
});
Expand Down