Primary key is too long for oracle

[nickvergessen]

This is somewhat a crucial problem. The app is force enabled but breaks the installation on oracle.

Tested with nextcloud/notifications#75
Broken: https://travis-ci.org/nextcloud/notifications/builds/224355934#L1273
With this little change it passes: https://travis-ci.org/nextcloud/notifications/builds/224360873

The problem is, the old name was too long:

• 3 chars prefix: oc_
• 22 chars table name: twofactor_backup_codes
• 6 key suffix: _AI_PK

=> 31 chars, but the maximum is 30.

The only way I see is, that we rename the table manually on a pre-update step for existing installations. Otherwise all backup codes will be dropped on the update.

cc @MorrisJobke @ChristophWurst @LukasReschke @rullzer

[MorrisJobke]

The only way I see is, that we rename the table manually on a pre-update step for existing installations. Otherwise all backup codes will be dropped on the update.

🙈

I have the fear of long running upgrades somehow, but this is "only" a table rename and should be fine right?

[ChristophWurst]

The only way I see is, that we rename the table manually on a pre-update step for existing installations. Otherwise all backup codes will be dropped on the update.

Dunno if that is suitable here, but according to the docs MDB2 declaration has the was attribute for tables to allow renames: http://www.wiltonhotel.com/_ext/pear/docs/MDB2/docs/xml_schema_documentation.html#220.3.1

[nickvergessen]

Dunno if that is suitable here, but according to the docs MDB2 declaration has the was attribute for tables to allow renames

Yeah, maybe. But the Doctrine code we use currently does not support it.

[codecov]

Codecov Report

Merging #4425 into master will decrease coverage by 0.02%.
The diff coverage is 11.11%.

@@             Coverage Diff              @@
##             master    #4425      +/-   ##
============================================
- Coverage     54.19%   54.16%   -0.03%     
- Complexity    22168    22174       +6     
============================================
  Files          1364     1365       +1     
  Lines         84853    84886      +33     
  Branches       1322     1322              
============================================
- Hits          45985    45982       -3     
- Misses        38868    38904      +36

Impacted Files	Coverage Δ	Complexity Δ
.../twofactor_backupcodes/lib/Db/BackupCodeMapper.php	10% <33.33%> (ø)	5 <0> (ø)	⬇️
...kupcodes/lib/Migration/CopyEntriesFromOldTable.php	9.09% <9.09%> (ø)	6 <6> (?)
lib/private/Files/Cache/Propagator.php	94.93% <0%> (-1.27%)	16% <0%> (ø)
core/js/js.js	61.21% <0%> (-0.57%)	0% <0%> (ø)

[nickvergessen]

Actually the fix was quite easy, because we don't delete tables automatically yay
So now I renamed the table, created a migration step which checks whether the old table exists and if it does, it copies all the rows over and deletes the old table.

[MorrisJobke]

Tested and works via SQLite and Mysql

[ChristophWurst]

Gave it a test run on sqlite: 💥

php occ upgrade                                                                                                                  
Nextcloud or one of the apps require upgrade - only a limited number of commands are available
You may use your browser or the occ upgrade command to do the upgrade
Set log level to debug
Turned on maintenance mode
Updating database schema
Updated database
Updating <twofactor_backupcodes> ...
Doctrine\DBAL\Exception\TableExistsException: An exception occurred while executing 'CREATE INDEX two_factor_backupcodes_user_id ON "oc_twofactor_backupcodes" ("user_id")':

SQLSTATE[HY000]: General error: 1 index two_factor_backupcodes_user_id already exists
Update failed
Maintenance mode is kept active
Reset log level

[ChristophWurst]

@nickvergessen do we have to rename the index

server/apps/twofactor_backupcodes/appinfo/database.xml

Line 40 in 1ce47db

<name>two_factor_backupcodes_user_id</name>

too?

[nickvergessen]

Yeah looks like, too odd. I hope SQLite is the only DB which has global index names instead of being bound to the table they are in?

[nickvergessen]

Done, thanks for testing :)

[ChristophWurst]

Now it works 🚀

[MorrisJobke]

Let me try to fix the conflicts.

[MorrisJobke]

Rebased.