New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for #791 & refactor and fix bugs in dbschemasetup #930

Merged
merged 3 commits into from Sep 20, 2018

Conversation

Projects
None yet
3 participants
@takkaria
Copy link
Contributor

takkaria commented Jul 21, 2018

This pull request refactors dbschemasetup.php to remove code duplication and logic inconsistencies, fixes several bugs, and adds documentation. I started doing this when trying to set up emoncms master, I bumped into the issue with the user table's tags column being set to NOT NULL, and I refactored to try and work out what was going on. This is the result!

There are three cases when this file creates or updates a column in the database:

  1. when adding a new table
  2. when adding a new column to an existing table, and
  3. when altering an existing column

All these cases currently use different logic, resulting in inconsistencies. For a column that has 'null'=false, in cases 1 and 3 it is not given a NOT NULL constraint, but in case 2 it is. If 'null'=true, however, in cases 1 and 2 it doesn't get a NOT NULL constraint, but in case 3 it is. I've refactored so that all three cases use the same code to decide what a column should look like in the database.

There was also a bug in case 3, where only the changes to the column specification would be written on update. So if you had a column, say mothers_surname and it had a default value, if you then later updated it to be NOT NULL without respecifying the default value, the default would be lost, like so:

> ALTER TABLE users ADD mothers_surname text DEFAULT 'Smith';
> ALTER TABLE users MODIFY mothers_surname text NOT NULL;
> DESCRIBE users mothers_surname;
+-----------------+------+------+-----+---------+-------+
| column          | Type | Null | Key | Default | Extra |
+-----------------+------+------+-----+---------+-------+
| mothers_surname | text | NO   |     | NULL    |       |
+-----------------+------+------+-----+---------+-------+

This patch avoid this by always respecifying the full datatype when modifying a column.

Columns that had an explicit default of 0 or '' weren't getting this set in the database in case 1 or case 3. PHP's loose equality operator has probably covered this up until now.

NOT NULL wasn't being set on most columns that requested it in case 1 or case 3.

The new index creation code didn't trigger in case 2.

And finally, only case 1 reported errors. All cases now report errors.

Future suggestions

  • use consistent case for array keys in the column specification (at the moment there is 'Null' and 'Key' but also 'type' and 'default')
  • rename 'Extra' field specifier to 'autoincrement' since that is all it does in practice
  • only allow null=true or null=false, don't allow null="no"
  • splitting db_schema_setup() into two: maybe db_schema_get_statements and db_schema_apply_statements instead of having an $apply argument. This way, errors can be returned from the latter without mixing them up with the return of SQL statements.

I'm happy to do all the above, but didn't because I presume there are compatibility issues with changing schema definitions if people have custom modules, and I didn't want to touch code outside of this file if I could help it.

I wrote some test cases while working on the code. I didn't see any unit test or integration test infrastructure but I thought I'd include them anyway in case this is something the project adopts in the future.

Any feedback welcome, I realise I'm a new contributor here :)

@takkaria takkaria changed the title Refactor & fix bugs in dbschemasetup fix for #791 & refactor and fix bugs in dbschemasetup Jul 26, 2018

@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Jul 26, 2018

Thanks @takkaria I will study your pull request in detail and hopefully get back to you soon

@TrystanLea TrystanLea merged commit 3657457 into emoncms:master Sep 20, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Sep 20, 2018

Thankyou @takkaria for this and sorry for the delay in merging, I havent fully had a chance to understand the code in detail but testing here it works well, it certainly all looks much more comprehensive than what I had in there so thanks a lot for your work on this.

I note it caught quite a few incorrectly set schema entries for text datatypes where a default was set. I've removed these from emoncms core modules: user, schedule and input.

Your future suggestions all sounds like good improvements and I would be happy to merge if you decide to work on those.

@danbates2

This comment has been minimized.

Copy link

danbates2 commented Sep 20, 2018

Hi both, I've deleted my above comment in light of testing the master branch this morning.
The new dbschemasetup.php looks really good.

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