-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
chore!: update mutator to take kwargs #19083
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19083 +/- ##
==========================================
- Coverage 66.52% 66.50% -0.03%
==========================================
Files 1667 1667
Lines 64341 64340 -1
Branches 6495 6495
==========================================
- Hits 42805 42787 -18
- Misses 19853 19870 +17
Partials 1683 1683
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
The hope here is that people will use the **kwargs param in their mutator methods instead of hardcoding the params. If they do that, then any future changes will be backward compatible. If they continue to look up the kwargs and add them explicitly to their method, then their implementation will break with future changes to the args that are passed in. |
82335fe
to
389e9e1
Compare
389e9e1
to
3bc6de0
Compare
@@ -504,7 +504,7 @@ def test_sql_mutator(self): | |||
sql = tbl.get_query_str(query_obj) | |||
self.assertNotIn("-- COMMENT", sql) | |||
|
|||
def mutator(*args): | |||
def mutator(*args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we write a better test for making sure the mutator properly takes the params?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hughhhh I added one more test to check that the database name is being passed in. LMK if you think more tests around that would be helpful. user_name
was null and the security_manager
is a function so printing them wouldn't really validate much.
sql = sql_query_mutator(sql, user_name, security_manager, database) | ||
sql = sql_query_mutator( | ||
sql, | ||
user_name=user_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Qi, Is this assigning the existing user_name here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the current user_name, or rather identifier. Sometimes it doesn't map directly to the person's name depending on how they've authenticated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
This is a breaking change. To make the mutator more flexible for future changes, I am passing all but the required
sql
argument that is in the default config to kwargs. If we want to add more values to the mutator in the future we can without breaking any mutator config overwrite functions.TESTING INSTRUCTIONS
There is an existing test for the mutator.
ADDITIONAL INFORMATION