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

Initial Version of SQL Parameterizer Codemod #90

Merged
merged 13 commits into from
Oct 25, 2023
Merged

Conversation

andrecsilva
Copy link
Contributor

Overview

Adds a codemod that parameterizes SQL queries.

@andrecsilva andrecsilva marked this pull request as ready for review October 23, 2023 13:45
Copy link
Member

@drdavella drdavella left a comment

Choose a reason for hiding this comment

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

This is some really impressive work overall. I'm relying heavily on the test cases to help me understand the feature. Most of my comments are minor in themselves but I think they all add up to a requested change.

# research how to detect attribute assigns in libcst
return [node]

def _find_gparent(self, n: cst.CSTNode) -> Optional[cst.CSTNode]:
Copy link
Member

Choose a reason for hiding this comment

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

We have some similar logic in use-walrus-if:

if parent := self.get_metadata(ParentNodeProvider, node):
if gparent := self.get_metadata(ParentNodeProvider, parent):
if (idx := gparent.children.index(parent)) >= 0:

It might be useful to generalize this into a separate utility function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could go the way of NameResolutionMixin and make something like ParentProviderMixin, but this feels like a small enough operation that it does not warrant a whole class for it. Besides I can't think of another operation that would go into that class and be used in other codemods for now.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking something more like a simple function/method that returned a grandparent node but it's not that important.

return updated_node


class SQLQueryParameterization(BaseCodemod, Codemod):
Copy link
Member

Choose a reason for hiding this comment

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

I think this ends up being cleaner if we use codemodder.api.BaseCodemod as the base here instead. It requires a few tweaks to the metadata fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want it to inherit from libcst's Codemod and not BaseTransformer -> VisitorBasedCodemodCommand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah and as long as we inherit from libcst's Codemod we can't use our current API. We'll have to create api base classes for this later on

src/core_codemods/sql_parameterization.py Outdated Show resolved Hide resolved
src/core_codemods/sql_parameterization.py Show resolved Hide resolved
src/core_codemods/sql_parameterization.py Show resolved Hide resolved
src/core_codemods/sql_parameterization.py Show resolved Hide resolved
src/core_codemods/sql_parameterization.py Show resolved Hide resolved
src/core_codemods/sql_parameterization.py Outdated Show resolved Hide resolved
src/core_codemods/sql_parameterization.py Outdated Show resolved Hide resolved
src/core_codemods/sql_parameterization.py Outdated Show resolved Hide resolved
src/core_codemods/sql_parameterization.py Outdated Show resolved Hide resolved
src/core_codemods/sql_parameterization.py Outdated Show resolved Hide resolved
tests/codemods/test_sql_parameterization.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #90 (efdbd14) into main (6187027) will decrease coverage by 0.55%.
The diff coverage is 91.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
- Coverage   95.72%   95.18%   -0.55%     
==========================================
  Files          49       51       +2     
  Lines        2012     2284     +272     
==========================================
+ Hits         1926     2174     +248     
- Misses         86      110      +24     
Files Coverage Δ
src/core_codemods/__init__.py 100.00% <100.00%> (ø)
...ansformations/remove_empty_string_concatenation.py 95.65% <95.65%> (ø)
src/codemodder/codemods/utils.py 89.79% <86.66%> (-5.45%) ⬇️
src/core_codemods/sql_parameterization.py 91.36% <91.36%> (ø)

@andrecsilva
Copy link
Contributor Author

The 100% coverage here is not happening unless I remove the formatted string placeholders and some sanity checks (e.g. the exception in _find_gparent()).

Copy link
Member

@drdavella drdavella left a comment

Choose a reason for hiding this comment

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

Looks good except for two minor comments that need to be addressed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this was intended to be committed. Can you add a pattern to .gitignore so we avoid this in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this to run the integration test without issues.

Copy link
Member

Choose a reason for hiding this comment

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

In general I'd like to avoid committing binary artifacts like this to the source tree. I don't want to hold this PR up any longer but eventually we should create this database as part of the test setup and then remove it at teardown.

Copy link
Contributor Author

@andrecsilva andrecsilva Oct 24, 2023

Choose a reason for hiding this comment

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

I just learned that you can create a memory-only database with the ":memory:" argument in sqlite3. I'll adjust the integration test and remove the extra file.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@andrecsilva andrecsilva merged commit 9f53a04 into main Oct 25, 2023
10 of 11 checks passed
@andrecsilva andrecsilva deleted the sqlp-prototype branch October 25, 2023 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants