Make owner a m2m relation on datasources #6544

leakingoxide · 2018-12-16T23:41:13Z

This is a PR for #6471.

I'm not sure about /save/ endpoint for datasources, because ideally I would just pass a list of ids in update_from_object. This, however, doesn't seem to work in sqlalchemy, and the only solution that I managed to get working, was retrieving a list of User objects from the database earlier. I'm open to suggestions how it could be done differently.

I'm also not quite sure about the migration, because every time I try to upgrade it alone on postgresql it hangs (probably #1260), and sqlite doesn't support all of the operations (removing the column). Which makes me unable to do proper testing for owner->owners data migration.

mistercrunch · 2018-12-17T00:46:44Z

superset/migrations/versions/3e1b21cd94a4_change_owner_to_m2m_relation_on_.py

+            )
+
+    session.close()
+    op.drop_constraint('user_id', 'tables', type_='foreignkey')


We've had issues in the past trying to drop constraints, I think other migrations have workarounds to catch edge cases, some of it may be reusable here. You can also wrap the brittle bits around a try block, where say leaving an unused column behind may be less trouble in those edge cases than dealing with this issue in SQLAlchemy.

mistercrunch · 2018-12-17T00:49:29Z

For some reason you may have to to use op.batch_alter_table to get around the SQLLite issue on the build.
It should look something like this:
https://github.com/apache/incubator-superset/blob/46c86672c8320c0003c2eb26f73db6daa2767996/superset/migrations/versions/956a063c52b3_adjusting_key_length.py#L17

mistercrunch · 2018-12-17T00:49:38Z

Otherwise LGTM

codecov-io · 2018-12-17T23:13:13Z

Codecov Report

Merging #6544 into master will increase coverage by <.01%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #6544      +/-   ##
==========================================
+ Coverage    73.5%   73.51%   +<.01%     
==========================================
  Files          72       72              
  Lines       10002    10009       +7     
==========================================
+ Hits         7352     7358       +6     
- Misses       2650     2651       +1

Impacted Files	Coverage Δ
superset/connectors/druid/views.py	`67.58% <ø> (ø)`	⬆️
superset/connectors/sqla/views.py	`63.39% <ø> (ø)`	⬆️
superset/connectors/druid/models.py	`82.37% <100%> (+0.04%)`	⬆️
superset/connectors/sqla/models.py	`77.92% <100%> (+0.08%)`	⬆️
superset/connectors/base/models.py	`91.37% <100%> (+0.04%)`	⬆️
superset/views/datasource.py	`92.85% <50%> (-3.3%)`	⬇️

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 5bac723...c67f347. Read the comment docs.

mistercrunch · 2018-12-18T00:47:51Z

Leaving open for a moment to see if @john-bodley or @timifasubaa have something to add.

* Make owner a m2m relation on datasources * Fix pylint * Make migration work in mysql & sqlite

