Page MenuHomePhabricator

Upgrading OATHAuth extension broken on sqlite
Closed, ResolvedPublic

Description

Based on reports at Support_desk, the schema updater for OATHAuth is broken on sqlite.

Adding module field to table oathauth_users ...Wikimedia\Rdbms\DBQueryError from line 1699 of /var/www/site/html/includes/libs/rdbms/database/Database.php: Error 1: near ",": syntax error

Function: Wikimedia\Rdbms\Database::sourceFile( /var/www/site/html/extensions/OATHAuth/sql/mysql/patch-add_generic_fields.sql )

Query: ALTER TABLE oathauth_users

ADD module TEXT NOT NULL,

ADD data BLOB NULL

According to @Ammarpad SQLite requires that NOT NULL columns have a default value.

Given that this is a bundled extension, it should work on sqlite.

For reference: https://www.mediawiki.org/wiki/Topic:Vv3g3jilvdjtesr7

Event Timeline

Bawolff created this task.Oct 3 2020, 9:32 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 3 2020, 9:32 AM
Reedy renamed this task from OATHAuth extension broken on sqlite to Upgrading OATHAuth extension broken on sqlite.Oct 3 2020, 1:59 PM
Reedy added a subscriber: Reedy.EditedOct 3 2020, 2:10 PM

I'm a little confused by this... Obviously not saying it's wrong though.

MySQL schema

CREATE TABLE /*_*/oathauth_users (
	-- User ID
	id int not null primary key,

	-- Module user has selected
	module varchar(255) not null,

	-- Data
	data blob null

) /*$wgDBTableOptions*/;

Which is obviously transformed for sqlite... but it does work for creation

So why doesn't this? As other than being an ALTER TABLE it's the same column definitions... Or is SQLite more lenieant with CREATE TABLE?

ALTER TABLE /*_*/oathauth_users
	ADD module VARCHAR( 255 ) NOT NULL,
	ADD data BLOB NULL;

This is obviously transformed based on the error messages reported

From a dev wiki I have that was created earlier this year, but a while after these migrations were already added

sqlite> .schema oathauth_users
CREATE TABLE oathauth_users (
 id INTEGER not null primary key,
 module TEXT not null,
 data BLOB null
 );
sqlite>

Which seems to match the fact that ALTER TABLE is more strict/stringent about these things. Which is kinda stupid.

Reedy added a comment.EditedOct 3 2020, 2:19 PM

According to @Ammarpad SQLite requires that NOT NULL columns have a default value.

I don't think this is completely true.

https://www.sqlitetutorial.net/sqlite-not-null-constraint/

None of those create tables have defaults for the table.

I guess there is a difference when altering a table; sqlite needs to know what it needs to set for the default. MySQL presumably does something and decides on a default implicitly.

So Ammarpad's comment is correct for ALTER TABLE but not for CREATE TABLE. Which does make sense semantically... If a column can't be null, and the default is possibly null... What should it be? Computer says no.

I guess we should be able to make the mysql patch have a default empty string and sort this...

Ammarpad added a comment.EditedOct 3 2020, 2:27 PM

According to @Ammarpad SQLite requires that NOT NULL columns have a default value.

I don't think this is completely true.

I didn't say for all commands. It's for ALTER, that's the source of the error being discussed. My full comment on wiki has the context.

Change 631812 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/OATHAuth@master] Set a default when adding module column

https://gerrit.wikimedia.org/r/631812

[...]
Which seems to match the fact that ALTER TABLE is more strict/stringent about these things. Which is kinda stupid.

I don't think it is stupid, I feel it actually makes perfect sense. When you create a table, database constraints should make sure you never add a NULL value (hence, if a default value is not specified, insert queries would fail if the NOT NULL column w/o default isn't specified). However, if you run ALTER TABLE, there probably already are some rows. Once the column is added, all the previously-added rows need to have some value for the new-column too. Usually, DBMSes use NULL, but since the new column is NOT NULL, it needs to be something else. Since the DB cannot really guess for the user, it refuses to run ALTER TABLE at all.

What _is_ strange for me is how MySQL itself actually works, since it needs to add some default too. Maybe it adds an empty string by default?

Reedy added a comment.Oct 4 2020, 12:37 PM

Something can make sense and still be stupid. They’re not mutually exclusive.

At minimum, it’s inconsistent. But it also encourages potential schema drift. Look how much of a mess wmf production is in, and the work it’s causing for amir and the DBAs

If you do a MySQL style alter, you will have to have a default. Which you don’t need in the base schema. And in most cases you won’t change the base schema, because it’s unnecessary and you will always be inserting a row with a value in that column anyway. So you then lose the extra error handling that provides by it being not null without a default; usages have to provide a value

Of course, you can get around this by doing the temp table dance for the sqlite schema change and then just insert a value to work as the default. But still. Special handling as always

Reedy added a comment.Oct 4 2020, 1:29 PM

Oh, and patch-remove_module_specific_fields.sql wouldn't have worked for SQLite either. Fixed that too in my patch

Change 631822 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/OATHAuth@REL1_35] Make SQLite compatible patches

https://gerrit.wikimedia.org/r/631822

Change 631812 merged by jenkins-bot:
[mediawiki/extensions/OATHAuth@master] Make SQLite compatible patches

https://gerrit.wikimedia.org/r/631812

Change 631822 merged by jenkins-bot:
[mediawiki/extensions/OATHAuth@REL1_35] Make SQLite compatible patches

https://gerrit.wikimedia.org/r/631822

Urbanecm assigned this task to Reedy.Oct 4 2020, 9:10 PM
Reedy closed this task as Resolved.Oct 4 2020, 9:11 PM