refactor: extract json_required view decorator

[ad-m]

SUMMARY

During review #18151 there was a comment about code duplication of snippet like:

        if not request.is_json:
            raise InvalidPayloadFormatError("Request is not JSON")

Do we have a decorator for this? Might want to add one if not yet.

Originally posted by @ktmud in #18151 (comment)

I would like to get familiar with part of the codebase of the project written in Python, so I perceived that as a good first issue for a new contributor.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

Python test handle all scenarios. I added a new one integration tests to verify behaviour of extracted decorator.

ADDITIONAL INFORMATION

• Has associated issue: refactor: Moves the Explore form_data endpoint #18151
• Required feature flags: NO
• Changes UI: NO
• Includes DB Migration (follow approval process in SIP-59): NO
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API: NO
• Removes existing feature or API: NO

@ktmud, @michael-s-molina Can I request reviews? I am aware this is a trivial change, but I want to try to grab the project style & structure to be able to make bigger changes, and the beginning has to be somewhere.

[michael-s-molina]

Thank you for the PR @ad-m. Grabbing an example from Java, I really like the abstraction of @Produces and @Consumes annotations of JAX-RS. They accept the MIME type as an argument.

@Consumes("application/json")

Since request.is_json is pretty much checking the MIME type we could create a generic decorator the same way JAX-RS does. This will allow us to validate all request types in our endpoints.

It would also be a good idea to keep the MIME types as constants if Python/Flask does not provide them yet.

[ad-m]

I like to make it even more generic. I will apply you suggestion soon! śr., 26 sty 2022, 12:21 użytkownik Michael S. Molina < ***@***.***> napisał:

…

Thank you for the PR @ad-m <https://github.com/ad-m>. Grabbing an example from Java, I really like the abstraction of @produces and @consumes annotations of JAX-RS <https://docs.oracle.com/cd/E19798-01/821-1841/gipzh/index.html>. They accept the MIME type as an argument. @consumes("application/json") Since request.is_json <https://flask.palletsprojects.com/en/2.0.x/api/#flask.Request.is_json> is pretty much checking the MIME type we could create a generic decorator the same way JAX-RS does. This will allow us to validate all request types in our endpoints. — Reply to this email directly, view it on GitHub <#18170 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA3TNL3EOCQQLMKJOC56K4LUX7KLRANCNFSM5MZ3RLVA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[ktmud]

I'm not sure we need to make it generic. Which other request payload type we must handle?

[ktmud]

I'd recommend use a single decorator to keep things simple---so you don't have to import both the decorator and the constant. It may be more appropriate to name the function with a verb, i.e. requires_json instead of json_required (which sounds like a state).

[michael-s-molina]

I'm not sure we need to make it generic. Which other request payload type we must handle?

The */import endpoints use multipart/form_data as the MIME type for the body.

I'd recommend use a single decorator to keep things simple---so you don't have to import both the decorator and the constant. It may be more appropriate to name the function with a verb, i.e. requires_json instead of json_required (which sounds like a state).

While checking the API docs I found application/json and multipart/form_data as request MIME types. I also find application/text as the response MIME type for some endpoints. There doesn't seem to be much variation, so I'm fine with either approach.

[ktmud]

I think eventually we would want everything to be submitted via JSON, except maybe when uploading files, in which case you can just not use this decorator.

[codecov]

Codecov Report

Merging #18170 (ee92c54) into master (a4e93af) will increase coverage by 0.02%.
The diff coverage is 97.91%.

❗ Current head ee92c54 differs from pull request most recent head 00b2d09. Consider uploading reports for the commit 00b2d09 to get more accurate results

@@            Coverage Diff             @@
##           master   #18170      +/-   ##
==========================================
+ Coverage   66.05%   66.08%   +0.02%     
==========================================
  Files        1591     1591              
  Lines       62418    62413       -5     
  Branches     6286     6286              
==========================================
+ Hits        41228    41243      +15     
+ Misses      19568    19548      -20     
  Partials     1622     1622              

Flag	Coverage Δ
hive	52.29% <87.50%> (+0.11%)	⬆️
mysql	81.34% <97.91%> (+0.06%)	⬆️
postgres	81.39% <97.91%> (+0.06%)	⬆️
presto	52.13% <87.50%> (+0.11%)	⬆️
python	81.83% <97.91%> (+0.06%)	⬆️
sqlite	81.09% <97.91%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/views/base_api.py	97.89% <92.85%> (-0.33%)	⬇️
superset/annotation_layers/annotations/api.py	89.31% <100.00%> (+1.34%)	⬆️
superset/annotation_layers/api.py	86.32% <100.00%> (+1.45%)	⬆️
superset/charts/api.py	85.93% <100.00%> (+0.70%)	⬆️
superset/dashboards/api.py	92.54% <100.00%> (+0.65%)	⬆️
superset/dashboards/filter_sets/api.py	99.02% <100.00%> (+1.88%)	⬆️
superset/databases/api.py	93.99% <100.00%> (+0.96%)	⬆️
superset/datasets/api.py	91.89% <100.00%> (+0.86%)	⬆️
superset/explore/form_data/api.py	95.74% <100.00%> (+1.99%)	⬆️
superset/key_value/api.py	81.52% <100.00%> (+1.73%)	⬆️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4e93af...00b2d09. Read the comment docs.

[ad-m]

I renamed decorator to requires_json and created a separate decorator requires_form_data for multipart/form_data request.

I inspected code looking for other request formats and I notice that have superset.views.core.Superset.import_dashboards (endpoint /import_dashboard/) which uses two request formats depending on the request method - empty request (GET with HTML form in response) or form-data (POST with redirection on success response). However, I think we can handle this particular scenario in a special way.

I decided to use two decorators because I considered a small number of scenarios, I felt there was no reason to add more responsibility to one function (which increases complexity) since there are only two simple scenarios. The current approach is simple - readable and easy to use.

[michael-s-molina]

LGTM