Skip to content
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

Inttest adding tests using big fileids #28364

Merged
merged 5 commits into from
Aug 3, 2017

Conversation

SergioBertolinSG
Copy link
Contributor

@SergioBertolinSG SergioBertolinSG commented Jul 11, 2017

Altering most of relevant to file_id columns to be BIGINT.
Ref #26901

With integration tests.

Not sure if I am in the right path, @PVince81 what do you think?

@SergioBertolinSG
Copy link
Contributor Author

SergioBertolinSG commented Jul 11, 2017

After using this app and doing

curl -X POST http://admin:[email protected]/html/multilinkshares/ocs/v1.php/apps/testing/api/v1/increasefileid

Checking oc_filecache table in the database I see the entry correctly added. But when accessing the server there is an exception:

"reqId":"G38RbN69ys8hW1Kqptl7","level":3,"time":"2017-07-11T10:58:55+00:00","remoteAddr":"10.40.40.134","user":"admin","app":"PHP","method":"POST","url":"\/html\/multilinkshares\/ocs\/v1.php\/apps\/testing\/api\/v1\/increasefileid","message":"Error: Call to a member function succeeded() on null at \/var\/www\/html\/multilinkshares\/lib\/private\/legacy\/api.php#214"}
{"reqId":"xinaIyhR1BW9s0u1R4CJ","level":3,"time":"2017-07-11T10:59:27+00:00","remoteAddr":"10.40.40.134","user":"admin","app":"index","method":"GET","url":"\/html\/multilinkshares\/index.php\/apps\/files\/","message":"Exception: {\"Exception\":\"Doctrine\\\\DBAL\\\\Exception\\\\UniqueConstraintViolationException\",\"Message\":\"An exception occurred while executing 'INSERT INTO `oc_filecache` (`mimepart`,`mimetype`,`mtime`,`size`,`etag`,`storage_mtime`,`permissions`,`checksum`,`path_hash`,`path`,`parent`,`name`,`storage`) SELECT ?,?,?,?,?,?,?,?,?,?,?,?,? FROM `oc_filecache` WHERE `storage` = ? AND `path_hash` = ? HAVING COUNT(*) = 0' with params [\\\"1\\\", \\\"2\\\", 1499770764, -1, \\\"5964af8f94471\\\", 1499770764, 23, \\\"\\\", \\\"d41d8cd98f00b204e9800998ecf8427e\\\", \\\"\\\", -1, \\\"\\\", 2, 2, \\\"d41d8cd98f00b204e9800998ecf8427e\\\"]:\\n\\nSQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '2147483647' for key 'PRIMARY'\",\"Code\":0,\"Trace\":\"#0 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/composer\\\/doctrine\\\/dbal\\\/lib\\\/Doctrine\\\/DBAL\\\/DBALException.php(128): Doctrine\\\\DBAL\\\\Driver\\\\AbstractMySQLDriver->convertException('An exception oc...', Object(Doctrine\\\\DBAL\\\\Driver\\\\PDOException))\\n#1 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/composer\\\/doctrine\\\/dbal\\\/lib\\\/Doctrine\\\/DBAL\\\/Connection.php(1015): Doctrine\\\\DBAL\\\\DBALException::driverExceptionDuringQuery(Object(Doctrine\\\\DBAL\\\\Driver\\\\PDOMySql\\\\Driver), Object(Doctrine\\\\DBAL\\\\Driver\\\\PDOException), 'INSERT INTO `oc...', Array)\\n#2 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/DB\\\/Connection.php(211): Doctrine\\\\DBAL\\\\Connection->executeUpdate('INSERT INTO `oc...', Array, Array)\\n#3 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/DB\\\/Adapter.php(113): OC\\\\DB\\\\Connection->executeUpdate('INSERT INTO `oc...', Array)\\n#4 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/DB\\\/Connection.php(249): OC\\\\DB\\\\Adapter->insertIfNotExist('*PREFIX*filecac...', Array, Array)\\n#5 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/Files\\\/Cache\\\/Cache.php(265): OC\\\\DB\\\\Connection->insertIfNotExist('*PREFIX*filecac...', Array, Array)\\n#6 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/Files\\\/Cache\\\/Cache.php(222): OC\\\\Files\\\\Cache\\\\Cache->insert('', Array)\\n#7 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/Files\\\/Cache\\\/Scanner.php(264): OC\\\\Files\\\\Cache\\\\Cache->put('', Array)\\n#8 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/Files\\\/Cache\\\/Scanner.php(204): OC\\\\Files\\\\Cache\\\\Scanner->addToCache('', Array, -1)\\n#9 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/Files\\\/Cache\\\/Scanner.php(168): OC\\\\Files\\\\Cache\\\\Scanner->scanFile('')\\n#10 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/Files\\\/Cache\\\/Scanner.php(307): OC\\\\Files\\\\Cache\\\\Scanner->scanFile('avatars', 3, -1, NULL, true)\\n#11 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/Files\\\/View.php(1302): OC\\\\Files\\\\Cache\\\\Scanner->scan('avatars', false)\\n#12 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/Files\\\/View.php(1343): OC\\\\Files\\\\View->getCacheEntry(Object(OCA\\\\Files_Trashbin\\\\Storage), 'avatars', '\\\/avatars')\\n#13 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/Files\\\/Node\\\/Root.php(182): OC\\\\Files\\\\View->getFileInfo('\\\/avatars')\\n#14 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/AvatarManager.php(97): OC\\\\Files\\\\Node\\\\Root->get('\\\/avatars')\\n#15 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/AvatarManager.php(117): OC\\\\AvatarManager->getFolder(Object(OC\\\\Files\\\\Node\\\\Root), 'avatars')\\n#16 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/AvatarManager.php(91): OC\\\\AvatarManager->getAvatarFolder(Object(OC\\\\User\\\\User))\\n#17 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/TemplateLayout.php(101): OC\\\\AvatarManager->getAvatar('admin')\\n#18 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/legacy\\\/template.php(232): OC\\\\TemplateLayout->__construct('user', 'files')\\n#19 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/public\\\/AppFramework\\\/Http\\\/TemplateResponse.php(156): OC_Template->fetchPage()\\n#20 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/AppFramework\\\/Http\\\/Dispatcher.php(112): OCP\\\\AppFramework\\\\Http\\\\TemplateResponse->render()\\n#21 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/AppFramework\\\/App.php(98): OC\\\\AppFramework\\\\Http\\\\Dispatcher->dispatch(Object(OCA\\\\Files\\\\Controller\\\\ViewController), 'index')\\n#22 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/AppFramework\\\/Routing\\\/RouteActionHandler.php(46): OC\\\\AppFramework\\\\App::main('ViewController', 'index', Object(OC\\\\AppFramework\\\\DependencyInjection\\\\DIContainer), Array)\\n#23 [internal function]: OC\\\\AppFramework\\\\Routing\\\\RouteActionHandler->__invoke(Array)\\n#24 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/private\\\/Route\\\/Router.php(299): call_user_func(Object(OC\\\\AppFramework\\\\Routing\\\\RouteActionHandler), Array)\\n#25 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/base.php(920): OC\\\\Route\\\\Router->match('\\\/apps\\\/files\\\/')\\n#26 \\\/var\\\/www\\\/html\\\/multilinkshares\\\/index.php(49): OC::handleRequest()\\n#27 {main}\",\"File\":\"\\\/var\\\/www\\\/html\\\/multilinkshares\\\/lib\\\/composer\\\/doctrine\\\/dbal\\\/lib\\\/Doctrine\\\/DBAL\\\/Driver\\\/AbstractMySQLDriver.php\",\"Line\":66}"}

@SergioBertolinSG
Copy link
Contributor Author

OK, it is ready.

It increases the fileid in the before suite statement only if it is not present already.

@VicDeo This PR should wait for your fix. You could do it here or I'll rebase it after it is merged, as you wish.

@VicDeo VicDeo mentioned this pull request Jul 12, 2017
9 tasks
@VicDeo
Copy link
Member

VicDeo commented Jul 12, 2017

So...

Doctrine\DBAL\Platforms\PostgreSqlPlatform:

    /**
     * {@inheritDoc}
     */
    public function getBigIntTypeDeclarationSQL(array $field)
    {
        if ( ! empty($field['autoincrement'])) {
            return 'BIGSERIAL';
        }

        return 'BIGINT';
    }

If a column has an autoincrement this will produce the query
ALTER TABLE tablename ALTER id TYPE BIGSERIAL

and postgre will die: type "bigserial" does not exist error on postgres. Because bigserial is not a real type.
I'm stuck :(

@VicDeo VicDeo force-pushed the inttest-adding-tests-using-big-fileids branch from 1f4b513 to bf00edd Compare July 12, 2017 20:42
@PVince81
Copy link
Contributor

@VicDeo are you saying that postgres has no support for bigint ??

@VicDeo
Copy link
Member

VicDeo commented Jul 13, 2017

@PVince81 no, I say that design of doctrine/dbal leaves no room for a clean solution.

  1. bigserial is a metatype. it can't be used in ALTER blabla queries.
  2. getBigIntTypeDeclarationSQL is not aware of a query context. It always returns bigserial for bigints with autoincrement
  3. 1+2 = schemaDiff produces broken SQL for Postrge

To prevent this I can skip this migration step for this field on postgre and write plain SQL migration with a direct ALTER query

UPD.
So the complete story. For the following migration script

$table = $toSchema->getTable("oc_cards");

$idColumn = $table->getColumn('id');
if ($idColumn){
$idColumn->setType(Type::getType(Type::BIGINT));
}

PostgreSqlPlatform::getAlterTableSQL will produce the following two queries:

ALTER TABLE oc_cards ALTER id TYPE BIGSERIAL
ALTER TABLE oc_cards ALTER id DROP DEFAULT
  1. The first query is wrong due to BIGSERIAL
  2. The second query wipes autoincrement assigned to the field so any INSERT query will fail in future.

To prevent 2. migration should have a new default that is constructed according to Postgres rules:

$table = $toSchema->getTable("oc_cards");

$idColumn = $table->getColumn('id');
if ($idColumn){
	$idColumn->setType(Type::getType(Type::BIGINT));
	// Fixup postgres autoincrement
	if ($this->connection->getDatabasePlatform() instanceof PostgreSqlPlatform){
		$default = sprintf("nextval('%s_%s_seq'::regclass)", $table->getName(), $idColumn->getName());
		$idColumn->setDefault($default);
	}
}

@PVince81
Copy link
Contributor

@DeepDiver1975 I'd like your input on this #28364 (comment)

We could also try to submit a PR upstream to Doctrine/dbal...

@VicDeo VicDeo force-pushed the inttest-adding-tests-using-big-fileids branch 5 times, most recently from 2787ecc to c0dd51a Compare July 14, 2017 23:08
@tomneedham
Copy link
Contributor

See #28429

@VicDeo
Copy link
Member

VicDeo commented Jul 19, 2017

hmm. there is no crash locally

@VicDeo VicDeo force-pushed the inttest-adding-tests-using-big-fileids branch from 22a7268 to ce6736a Compare July 20, 2017 19:19
@VicDeo
Copy link
Member

VicDeo commented Jul 21, 2017

@PVince81 @DeepDiver1975 All tests are green.

@VicDeo VicDeo changed the title [WIP] Inttest adding tests using big fileids Inttest adding tests using big fileids Jul 21, 2017
@@ -13,7 +13,7 @@
<default>0</default>
<notnull>true</notnull>
<autoincrement>1</autoincrement>
<length>4</length>
<length>10</length>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I thought @phisch got rid of these files when he converted 8.2 stuff to migrations. But maybe these were older schemas that didn't need migrating

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about - for the sake of clearness - to kill all database.xml files and add them as migrations ....

