Skip to content

[connectors] Make names non-nullable#7067

Closed
john-bodley wants to merge 1 commit into
apache:masterfrom
john-bodley:john-bodley--column-non-nullable
Closed

[connectors] Make names non-nullable#7067
john-bodley wants to merge 1 commit into
apache:masterfrom
john-bodley:john-bodley--column-non-nullable

Conversation

@john-bodley
Copy link
Copy Markdown
Member

@john-bodley john-bodley commented Mar 20, 2019

This PR ensures that the various database values cannot be NULL which surfaced as a result #5445 where it became apparent there could be ill-defined and/or erroneous database values. This is also reflected in the CRUD views which is handled automatically by Flask-AppBuilder. Specifically the following additional fields are now non-NULL:

Druid

  • Datasource: datasource_name
  • Column: column_name
  • Metric: metric_name, json

SQLAlchemy

  • Table: table_name
  • Column: column_name
  • Metric: name, expression

The attached screenshot provides an example of a form comprising of a required field (*) and form validation upon saving.

Screen Shot 2019-03-19 at 10 09 18 PM

Note this PR also includes a migration to update the schema to include the non-nullable fields. In order for this to happen however erroneous records need to be removed and ill-defined records need to be mutated.

Erroneous records are defined when:

  • A Druid or SQLAlchemy column name and column expression does not exist.
  • A Druid or SQLAlchemy metric name and metric expression does not exist.

Ill-defined records are defined when:

  • A Druid datasource or SQLAlchemy table name does not exist.
  • A Druid or SQLAlchemy column name does not exist however there is a column expression defined.
  • A Druid or SQLAlchemy metric name xor metric expression exists.

Given that the CRUD model is broken with NULL values the migration mutates the relevant fields to contain the migration_40f150716b13_ prefix. This allows the admin to then provide a cleanup pass if required to remove or update the violating records.

to: @graceguo-supercat @michellethomas @mistercrunch @xtinec

@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 20, 2019

Codecov Report

Merging #7067 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #7067      +/-   ##
=========================================
+ Coverage   64.48%   64.5%   +0.02%     
=========================================
  Files         421     421              
  Lines       20547   20559      +12     
  Branches     2250    2250              
=========================================
+ Hits        13249   13261      +12     
  Misses       7171    7171              
  Partials      127     127
Impacted Files Coverage Δ
superset/connectors/base/models.py 90.37% <100%> (ø) ⬆️
superset/connectors/druid/models.py 82.71% <100%> (ø) ⬆️
superset/connectors/druid/views.py 67.53% <100%> (+1.31%) ⬆️
superset/connectors/sqla/models.py 81.86% <100%> (ø) ⬆️
superset/connectors/sqla/views.py 65% <100%> (+1.84%) ⬆️

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 30f88ca...3250807. Read the comment docs.

Comment thread superset/connectors/sqla/views.py Outdated
Comment thread superset/connectors/sqla/views.py Outdated
Comment thread superset/connectors/sqla/views.py Outdated
@mistercrunch
Copy link
Copy Markdown
Member

mistercrunch commented Mar 20, 2019

Outside of typos LGTM

@john-bodley john-bodley force-pushed the john-bodley--column-non-nullable branch 2 times, most recently from 3250807 to 3d2e7e5 Compare March 20, 2019 17:06
@john-bodley
Copy link
Copy Markdown
Member Author

@mistercrunch apologies for the copy-and-paste typos. I realized that the edit_form_extra_fields is not required as Flask-AppBuilder handles this for non-nullable fields.

@mistercrunch
Copy link
Copy Markdown
Member

Sweet, well at least the typo made us discover that it wasn't necessary :)

LGTM

@john-bodley john-bodley changed the title [connectors] Make names non-nullable [wip][connectors] Make names non-nullable Mar 20, 2019
@john-bodley john-bodley force-pushed the john-bodley--column-non-nullable branch 5 times, most recently from 0056033 to 1c9ba3d Compare March 20, 2019 22:07
@john-bodley john-bodley changed the title [wip][connectors] Make names non-nullable [connectors] Make names non-nullable Mar 20, 2019
@john-bodley john-bodley force-pushed the john-bodley--column-non-nullable branch 4 times, most recently from 38ba9da to 2555b03 Compare March 20, 2019 22:56
@john-bodley john-bodley force-pushed the john-bodley--column-non-nullable branch from 2555b03 to 940519a Compare March 21, 2019 17:01
@john-bodley john-bodley force-pushed the john-bodley--column-non-nullable branch from 940519a to 452b216 Compare March 21, 2019 21:31
@john-bodley john-bodley force-pushed the john-bodley--column-non-nullable branch from 452b216 to b581b49 Compare March 22, 2019 00:06
@john-bodley
Copy link
Copy Markdown
Member Author

Closing this in favor of resurrecting (and augmenting) the following PRs:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants