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

Jump to bottom

chore: Updating log_this form-data logic #10197

Merged
merged 1 commit into from Jul 4, 2020
Merged

chore: Updating log_this form-data logic #10197

merged 1 commit into from Jul 4, 2020

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Jun 30, 2020 •
edited

SUMMARY

The log_this method logs the payload associated with the request however the existing logic doesn't accurately reflect how the form_data field is handled, i.e., it tries to fetch the payload from request.form (note I'm unsure why we're not using the application/json content type which uses request.json) and then override the payload with the request parameters. Note the impetus for this change was I was trying to see how the explore_json endpoint was being logged using dashboard filter overrides and realized that the json column only contained data of the form (which re-encoded the form-data as a JSON encoded string which seems wrong),

{"form_data": "{\"slice_id\":54}", "dashboard_id": "6"}

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 the get_form_data method (when relevant) which performs a similar merge on the JSON payload associated with the form_data key as opposed to the entire payload.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

CI, tested locally, and added unit test.

Before

{"form_data": "{\"slice_id\":54}", "dashboard_id": "6"}

After

{
    "form_data": {
        "datasource": "2__table",
        "viz_type": "treemap",
        "slice_id": 54,
        "url_params": {},
        "granularity_sqla": "year",
        "time_range": "1960-01-01 : now",
        "metrics": ["sum__SP_POP_TOTL"],
        "adhoc_filters": null,
        "groupby": ["region", "country_code"],
        "row_limit": 50000,
        "color_scheme": "bnbColors",
        "label_colors": {"0": "#ff5a5f", "1": "#7b0051", "2": "#007A87"},
        "treemap_ratio": 1.618033988749895,
        "number_format": "SMART_NUMBER",
        "queryFields": {"metrics": "metrics", "groupby": "groupby"},
        "extra_filters": [
            {"col": "region", "op": "in", "val": ["Latin America & Caribbean"]}
        ],
        "time_range_endpoints": ["unknown", "inclusive"],
    },
    "dashboard_id": "6",
}

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

john-bodley
john-bodley commented Jun 30, 2020
View reviewed changes
superset/utils/log.py
@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 {}
Copy link
Member Author

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.

superset/utils/log.py
@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
Copy link
Member Author

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.

bkyryliuk
bkyryliuk reviewed Jun 30, 2020
View reviewed changes
superset/utils/log.py
@@ -36,31 +36,32 @@ def log(
pass

def log_this(self, f: Callable[..., Any]) -> Callable[..., Any]:
from superset.views.core import get_form_data
Copy link
Member

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

Copy link
Member Author

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.

@john-bodley john-bodley force-pushed the john-bodley--log-this-form-data branch from e5e0505 to c98ba70 Compare July 1, 2020 04:40
graceguo-supercat
graceguo-supercat approved these changes Jul 2, 2020
View reviewed changes
Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LTGM!

@graceguo-supercat
Copy link

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.

bkyryliuk
bkyryliuk approved these changes Jul 2, 2020
View reviewed changes
tests/utils_tests.py
@@ -1316,3 +1316,30 @@ def test_get_form_data_globals(self) -> None:
)

self.assertEqual(slc, None)

def test_log_this(self) -> None:
Copy link
Member

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 :)

villebro
villebro approved these changes Jul 3, 2020
View reviewed changes
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM

@john-bodley john-bodley force-pushed the john-bodley--log-this-form-data branch from c98ba70 to bcaa487 Compare July 4, 2020 19:29
@john-bodley john-bodley merged commit b181e48 into apache:master Jul 4, 2020
@john-bodley john-bodley deleted the john-bodley--log-this-form-data branch July 4, 2020 19:51
@john-bodley john-bodley mentioned this pull request Aug 25, 2020
Merged
6 tasks
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@john-bodley @auxten
[log] Updating form-data logic (apache#10197)
b86898a 
Co-authored-by: John Bodley <john.bodley@airbnb.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkyryliuk bkyryliuk bkyryliuk approved these changes

@graceguo-supercat graceguo-supercat graceguo-supercat approved these changes

@villebro villebro villebro approved these changes

@etr2460 etr2460 Awaiting requested review from etr2460

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 0.37.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@john-bodley @graceguo-supercat @bkyryliuk @villebro @mistercrunch