// BIGSERIAL could not be used in statements altering column type
// That's why we replace it with BIGINT
// see https://github.com/owncloud/core/pull/28364#issuecomment-315006853
if (preg_match('|(ALTER TABLE\s+\S+\s+ALTER\s+\S+\s+TYPE\s+)(BIGSERIAL)|i', $sql, $matches)){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you try doing this earlier, maybe within the schema diff ? or is the word "BIGSERIAL" generated very late ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PVince81 sqls are generated in line 68 $sqls = $schemaDiff->toSql($connection->getDatabasePlatform());

// see https://github.com/owncloud/core/pull/28364#issuecomment-315006853
if (preg_match('|(ALTER TABLE\s+\S+\s+ALTER\s+\S+\s+TYPE\s+)(BIGSERIAL)|i', $sql, $matches)){
$alterTable = $matches[1];
$sql = $alterTable . 'BIGINT';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strange, you just append it to the end ? wouldn't BIGSERIAL appear in the middle of a statement sometimes ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PVince81 Pure regexp magic. I capture everything into $matches
(something)(anotherthing)
when the input string is somethinganotherthing
$matches[0] will be somethinganotherthing
$matches[1] will be something
$matches[2] will be anotherthing

in this case
$matches[1] is ALTER TABLE whatever ALTER somecolumn TYPE
and
$matches[2] is BIGSERIAL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if there is more than one column in a single ALTER statement, or does Doctrine always split these ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


// Fixup postgres autoincrement
if ($this->connection->getDatabasePlatform() instanceof PostgreSqlPlatform){
$default = sprintf("nextval('%s_%s_seq'::regclass)", $table->getName(), $idColumn->getName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sad if we can't do this inside of PostgreSqlPlatform. Will we need this statement in more places ? Worst case, put it as a utility function in PostgreSqlPlatform then for reusability ?

@PVince81
Copy link
Contributor

Ok, I just saw your comment that you already looked into some way to do it in Doctrine. Since it's not possible, let's see if we can make parts of this code reusable, especially the logic for setDefault. I expect we'll need it in subsequent PRs when addressing more ids.

@VicDeo VicDeo force-pushed the inttest-adding-tests-using-big-fileids branch from ce6736a to d854ce6 Compare July 22, 2017 00:27
@@ -129,6 +129,10 @@ public function getConnection($type, $additionalConnectionParams) {
$additionalConnectionParams['platform'] = new OCSqlitePlatform();
$eventManager->addEventSubscriber(new SQLiteSessionInit(true, $journalMode));
break;
case 'pgsql':
case 'postgresql':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postgresql? we do support this key?

use Doctrine\DBAL\Schema\TableDiff;
use Doctrine\DBAL\Types\Type;

class OCPostgreSqlPlatform extends \Doctrine\DBAL\Platforms\PostgreSqlPlatform {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use ...


class OCPostgreSqlPlatform extends \Doctrine\DBAL\Platforms\PostgreSqlPlatform {

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent looks off

@DeepDiver1975
Copy link
Member

Rebased - this was really out dated - please always update your branches - thx

@DeepDiver1975
Copy link
Member

bildschirmfoto von 2017-07-31 09-11-00

@DeepDiver1975
Copy link
Member

if stuff above is addressed: 👍

@PVince81
Copy link
Contributor

Talked with @DeepDiver1975: let's not backport the bigint fileid thing. People should upgrade, especially considering that we're preparing a way already to upgrade from 8.2 to 10 (upgrade from 9.0 to 10 is already possible):

@VicDeo
Copy link
Member

VicDeo commented Jul 31, 2017

@DeepDiver1975 updated
@PVince81 so, this change is not for OC 10 or we will release 10 from master?

@PVince81
Copy link
Contributor

PVince81 commented Aug 1, 2017

@VicDeo sorry for the confusion. This is to be released for 10.0.3, so backport to stable10. But nothing for OC <= 9.1

@felixboehm felixboehm added the p1-urgent Critical issue, need to consider hotfix with just that issue label Aug 1, 2017
@VicDeo VicDeo force-pushed the inttest-adding-tests-using-big-fileids branch from 913441a to 20fa3f0 Compare August 2, 2017 16:26
@VicDeo
Copy link
Member

VicDeo commented Aug 2, 2017

Rebased for CI

@VicDeo VicDeo dismissed DeepDiver1975’s stale review August 2, 2017 16:26

All is addressed

@VicDeo
Copy link
Member

VicDeo commented Aug 2, 2017

@PVince81 @DeepDiver1975 Tests are passed. Can't merge without approval

@PVince81 PVince81 merged commit 51d6389 into master Aug 3, 2017
@PVince81 PVince81 deleted the inttest-adding-tests-using-big-fileids branch August 3, 2017 07:30
@PVince81
Copy link
Contributor

PVince81 commented Aug 3, 2017

@VicDeo please backport to stable10

@VicDeo
Copy link
Member

VicDeo commented Aug 3, 2017

Stable10: #28581

@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3 - To Review p1-urgent Critical issue, need to consider hotfix with just that issue status/STALE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants