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

fix(sql-lab): recover from Cannot convert object to primitive value i… #19432

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

victorarbuesmallada
Copy link

…ssue

A Cannot convert object to primitive value error appears in the developer console when trying to format the data coming from Presto. This issue causes the SQL Lab to crash. This can be sorted by stringifying the row data.

SUMMARY

The SQL Lab crashes when data coming from Presto contains non primitive values. This fix basically catches the exception that occurs and recovers by formatting the row data using JSON.stringify. It does the trick in our case.
We also had a look at fixing this on the presto db engine spec but couldn't really fix it over there.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screenshot 2022-03-30 at 12 56 53

TESTING INSTRUCTIONS

I guess this is hard to test but, in our case, we have a Presto datasource that returns nested data that blows up the SQL Lab when it's getting formatted in a tabulated way.

ADDITIONAL INFORMATION

  • Has associated issue: PRESTO_EXPAND_DATA causes SQL Lab crash #11295
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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
  • Removes existing feature or API

…ssue

A `Cannot convert object to primitive value` error appears in the developer console when trying to format the data coming from Presto. This issue causes the SQL Lab to crash. This can be sorted by stringifying the row data.
@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (fa35109) 66.59% compared to head (a86d850) 65.27%.
Report is 3354 commits behind head on master.

Files Patch % Lines
superset-frontend/src/utils/common.js 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #19432      +/-   ##
==========================================
- Coverage   66.59%   65.27%   -1.32%     
==========================================
  Files        1670     2109     +439     
  Lines       63888    90076   +26188     
  Branches     6510    13355    +6845     
==========================================
+ Hits        42544    58801   +16257     
- Misses      19654    28165    +8511     
- Partials     1690     3110    +1420     
Flag Coverage Δ
javascript 55.85% <75.00%> (+4.52%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rusackas rusackas requested a review from villebro February 6, 2024 20:19
@rusackas
Copy link
Member

rusackas commented Feb 6, 2024

Hi there! Thanks for the contribution, and sorry it didn't get more attention. Is this still an issue in the current era of Superset? I've added another reviewer that (I believe) uses Trino to validate/assess. I'll also close/reopen this PR to retrigger CI, and perhaps we can get this across the finish line!

@rusackas rusackas closed this Feb 6, 2024
@rusackas rusackas reopened this Feb 6, 2024
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Can we add some tests, covering the issue this PR fixes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants