metabase / metabase

The simplest, fastest way to get business intelligence and analytics to everyone in your company :yum:
https://metabase.com
Other
38.77k stars 5.15k forks source link

POST `/api/mt/gtap` should throw an exception when related card does not have `result_metadata` #49671

Open kamilmielnik opened 1 week ago

kamilmielnik commented 1 week ago

Describe the bug

See: https://github.com/metabase/metabase-enterprise/issues/520#issuecomment-772528159


We have this test: https://github.com/metabase/metabase/blob/86ff88a/e2e/test/scenarios/permissions/sandboxes.cy.spec.js#L591-L606 The "normal" variant is an actual repro for https://github.com/metabase/metabase-enterprise/issues/520. This test is currently @quarantined because it fails. It fails because card.result_metadata of related card is null. It's null because the question has never been run.

There are 2 real-world use-cases where it can happen:

  1. admin creates an SQL question via UI without running it
  2. admin creates a question via API without running it - this is exactly what the test reproduces

So it means the problem (https://github.com/metabase/metabase-enterprise/issues/520) is still there. Sandboxing won't work correctly if the card's result_metadata is null.

Suggested solution: POST /api/mt/gtap should throw an exception when related card does not have result_metadata. Thanks to this, in 2 aforementioned cases we'd just inform the user that they need to run the question first before sandboxing it.

Once we do this, we should replace these tests. We'd probably need 2 new tests:

To Reproduce

  1. Run this test - the "normal" version

Information about your Metabase installation

master, 6bf6a5de38

Severity

P3

noahmoss commented 12 hours ago

@kamilmielnik FYI I've fixed the underlying issue on the backend in https://github.com/metabase/metabase/pull/50049 and re-enabled the Cypress test in question. So I think this can be closed once that merges. Let me know if you have any concerns.