fix(sqllab): save datasets with template parameters

[ethan-l-geotab]

SUMMARY

ENABLE_TEMPLATE_PROCESSING=true

Looking to solve this: #25786.
Trying to allow for datasets with stuff in the parameters to save without having a fatal error.
All it does is add the template parameters at the creation (post) of the dataset

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

TAKEN FROM THE ISSUE:

1. Go to the SQL lab and open a new query, select the default examples database
2. Click on the three-dots menu and Parameters.
3. Enter the following JSON to specify defaults values for Jinja templates (relevant doc )
   {
   "my_table": "persons"
   }
4. Enter the following text as the query
   SELECT * FROM {{ my_table }}
   Observe that you can execute the query, the context you specified in the Parameters is correctly injected because the results appear as expected.
5. Save the query as a dataset and it should save correctly instead of giving fatal error.

ADDITIONAL INFORMATION

• Has associated issue: Cannot save a query with Parameters as a Dataset (Jinja template default values) #25786
• 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

[korbit-ai]

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

[github-actions]

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

[korbit-ai]

I've completed my review and didn't find any issues.

Files scanned

​

File Path	Reviewed
superset-frontend/src/utils/datasourceUtils.js	✅
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx	✅
superset/datasets/schemas.py	✅
superset-frontend/src/SqlLab/actions/sqlLab.js	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.03%. Comparing base (76d897e) to head (4975366).
Report is 1918 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #33195       +/-   ##
===========================================
+ Coverage   60.48%   83.03%   +22.55%     
===========================================
  Files        1931      555     -1376     
  Lines       76236    40776    -35460     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    33860    -12254     
+ Misses      28017     6916    -21101     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	47.62% <100.00%> (-1.53%)	⬇️
javascript	?
mysql	74.09% <100.00%> (?)
postgres	74.15% <100.00%> (?)
presto	51.99% <100.00%> (-1.82%)	⬇️
python	83.03% <100.00%> (+19.53%)	⬆️
sqlite	73.65% <100.00%> (?)
unit	61.60% <100.00%> (+3.97%)	⬆️

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Vitor-Avila]

Should this be behind a checkbox in the save dataset modal/dialog? I'm thinking if users setting a _filters config just to emulate dashboard filters might not want these to persist to the dataset 🤔 a checkbox that's disabled by default (to match previous behavior) and can be enabled if desired could help maintaining flexibility.

[rusackas]

CC @Vitor-Avila as well. I need to ramp up on my jinja intricacies!

[Vitor-Avila]

Hey @ethan-l-geotab,
This is awesome! Thank you so much for this change.

Could we make this behind a checkbox (or toggle) in the save dataset dialog? Something like "Include parameters". Not sure if there are users that do not want to persist these settings, but having it configureable would make sure we can support both flows.

[ethan-l-geotab]

Sure! Just to double check.

1. Currently it only sets the template params if there is _filters in it.
   ie: if template_parameters has p._filters, delete p._filters and save everything else.
   The PR in it's current state, saves everything EXCEPT for the p._filters every time.
2. I will make a checkbox that if checked, will save the rest of the template params regardless of if there is p._filters.

My question is, if the template params includes p._filters and the checkbox is NOT checked, do we want to still save the template params?
If the checkbox IS checked, we still want to remove the p._filters, right?

[Vitor-Avila]

Hey @ethan-l-geotab, these would be my cents -- let me know if you think these don't make sense.

• p._filters should always be excluded -- these are typically declared to simulate values for {{ filter_values() }} macros in SQL Lab. To build a robust query, my understanding is that users should rely on if/else to have default values when there isn't a filter (or even pass a default value to the macro).
• Any other parameter should always persist if the checkbox is enabled. For example, if I declare foo as a param in SQL Lab, and I have the checkbox checked foo should persist to the dataset regardless if I have p._filters or not. On the other hand, they should never sync if the checkbox is unchecked.

Let me know if you have any questions!

[ethan-l-geotab]

Thanks for the answers, @Vitor-Avila
I made something like this:

Just a quick summary:
If checked, delete filters and and keep the rest of the parameters.
If not checked, skip templateParams all together.

[Vitor-Avila]

that's awesome, @ethan-l-geotab! Thank you so much 🙏

Do you think it would be possible to create some tests, perhaps in SaveDatasetModal.test.tsx? It would be great if we could test the element is rendered properly, and the payload sent based on the checkbox state.

[ethan-l-geotab]

@Vitor-Avila. I added tests in SaveDatasetModal.test.tsx!

1. to check the render of the checkbox (only render if the flag is enabled)
2. to check if "createDatasource" has been called with template parameters based on the checkbox status

[github-actions]

@Vitor-Avila Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

[github-actions]

@Vitor-Avila Ephemeral environment spinning up at http://34.212.143.108:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

[Vitor-Avila]

ooof forgot to enable the FF in the ephemeral -- will have to redo that

[github-actions]

@Vitor-Avila Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

[github-actions]

@Vitor-Avila Ephemeral environment spinning up at http://35.165.209.101:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

[Vitor-Avila]

Couldn't make the FF work so I tested locally -- thank you so much for this feature!

[Vitor-Avila]

LGTM!