Wikimedia Bugzilla is closed!

Nothing should be using safeQuery() like that anyway.

Created attachment 11128 [details]
Patch for table creation in test case.

The attached patch solves the table creation bug (although the BLOB data type needs more thought), but the actual test then fails with:

| There was 1 error:

| 1) TestORMRowTest::testSave with data set #0 (array('Foobar', 42, 9000.1, true, array(13, 11, 7, 5, 3, 2), stdClass), true)
| DBQueryError: A database error has occurred.  Did you forget to run maintenance/update.php after upgrading?  See: https://www.mediawiki.org/wiki/Manual:Upgrading#Run_the_update_script
| Query: INSERT INTO "unittest_orm_test" (test_id,test_name,test_age,test_height,test_awesome,test_stuff,test_moarstuff) VALUES (NULL,'Foobar','42','9000.1',1,'a:6:{i:0;i:13;i:1;i:11;i:2;i:7;i:3;i:5;i:4;i:3;i:5;i:2;}','O:8:"stdClass":3:{s:3:"foo";s:3:"bar";s:3:"bar";a:2:{i:0;i:4;i:1;i:2;}s:3:"baz";b:1;}')
| Function: ORMRow::insert
| Error: 23502 FEHLER:  NULL-Wert in Spalte »test_id« verletzt Not-Null-Constraint

This is due to explicitly passing NULL as the value for the ID column which in MySQL means "use next value", but in PostgreSQL means "use NULL".  It can be worked around by passing the keyword "DEFAULT" (without quotes) as the value, e. g. "INSERT INTO t (c) VALUES (DEFAULT);".

Jeroen, what do you think is the best approach to support PostgreSQL here?  From a glance at the code, I would probably exclude ID columns in ORMRow::getWriteValues() when their value is null.

Marcin: thanks for pointing this out

Aaron: the ORMTable/Row code is not using safeQuery. I can in fact not find the method, so am guessing it got killed?

Tim Landscheidt: thanks for the patch. Will also try to look at the blob issue soonish.

(In reply to comment #3)
> [...]
> Tim Landscheidt: thanks for the patch. Will also try to look at the blob issue
> soonish.

Just to clarify: There isn't necessarily an "issue" with PostgreSQL's blob data type, it just has to be reviewed that BYTEA is an appropriate data type in this case as well as whether for example test_age's TINYINT unsigned can be translated to SMALLINT or should be translated to "SMALLINT CHECK(test_age >= 0)".  As this is "just" a test case with the emphasis on the ORM part, it's probably not that important, but I wanted to have pointed it out.

*** Bug 38930 has been marked as a duplicate of this bug. ***

Change 100180 had a related patch set uploaded by saper:
Database independent test for bug 37601

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

Reverting state to NEW:

Gerrit change Iebe9d6ff73fac930b456b24b6317cafb56d0163f is a test case to expose the failure in the database-independent way.

It seems to me that ORMRow::getWriteValues() had a test preventing "id"
fields and values from being ever written to the database (rightly so).

ece97c358424015777db3881d87aa33966d634bd has moved this responsibility from the row-level to the table level, but the test has been removed, therefore this bug.

I am not sure what is the proper place to fix this - to reintroduce the following lines from ORMRow into ORMTable

   227  // Skip null id fields so that the DBMS can set the default.
   228  if ( $name === 'id' && is_null( $value ) ) {
   229        continue;
   230  }

The test introduced in Iebe9d6ff73fac930b456b24b6317cafb56d0163f would be more elegant if there were some way to test getWriteValues() directly; maybe it even belongs to ORMTableTest instead but I could not figure out how to inject required IORMRow dependency into that test and lost too much energy trying.

Change 100183 had a related patch set uploaded by saper:
Teach ORMTable to use sequences

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

This is a very very early attempt to do something about primary keys.

With Gerrit change #100183 PostgreSQL and MySQL sucessfully passes tests/phpunit/includes/db/TestORMRowTest.phptests/phpunit/includes/db/TestORMRowTest.php in isolation, but fails when a whole test suite is run:


10) TestORMRowTest::testSaveAndRemove with data set #0 (array('Foobar', '2012-01-01 02:02:02 GMT', 42, 9000.1, true, array(13, 11, 7, 5, 3, 2), stdClass), true)
DBQueryError: A database error has occurred. Did you forget to run maintenance/update.php after upgrading?  See: https://www.mediawiki.org/wiki/Manual:Upgrading#Run_the_update_script
Query: SELECT nextval('orm_test_test_id_seq')
Function: DatabaseBase::query
Error: 42P01 ERROR:  relation "orm_test_test_id_seq" does not exist
LINE 1: ...ELECT /* DatabaseBase::query 127.0.0.1 */ nextval('orm_test_...
                                                             ^



/usr/home/saper/test/mytest/includes/db/Database.php:1111
/usr/home/saper/test/mytest/includes/db/DatabasePostgres.php:511
/usr/home/saper/test/mytest/includes/db/Database.php:1077
/usr/home/saper/test/mytest/includes/db/DatabasePostgres.php:991
/usr/home/saper/test/mytest/includes/db/ORMTable.php:1025
/usr/home/saper/test/mytest/includes/db/ORMRow.php:352
/usr/home/saper/test/mytest/tests/phpunit/includes/db/ORMRowTest.php:165
/usr/home/saper/test/mytest/tests/phpunit/MediaWikiTestCase.php:123
/usr/home/saper/test/mytest/tests/phpunit/MediaWikiPHPUnitCommand.php:80
/usr/home/saper/test/mytest/tests/phpunit/MediaWikiPHPUnitCommand.php:64
/usr/home/saper/test/mytest/tests/phpunit/phpunit.php:115

I am kind of at loss to figure out whether we are using mocked table and row instance but still somehow touch the database anyway; should we also mock sequences???  This is getting crazy.

Btw, shouldn't those tests be moved into @Database groups since the CREATE TABLEs and stuff?

I tend to believe that this patch fixes bug 43475 somehow, but tests are still broken.

Change 172662 had a related patch set uploaded by saper:
Mark TestORMRowTest as broken

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