-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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: Updating log_this form-data logic #10197
chore: Updating log_this form-data logic #10197
Conversation
@functools.wraps(f) | ||
def wrapper(*args: Any, **kwargs: Any) -> Any: | ||
user_id = None | ||
if g.user: | ||
user_id = g.user.get_id() | ||
form_data = request.form.to_dict() or {} | ||
payload = request.form.to_dict() or {} |
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.
The term payload
seems more accurate than form_data
.
@functools.wraps(f) | ||
def wrapper(*args: Any, **kwargs: Any) -> Any: | ||
user_id = None | ||
if g.user: | ||
user_id = g.user.get_id() | ||
form_data = request.form.to_dict() or {} | ||
payload = request.form.to_dict() or {} | ||
|
||
# request parameters can overwrite post body |
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.
I'm not sure if this comment (and the associated logic is relevant) as I'm unclear if this hold true only for payloads containing form-data
.
@@ -36,31 +36,32 @@ def log( | |||
pass | |||
|
|||
def log_this(self, f: Callable[..., Any]) -> Callable[..., Any]: | |||
from superset.views.core import get_form_data |
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.
it would be great to add a unit test here to prevent the regressions
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.
@bkyryliuk I've added a unit test per your suggestion. Note rather than mocking the log_this
wrapped function and callback opted to call the explore_json
endpoint and verified the logged database record.
e5e0505
to
c98ba70
Compare
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.
LTGM!
The old behavior is, only request parameters (parameters in the url) are logged. The new, desired behavior is both request parameters and post body should be logged. |
@@ -1316,3 +1316,30 @@ def test_get_form_data_globals(self) -> None: | |||
) | |||
|
|||
self.assertEqual(slc, None) | |||
|
|||
def test_log_this(self) -> None: |
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.
add a todo to add other test cases that cover log_this logic, could be fun onboarding task for someone :)
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
c98ba70
to
bcaa487
Compare
Co-authored-by: John Bodley <john.bodley@airbnb.com>
SUMMARY
The
log_this
method logs the payload associated with the request however the existing logic doesn't accurately reflect how theform_data
field is handled, i.e., it tries to fetch the payload fromrequest.form
(note I'm unsure why we're not using theapplication/json
content type which usesrequest.json
) and then override the payload with the request parameters. Note the impetus for this change was I was trying to see how theexplore_json
endpoint was being logged using dashboard filter overrides and realized that thejson
column only contained data of the form (which re-encoded the form-data as a JSON encoded string which seems wrong),i.e., what was parsed in the request parameters and lacked any of the body.
This approach seems incorrect when the form-data is encoded as a JSON object within the body or request parameters via the
form_data
key meaning the resulting payload is an override as opposed to merge. The solution here (and I believe the intent of the method) is to leverage theget_form_data
method (when relevant) which performs a similar merge on the JSON payload associated with theform_data
key as opposed to the entire payload.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
CI, tested locally, and added unit test.
Before
After
ADDITIONAL INFORMATION