1251309 - Use union instead of VIEW for ProductReleases table

Status: RESOLVED FIXED

(Release Engineering :: Applications: Shipit, defect)

Description

[Justin Wood (:Callek)]

In Bug 1168381, we added a new "View" for pagination, however in my local setup of shipit, adding a new release still leaves the table blank/empty.


https://github.com/mozilla/ship-it/pull/44 seems to be the culprit.


E.g. if I add an Exception at: https://github.com/mozilla/ship-it/blob/55f3982eb291d92ebd05ba3e17e701e2185a4cf4/kickoff/model.py#L255

And then use the flask console I can get:

>>> FirefoxRelease.query.all()
[<Release u'Firefox-28.0b6-build1'>, <Release u'Firefox-29.0b1-build1'>]
>>> table
<class 'kickoff.model.ProductReleasesView'>
>>> tables
(<class 'kickoff.model.ProductReleasesView'>)
>>> table.query.all()
[]


I'm doing this on my own fork/branch and using docker, so feel free to say "works for me" and help advise me on what I'm doing wrong. That said I don't see anything that writes to the new `Table` except the DB upgrade scripts.

My changes:
https://github.com/mozilla/ship-it/compare/master...Callek:mozharness
If you don't include docker stuff:
https://github.com/Callek/ship-it/compare/docker...Callek:mozharness

Comment 1

[Justin Wood (:Callek)]

Ex-Dev,

Is this something you feel you can fix?

At first glance looks like the issue is that the ORM creates a Table for the Releases, rather than a View.

class ProductReleasesView(Release, db.Model):
    __tablename__ = 'product_releases'

    product = db.Column(db.String(100))
    partials = db.Column(db.String(100))
    promptWaitTime = db.Column(db.Integer(), nullable=True)
    commRevision = db.Column(db.String(100))
    commRelbranch = db.Column(db.String(50))


While the upgrade .sql does a "CREATE VIEW"...

http://stackoverflow.com/questions/9766940/how-to-create-an-sql-view-with-sqlalchemy is a good doc along with https://bitbucket.org/zzzeek/sqlalchemy/wiki/UsageRecipes/Views

Comment 2

[Allan [:asilva]]

Hi, 
I don't found a way to prevent the table creation, due to this problem I created an upgrade script that drop the table if it exists.

Try run "migrate_repo" script.
The script that drop the table is: "010_drop_table_product_releases_if_exists_default_upgrade.sql"

Comment 3

[Justin Wood (:Callek)]

