Censor fixes

[storojs72]

1. Move common table matching logic from whitelist/blacklist handlers;
2. Fix parsing error logic
3. Some refactoring

[Lagovas]

for what second parsing the same query?

[storojs72]

Because I need initial query (without redacted values) in parsed form outside

[Lagovas]

then better to parse second time after error check because in worst case we will parse two times with errors without need

[storojs72]

Agree

[Lagovas]

To avoid casts inside case statements we can cast once using switch statement and reduce lines of code below

switch query := parsedQuery.(type) {
  case *sqlparser.Select:
      // query here casted to *sqlparser.Select
      //query.From
}

[storojs72]

Agree

[Lagovas]

is it works for case .... FROM table1 as tbl ? Is Table.Name.String() will not return tbl instead table1?

[storojs72]

This is INSERT statement. Only one table is possible

[Lagovas]

else { 
  break
}

we don't need to continue if we have one false for alltablesMatchInternal

[storojs72]

To use it both whitelist and blacklist we should have information if AT LEAST ONE table is in list (blacklist blocks table if it's true, allows - otherwise) and if ALL tables are in list (whitelist allows table if it's true and blocks otherwise)

[Lagovas]

yes, but if we have oneTableMatch == true and meet first allTablesMatchInternal == false then we can stop iteration and return true, false because all allTablesMatchInternal will not true anyway

[storojs72]

Agree

[Lagovas]

I think we can simplify this function and don't add specific functions for each type of table. Each case will be < 10 lines of code and here we work with specific types. On my opinion better to check all cases here instead in separate functions (checkAliasedTable, checkJoinedTable, checkParenTable)
switch tbl := table.(type) {
then we can reduce code and use recursion

case *sqlparser.ParenTableExpr:
		oneTableMatch, allTablesMatch = checkParenTable(tbl.Exprs, setOfTables)

[Lagovas]

nice

[Lagovas]

what about store query as is without normalization? to support unparsable queries

[Lagovas]

oh, I missed handler.queries[query] = true
I think we should store one variant: normalized or as is. We don't need to store both IMHO

[storojs72]

I think that we should save both versions (normalized and raw). If parsing error occurred, we add/remove only raw version from list in all cases

[Lagovas]

for what we need store both versions? how we use it? we can add/remove only normalized or raw query. anyway, we try to parse in both cases and check with logical OR both versions (in queryignore handler). so we can reduce overhead of memory and store one version

[storojs72]

In previous version we tried to parse and store normilized. If parse error occurred, we save as is. I agree that we have to save only normalized or raw. It seems that raw type is better - no parse, faster work

[storojs72]

Eventually I decided to use normalized queries CheckQuery and normalize them in Add/Remove Queries functions. So "SELECT * FROM STUDENTS;" will match "select * from students" but if parse error occurred in Add/Remove, then censor will throw it while loading

[Lagovas]

if we can parse then we operate with parsed queries. and when we will try to add/remove parsable query then it will correctly added/removed. and when we cant parse then we add as is in raw form and add/remove will fail with parsing too and correctly added/removed using raw form of query

[storojs72]

I think we shouldn't allow add to black/white list unparsed queries - we check normalized queries in CheckQuery - not raw, and normalized query means that it's parsed.

[Lagovas]

it's true for patterns or logic which require correct parsing, but not simple comparison as is

[storojs72]

Currently every input query is parsed twice (one statement used for hiding values and second is processed) independently of further handling logic. When we parse while add/remove, it's done only once when loading. So even if we use unparsed queries in checking queries, It will not increase performance, but may cause some annoying typo errors, like add to whitelist "select * from x" but it will not block "select * from x;" because strings are not actually equal.

[Lagovas]

can we log an original error to see it in logs what actually happens and where was a mistake? sometimes it was need for tests and debugging