* [SIP-3] Scheduled email reports for Slices / Dashboards (#5294) * [scheduled reports] Add support for scheduled reports * Scheduled email reports for slice and dashboard visualization (attachment or inline) * Scheduled email reports for slice data (CSV attachment on inline table) * Each schedule has a list of recipients (all of them can receive a single mail, or separate mails) * All outgoing mails can have a mandatory bcc - for audit purposes. * Each dashboard/slice can have multiple schedules. In addition, this PR also makes a few minor improvements to the celery infrastructure. * Create a common celery app * Added more celery annotations for the tasks * Introduced celery beat * Update docs about concurrency / pools * [scheduled reports] - Debug mode for scheduled emails * [scheduled reports] - Ability to send test mails * [scheduled reports] - Test email functionality - minor improvements * [scheduled reports] - Rebase with master. Minor fixes * [scheduled reports] - Add warning messages * [scheduled reports] - flake8 * [scheduled reports] - fix rebase * [scheduled reports] - fix rebase * [scheduled reports] - fix flake8 * [scheduled reports] Rebase in prep for merge * Fixed alembic tree after rebase * Updated requirements to latest version of packages (and tested) * Removed py2 stuff * [scheduled reports] - fix flake8 * [scheduled reports] - address review comments * [scheduled reports] - rebase with master * Typo: Fixed link (#6087) * Update contributing.md with latest local dev instructions (#6513) * Fix malformed table in docs/visualization.rst (#6409) * Update requests version (#6510) * Update requests version * Run pip-compile --output-file requirements.txt * [warm] Enforcing consistent form-data (#6531) * Avoid resetting margin to 0 (#6536) * Avoid resetting margin to 0 * Fixing margin for pie chart * Refactor teradata to new time_grain_functions spec (#6539) * Refactor teradata to new time_grain_functions spec * Add test for time_grain_functions * Add docker files to gitignore (#6507) * add docker files to gitignore * Update .gitignore * adding in a dependency version to fix an error with Flask CLI (#6547) * Fix string value displaying NaN (#6534) * Documentation Correction to use http.server for Python3 (#6549) * Remove note about snowflake-sqlalchemy stable version regression (#6398) * filter_values documentation fix (#5977) * Pass security manager to QUERY_LOGGER (#6548) * Pass security manager to log_query * Fix lint * Increase size of column `name` in table `ab_view_meu` (#6532) * Create migration script * Use batch operation for ALTER COLUMN * fix addr_str format bug (#6551) * Fix deck.gl Polygon not show (#6545) * Fix deck.gl Polygon not show * Replace "//" to "// " * Secure unsecured views and prevent regressions (#6553) * Secure views and prevent regressions * Force POST on shortner * Fix tests * [cosmetic] add css no-wrap on CRUD's last modified (#6522) * [RfC] Fix URL too long (#6519) When a URL gets too long (usually happens with controls that allow free form text), it creates an array of bugs. * bug when creating a short URL, because the POST request's referrer field is too long * a bug when saving a chart (when the modal shows up) also because of the referrer being too long Some work has been done in the past to mitigate this, and I'm unclear if it's some sort of regression because of the Flask upgrade or some browser change, or whether these bugs have always been around. This is a request for comment on an approach that works. Let me know if you can think of better ideas as to how to manage this. My current solution looks for 8000+ characters URLs and shrinks them to only `/superset/explore/?URL_IS_TOO_LONG_TO_SHARE&form_data=...` and we only keep the formData keys for `datasource` and `viz_type`. Not super elegant but does the trick. * minor, auto tune debug mode when use flask_env (#6550) * minor, auto tune debug mode when use flask_env * update changes * Make owner a m2m relation on datasources (#6544) * Make owner a m2m relation on datasources * Fix pylint * Make migration work in mysql & sqlite * Fix multiple db_migrations heads (#6560) * [SIP-14] remove dependency on yarn in favor of npm (#6541) * [SIP-14] remove dependency on yarn in favor of npm Find more details here #6217 * Add core-js to dev deps * lint * fix cypress * break down cypress_build.sh * fix cypress * Use 'npm ci' in place of 'npm install' * Bump npm req to >=6.5.0 * Enforcing npm@'>=6.5.0' * Using latest lts of nvm * Make cache settings global * remove uneeded 'npm run test' as 'npm run cover' takes care of that * Prefix with 'time' * Trying to upgrade babel-eslint instead of downgrading * upgrading babel-polyfill to '@babel/polyfill' * Moving to babel 7 * remove unused packages * Make boto3/botocore installation optional (#6540) * Make boto3 installation optional * pylinting * [SQL Lab] Allow running multiple statements (#6112) * Allow running multiple statements from SQL Lab * fix tests * More tests * merge heads * fix heads * Adding missing slash (#6567) * [refactor] moving some datasource-related code to the frontend (#5769) * [refactor] moving some datasource-related code to the frontend * fix js tests * fix tests * fix test * json_iso_dttm_ser use for TableViz (#6563) * fix #6530 and add some other chinese translation (#6569) * Fix: updated required cryptography version to 2.4.2 to resolve #6509 (#6516) * Fix: updated required cryptography version to 2.4.2 to resolve #6509 * Fix: updated cryptography version to 2.4.2 to resolve #6509 * Fix: updated cryptography version to 2.4.2 to resolve #6509 * Revert "Fix: updated required cryptography version to 2.4.2 to resolve #6509" This reverts commit b57b08c. * Fix: updated cryptography version to 2.4.2 * Fix multi/dual when no right y axis (#6571) * [design] use angle icons instead of carets for expandable panels (#6564) * [design] use angle icons instead of carets for expandable panels Also moving to the right to conform to the material design specs * Fix cypress test * fixing issue #6572 with Oracle date handling (#6580) * fix Oracle engine specs for dates issue #6572 * fix Oracle engine specs for dates issue #6572 * fix Oracle engine specs for dates issue #6572, removing comment * ng a trailing space * Change margin for slice description in charts for the dashboard view (#6575) Changed margin for the slices description in dashboard view, previously the margin was 5px for all the sides but for improve the view just apply margin for top and bottom. * [bugfix] moving from reactable to reactable-arc fork (#6576) * Using batch_op in db migration 0b1f1ab473c0 (#6581) As I needed to downgrade from db migration 0b1f1ab473c0, I realized I needed to use batch_op against SQLLite. * Finish move to babel 7 (#6573) * Finish move to babel 7 * Bump jest to 23.6.0 * Address comments * Enhance Docker (#6504) Refactored deprecated functionalities: - Used 'celery worker' command instead of 'superset worker' which is deprecated since 0.26.0 - Used 'gunicorn' command instead of 'superset runserver' * add chinese translate (#6592) * Fix bug: some word not translate in js. (#6598) delete the 'null,' in messages.json, so that word can be translate in js. * shift labels down along y-axis (#6596) * Fix 'Uncaught TypeError: Cannot read property 'value' of undefined' #6556 (#6574) closes #6556 * added get_data ; * added cache logic * added cache logic * added cache key logic * add missing part * revised the metric part * added to_dict * fix lint * fix lint * fix lint * fix lint * fix lint * fix flake8 * fix invalid name * fix invalid name * fix invalid name * enable worldcloid * working version * rebase * fixed expore link issue * add endpoint * added migration script WIP * fixed lint * remove migration script * fix flake8 * fix tesT * fix tesT * fix tesT * fix typo * fixed metric format * fixed metric format * fixed metric format * fixed tests * fixed test * fixed druid test * fixed more tests * fixed more tests * fix test for viz * fixed js tests * fixed ts test * fixed ts test * fixed cypress test * fixed the metric format in filterViz

* Make owner a m2m relation on datasources * Fix pylint * Make migration work in mysql & sqlite (cherry picked from commit fd03386)

* Make owner a m2m relation on datasources * Fix pylint * Make migration work in mysql & sqlite (cherry picked from commit fd03386) (cherry picked from commit 2e35990)

jdbertron · 2019-04-18T17:06:18Z

Same behavior on MySQL for this migration. The extra session remains in Sleep state, locking the information schema. The ALTER below is unable to obtain a table metadata lock until that session is killed.
INFO [alembic.runtime.migration] Running upgrade 6c7537a6004a -> cefabc8f7d38, Increase size of name column in ab_view_menu
INFO [sqlalchemy.engine.base.Engine] ALTER TABLE ab_view_menu MODIFY name VARCHAR(255) NOT NULL

mistercrunch · 2019-04-19T04:24:25Z

Kill locking transactions...

jdbertron · 2019-04-25T23:05:29Z

Nah, that's not a viable solution for a production environment. The issue is actually not with this migration, but cefabc8f7d38_increase_size_of_name_column_in_ab_view_.py. The problem is that connections created during the app creation in superset init, (with all the stuff it does to create roles) is never released and the connection holds a metadata lock on ab_view_menu. The cefab.. migration can't run the ALTER until the connection is killed manually. The simplest way I found to fix it is to run the AppBuilder creation within the app context. Then when the context goes out of scope the sessions are released to the pool correctly by the exit event handling, and the migration runs fine. I'll push the fix.

jdbertron · 2019-04-30T21:54:27Z

I pushed a fix. #7417

leakingoxide added 2 commits December 17, 2018 00:20

Make owner a m2m relation on datasources

86469eb

Fix pylint

2e24e16

mistercrunch reviewed Dec 17, 2018

View reviewed changes

mistercrunch added the risk:db-migration PRs that require a DB migration label Dec 17, 2018

Make migration work in mysql & sqlite

c67f347

mistercrunch merged commit fd03386 into apache:master Dec 21, 2018

conglei pushed a commit to conglei/incubator-superset that referenced this pull request Jan 7, 2019

Make owner a m2m relation on datasources (apache#6544)

025373b

* Make owner a m2m relation on datasources * Fix pylint * Make migration work in mysql & sqlite

xtinec pushed a commit to lyft/incubator-superset that referenced this pull request Jan 29, 2019

Make owner a m2m relation on datasources (apache#6544)

2e35990

* Make owner a m2m relation on datasources * Fix pylint * Make migration work in mysql & sqlite (cherry picked from commit fd03386)

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make owner a m2m relation on datasources #6544

Make owner a m2m relation on datasources #6544

leakingoxide commented Dec 16, 2018

mistercrunch Dec 17, 2018

mistercrunch commented Dec 17, 2018

mistercrunch commented Dec 17, 2018

codecov-io commented Dec 17, 2018 •

edited

Loading

mistercrunch commented Dec 18, 2018

jdbertron commented Apr 18, 2019

mistercrunch commented Apr 19, 2019

jdbertron commented Apr 25, 2019 •

edited

Loading

jdbertron commented Apr 30, 2019

Make owner a m2m relation on datasources #6544

Make owner a m2m relation on datasources #6544

Conversation

leakingoxide commented Dec 16, 2018

mistercrunch Dec 17, 2018

Choose a reason for hiding this comment

mistercrunch commented Dec 17, 2018

mistercrunch commented Dec 17, 2018

codecov-io commented Dec 17, 2018 • edited Loading

Codecov Report

mistercrunch commented Dec 18, 2018

jdbertron commented Apr 18, 2019

mistercrunch commented Apr 19, 2019

jdbertron commented Apr 25, 2019 • edited Loading

jdbertron commented Apr 30, 2019

codecov-io commented Dec 17, 2018 •

edited

Loading

jdbertron commented Apr 25, 2019 •

edited

Loading