Support for f-strings in SQLQueryParameterization

[andrecsilva]

Overview

Added support for query parameterization contained in f-strings.

Description

Aside from the main change, the following are of note:

• Added a new utility: BaseType to represent literal types in python. It comes along with a method to infer an expression type from a few common cases
• Changed how multiple expressions in a single injection are parameterized. The reason is to avoid problems with type conversions. For example, given the following:

cursor.execute("SELECT * FROM users where name ='" + name + '_and_' + lastname + "'")

will now produce:

cursor.execure("SELECT * FROM users where name =?", ('{0}_and_{1}'.format(name, lastname), ))

while before it would result in:

cursor.execure("SELECT * FROM users where name =?", (name + '_and_' + lastname, ))

The .format method was chosen for compatibility. While f-strings would look better, they require python 3.6+.

[codecov]

Codecov Report

Merging #124 (672c4cd) into main (954a20e) will increase coverage by 0.46%.
The diff coverage is 97.15%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #124      +/-   ##
==========================================
+ Coverage   95.82%   96.28%   +0.46%     
==========================================
  Files          64       64              
  Lines        2612     2695      +83     
==========================================
+ Hits         2503     2595      +92     
+ Misses        109      100       -9     

Files	Coverage Δ
src/codemodder/codemods/utils.py	93.50% <100.00%> (+3.71%)	⬆️
...ansformations/remove_empty_string_concatenation.py	96.96% <96.00%> (+1.31%)	⬆️
src/core_codemods/sql_parameterization.py	96.21% <96.74%> (+4.88%)	⬆️

[clavedeluna]

kinda similar to clean_simplestring which is in another util module. At some point we should try to combine

[drdavella]

LGTM overall, thanks for all of the hard work.

I do think there is some opportunity here to reduce duplication and improve readability, which would be nice to do before merging.

My longer term concern is that this is pretty complicated code, which makes it fairly difficult to understand as a reviewer/maintainer. It is also going to continue to grow as we support additional string processing and probably other SQL drivers.

Right now it feels like there is not a clear separation of concerns between the code that processes string literals and the core codemod itself. I think going forward it would be good to break these things into separate components where possible before this becomes too unwieldy.

[drdavella]

Just a nitpick but feels like this would read slightly cleaner with a match?

[andrecsilva]

Same as the previous comment, match would create an extra indent with no benefit. As a personal rule, I tend to use match to capture complex patterns or enforce types in case body. Mostly to appease the static analysis.

I'm not against to just use match in those cases. Maybe we could talk about adopting an informal coding standard here.

[drdavella]

That sounds good to me. I'm a little unclear on where match is beneficial myself. Although I do agree in certain places it appeases the type checker and in other places it enables destructuring which is quite useful.

[drdavella]

match?

[andrecsilva]

match would create an extra indent here with no additional benefit.

[drdavella]

Other than the final condition, this method exactly duplicates the body of _is_literal_start, which suggests it should be factored out.

[andrecsilva]

There is not much benefit in doing that. To elaborate, let's ponder what would be the return of the newly factored out method.

The logical, straightforward answer would be Optional[Tuple[...]] since we extract the prefix and raw_value and check for failing conditions (i.e. the b prefix). But since this is also a may-fail method, we would need to add an extra check for this which would result in a similar duplicated code.

This type of chaining of may-fail methods are prevalent here and the overall code could be cleaned up quite a using maybe monad as a programming pattern. As much as I'd like do use it, I've been avoiding it (at least explicitly) since would cost readability quite a bit as monads are seemingly infamously hard to grasp.

[andrecsilva]

LGTM overall, thanks for all of the hard work.

I do think there is some opportunity here to reduce duplication and improve readability, which would be nice to do before merging.

My longer term concern is that this is pretty complicated code, which makes it fairly difficult to understand as a reviewer/maintainer. It is also going to continue to grow as we support additional string processing and probably other SQL drivers.

Right now it feels like there is not a clear separation of concerns between the code that processes string literals and the core codemod itself. I think going forward it would be good to break these things into separate components where possible before this becomes too unwieldy.

I've added a comment explaining the major steps of the transformations. For now I'm expecting the code to change quite a bit when we add support for other string format options, so I'm avoiding factoring out some of the string processing methods.