(In reply to Allan [:ex-dev] from comment #2)
> Hi, 
> I don't found a way to prevent the table creation, due to this problem I
> created an upgrade script that drop the table if it exists.
> 
> Try run "migrate_repo" script.
> The script that drop the table is:
> "010_drop_table_product_releases_if_exists_default_upgrade.sql"

I think this is a bad idea, overall... 

Given the issue with views and the difficulty in altering them after-the-fact (to add a new column I have to redo the entire sql for the creation to begin with) perhaps this could have been better done via a union?

(locally)?
mysql> (select 'thunderbird' as product, name from thunderbird_release) UNION ( select 'firefox' as product, name from firefox_release)
    -> ;
+---------+-----------------------+
| product | name                  |
+---------+-----------------------+
| firefox | Firefox-28.0b6-build1 |
| firefox | Firefox-29.0b1-build1 |
+---------+-----------------------+
2 rows in set (0.00 sec)

Comment 4

[Justin Wood (:Callek)]

(In reply to Justin Wood (:Callek) from comment #3)
> (In reply to Allan [:ex-dev] from comment #2)
> > Hi, 
> > I don't found a way to prevent the table creation, due to this problem I
> > created an upgrade script that drop the table if it exists.
> > 
> > Try run "migrate_repo" script.
> > The script that drop the table is:
> > "010_drop_table_product_releases_if_exists_default_upgrade.sql"
> 
> I think this is a bad idea, overall... 

To further explain my reasoning:

I'm of the mindset that an app, when started and the *model* in the app is created, it should yield the same resulting DB (give or take some column ordering) as the migration files would generate if ran in order for an already live app.

The migration files should also never need to be run on a newly setup version of the app, except to upgrade/downgrade to a different state than you started with. (e.g. I shouldn't need to run a db migrate script on first-run, only to backout to prior to your patch, or run for a newer version after one of my own patches).

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Justin Wood (:Callek) from comment #4)
> (In reply to Justin Wood (:Callek) from comment #3)
> > (In reply to Allan [:ex-dev] from comment #2)
> > > Hi, 
> > > I don't found a way to prevent the table creation, due to this problem I
> > > created an upgrade script that drop the table if it exists.
> > > 
> > > Try run "migrate_repo" script.
> > > The script that drop the table is:
> > > "010_drop_table_product_releases_if_exists_default_upgrade.sql"
> > 
> > I think this is a bad idea, overall... 
> 
> To further explain my reasoning:
> 
> I'm of the mindset that an app, when started and the *model* in the app is
> created, it should yield the same resulting DB (give or take some column
> ordering) as the migration files would generate if ran in order for an
> already live app.
> 
> The migration files should also never need to be run on a newly setup
> version of the app, except to upgrade/downgrade to a different state than
> you started with. (e.g. I shouldn't need to run a db migrate script on
> first-run, only to backout to prior to your patch, or run for a newer
> version after one of my own patches).

This is not how these apps are designed to work. To get a functioning database you _must_ run the migration scripts. If you don't you don't get a migrate_version table, and subsequent schema upgrades will fail.

For local development, whatever is creating your dev database should run the migration scripts.

Comment 6

[Allan [:asilva]]

In fact, I agree that it is not a good idea. I tried to find a way to unite all tables via ORM, but I have not found. The main problem was to merge fields that exists in table "A" but not exists in table "B" etc.
So, to solve performance issues, I created the view and in this view file I made the tables union.

For now I think that we can solve this performance issues and maintain the design problem. I will research this question.



_
Note: I use a translator to help-me write in english, I if this message is confusing, I can can reformulate the message.

Comment 7

[Justin Wood (:Callek)]

(In reply to Allan [:ex-dev] from comment #6)
> In fact, I agree that it is not a good idea. I tried to find a way to unite
> all tables via ORM, but I have not found. The main problem was to merge
> fields that exists in table "A" but not exists in table "B" etc.
> So, to solve performance issues, I created the view and in this view file I
> made the tables union.
> 
> For now I think that we can solve this performance issues and maintain the
> design problem. I will research this question.

I with this "fix the SQL performance first" logic. And ben tells me it *is* expected that migrate run *always* rather than just on explicit migrations. So i'm tweaking my docker code for that, and will write a migration for my own needs.

I'll leave this bug for the union design process, we can choose to wontfix that change on its own merits, but I no longer feel "regression" given my current knowledge.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Allan, do you want to take care of that? Thanks

Comment 9

[Allan [:asilva]]

I need research this yet. I use the UNION clausule in the View (https://github.com/mozilla/ship-it/blob/master/migrate_repo/versions/012_create_view_product_releases_default_upgrade.sql).
For while, I did not found a way to union tables on SqlAlchemy that meets the following requirements:
-Union table with unmatched fields (e.g. Thunderbird has commRevision column but firefox has not);
-Supports sorting and filtering;
-Supports pagination (offset and set limit).

I will go back to this question.

Thanks

Comment 10

[Allan [:asilva]]

Attachment: PR for review

Due to incompatibility between native SqlAlchemy and Flask-SqlAlchemy features, was necessary change the inheritance of model entities:
-A Base class was created to store @hybrid_properties;
-The Release class inherits from Base and AbstractConcreteBase(sqlalchemy) classes;
-All other classes inherits from Release;
-A polymorphic_union was used to union all releases table.

On my research, common union operation does not worked properly with filters, orderby and limit. Then the polymorphic_union function seems work fine with our scenario.

For some reason the associated use of AbstractConcreteBase and db.Model overrides some attributes and due to this the inheritance was changed.

Comment 11

[Rail Aliiev [:rail]]

Comment on attachment 8827170 [details] [review]
PR for review

Merged. I'll deploy this later this week

Comment 12

[Johan Lorenzo [:jlorenzo]]

Merged in master at https://github.com/mozilla-releng/ship-it/commit/282785f929304ecb80f83d93956d6c7df7642681

Code has been deployed but not the new database schema yet, AFAIK. Rail, I don't have the rights to connect to the machine, and it may take longer than I expected (bug 1333804). Do you mind finishing the deployment?

Comment 13

[Rail Aliiev [:rail]]

All done now!