Encode feature flags to JSON pessimistically#8529
Encode feature flags to JSON pessimistically#8529betodealmeida merged 3 commits intoapache:masterfrom
Conversation
|
Would it be worth wrapping this in a test? I can imagine additional feature flags being introduced that could break the functionality further. |
Good point, let me add a test. |
Codecov Report
@@ Coverage Diff @@
## master #8529 +/- ##
==========================================
+ Coverage 66.72% 66.81% +0.09%
==========================================
Files 449 449
Lines 22696 22701 +5
Branches 2366 2366
==========================================
+ Hits 15143 15167 +24
+ Misses 7415 7396 -19
Partials 138 138
Continue to review full report at Codecov.
|
| "superset/basic.html", | ||
| entry="sqllab", | ||
| bootstrap_data=json.dumps(d, default=utils.json_iso_dttm_ser), | ||
| bootstrap_data=json.dumps(d, default=utils.pessimistic_json_iso_dttm_ser), |
There was a problem hiding this comment.
Feels weird to be passing variant/loosely-typed data to this but I guess Superset does that a lot so I won't complain much other than to point it out. :-)
* Encode feature flags to JSON pessimistically * Add unit test * Remove old imports
mistercrunch
added
🏷️ bot
6 participants
CATEGORY
Choose one
SUMMARY
#8470 introduced a feature flag that is a function, for customizing the query cost estimate provided by different backend engines (currently supported only in Presto). The presence of a function in the feature flags breaks the JSON serialization of the payload, when sending it to the frontend.
I fixed it by using the pessimistic JSON encoder, which fallbacks to a string representation of the object when the serialization fails.
TEST PLAN
I hit the error when deploying a custom feature flag, not sure why it wasn't triggered in #8470. Fixed it and verified it now works.
ADDITIONAL INFORMATION
REVIEWERS
@khtruong