fix: old unique constraint remnance #9882
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9882 +/- ##
=======================================
Coverage 71.24% 71.24%
=======================================
Files 584 584
Lines 30789 30789
Branches 3234 3238 +4
=======================================
+ Hits 21936 21937 +1
+ Misses 8744 8742 -2
- Partials 109 110 +1
Continue to review full report at Codecov.
|
mistercrunch
force-pushed
the
fix_old_unique_constraint
branch
from
c0b4cdc
to
5f51fc5
Compare
There was a problem hiding this comment.
I agree with the general direction here, but would prefer to hear @john-bodley 's comments prior to proceeding. In general I think we should squash all migrations for the 1.0 release and start from a clean slate, with proper constraint naming conventions and setting the bar much higher with up/down migration testing. Doing so should make the migrations much more robust, and should make it possible to use pretty much any proper SqlAlchemy supported RDBMS as a backend database going forward.
| __table_args__ = ( | ||
| UniqueConstraint( | ||
| "database_id", "schema", "table_name", name="_customer_location_uc", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Ping @john-bodley . I've been meaning to bring this up for discussion, as I also feel schema should be included in the constraint. However, I remember you had done work in this area before, and there was some reason why it wasn't possible to include schema in the unique constraint.
There was a problem hiding this comment.
I don't think this is a viable solution based on how different engine treat uniqueness for nullable columns (schema in this case). This is documented in #5449 which is an open PR from over two years ago but was updated last year to remedy the nullable issue. Though it's not perfect, i.e., ideally the constraint should reside within the database model I sense it's the only viable solution unless we special case the various engines.
There was a problem hiding this comment.
hey @john-bodley @villebro , sorry to bring this back, but how about we just get rid of the constraint altogether? You want 2 datasets pointing to the same table, why not?
At least we should alter the old migration that creates the old/bad constraint we then cannot delete successfuly
There was a problem hiding this comment.
Having multiple Superset datasources with the same schema and table name for a given database does not sense to me as it is not easily apparent from the current UI which datasource is being referenced as the name if the primary indicator throughout the application.
|
@villebro regarding the the naming conventions I agree we should explicitly define constraints (per the naming conventions) when we define the models to ensure consistency. Note there are a number of migrations (example) which leverage this logic. Note we also have utility functions ( |
|
Question: Will this PR fix bugreport: #8871? |
|
@rvanaalderen: maybe. Though one solution is to just go and DROP the bad constraint in your database. It's a one time fix to any environment. Of course we should fix it so people don't run into it anymore, but as highlighted here, it's not trivial |
|
@mistercrunch Hi there, what do you mean by "manually delete the constraint using DML against your database"? How am I going to do it in superset? |
SUMMARY
closes #8871
When constraints names aren't defined in sqlalchemy/alembic, they are non-deterministic, and it makes it hard/impossible to delete them in subsequent migrations. Here a bad constraint was created originally without a name, and some other migration tries to re-create it, and may fail on a subset of database engines. Also I think sqlite doesn't support constraint making the whole thing more of a headache.
I decided to go back to the first migration and prevent the constraint to be created in the first place. Try to delete the constraint in the subsequent migrations, and finally create the appropriate one.
TODO: We should squash [at least a subset of] migrations eventually.