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

Migration to fix term column length #28019

Merged
merged 5 commits into from
Jul 3, 2017
Merged

Migration to fix term column length #28019

merged 5 commits into from
Jul 3, 2017

Conversation

tomneedham
Copy link
Contributor

@tomneedham tomneedham commented May 26, 2017

Description

On some DB setups, the max index length can be as low as 767 bytes. This leads to a max column length of 191 chars - at the moment we have 256. https://dev.mysql.com/doc/refman/5.7/en/innodb-restrictions.html

Related Issue

#28007

How Has This Been Tested?

  • tested running the migration locally to observe column length change
  • tested with a term that is 256 chars and it is correctly deleted
  • added test to try and submit a term that is too long

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@tomneedham tomneedham added this to the 10.0.3 milestone May 26, 2017
@tomneedham tomneedham requested a review from PVince81 May 26, 2017 12:06
$prefix = $options['tablePrefix'];
$table = $schema->getTable("{$prefix}account_terms");
// Check column length
if($table->getColumn('term')->getLength() == 255) {
Copy link
Contributor

Choose a reason for hiding this comment

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

===


/** @var IDBConnection $db */
$db = \OC::$server->getDatabaseConnection();
$db->executeQuery("DELETE FROM {$prefix}account_terms WHERE CHAR_LENGTH(term) = 256;");
Copy link
Contributor

@PVince81 PVince81 May 26, 2017

Choose a reason for hiding this comment

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

>= 256 just to be on the safe side ?

for($i=0; $i<count($terms); $i++) {
// Ignore terms that are too long
if(strlen($terms[$i]) > 255) {
unset($terms[$i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

assuming that messing with the PHP array indices won't affect subsequent code

I'm saying this because once upon a time I had to call array_values() for such array to avoid logic that iterated over indices instead of foreach to fail...

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to use array_filter with a callback instead

@tomneedham
Copy link
Contributor Author

@@ -456,6 +456,10 @@ public function getSearchTerms() {
* @since 10.0.1
*/
public function setSearchTerms(array $terms) {
// Check length of terms
$terms = array_filter($terms, function($term) {
return strlen($term) <= 255;
Copy link
Member

Choose a reason for hiding this comment

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

Why not cut it off instead of fully skipping

Copy link
Contributor

Choose a reason for hiding this comment

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

@butonic cut or keep ?

there is also another ticket where @butonic suggests splitting terms on white spaces which should prevent overflows to occur in most cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt like cut off would just cause unpredictable behaviour, but then since we are searching from the beginning of these strings I guess even keeping the beginning could still be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

for now cut off. texts usually start with the most important information.

@PVince81
Copy link
Contributor

@tomneedham so we need to extend our query builder. Make the default "LENGTH" and have a special case in the MySQL adapter for "CHAR_LENGTH"

@PVince81 PVince81 added p1-urgent Critical issue, need to consider hotfix with just that issue backport-request labels May 30, 2017
@PVince81 PVince81 modified the milestones: 10.1, 10.0.3 May 31, 2017
@FlyingPersian
Copy link

When will this be updated on the main homepage as well? I'm still getting the SQLight error when downloading the install from the website. Also, I can't seem to be able to run this one when I download it. Seems that I need to compile it first?

@PVince81
Copy link
Contributor

PVince81 commented Jun 2, 2017

@FlyingPersian well first the fix must be finished and merged. Then it must be released as part of 10.0.3. And then we can update the web page and the owncloud-latest.zip link that the web installer uses to use 10.0.3.

If you're in a rush you can try installing from tarball directly and apply the patch manually before running the setup. (or manually changing that 256 value to 255 instead of patching)

@FlyingPersian
Copy link

FlyingPersian commented Jun 2, 2017

@PVince81: Alright, I tried adding the following three lines to the lib/private/User/User.php file, but the error still pops up:

// Check length of terms
		$terms = array_filter($terms, function($term) {
			return strlen($term) <= 255;

It basically looks like this now: http://i.imgur.com/aa1SqRY.png
I downloaded 10.0.2, extracted it, copied over my data folder and config.php file, added the three lines, set permissions with the permissions script, turned on maintenance mode, and then used the "occ upgrade" command to upgrade. That's when the errorr pops up.

Updating database schema
Doctrine\DBAL\Exception\DriverException: An exception occurred while executing ' CREATE TABLE oc_account_terms (id BIGINT UNSIGNED AUTO_INCREMENT NOT NULL, accou nt_id BIGINT UNSIGNED NOT NULL, term VARCHAR(256) NOT NULL, INDEX account_id_ind ex (account_id), INDEX term_index (term), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_bin ENGINE = InnoDB':

SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too lo ng; max key length is 767 bytes
Update failed

@PVince81
Copy link
Contributor

PVince81 commented Jun 2, 2017

@FlyingPersian I suggest to start over again (without what you patched) and use my simple instruction from #28007 (comment). There is only one place to change to get a successful install.

The rest of the PR here is only relevant if you're going to use LDAP.

@tomneedham
Copy link
Contributor Author

@FlyingPersian You can also disable mysql strict mode. @PVince81 this is the best solution / advice here, it will truncate the index as necessary, then this PR will fix the column in the next release.

@tomneedham
Copy link
Contributor Author

With mb4 support i believe the max index length is actually 191 characters. https://serversforhackers.com/mysql-utf8-and-indexing

@tomneedham
Copy link
Contributor Author

tomneedham commented Jun 5, 2017

Updated to reflect research into using 4 byte character sets. 191 is the max indexable string of characters.

Also updated to trim terms if too long.

@PVince81
Copy link
Contributor

Please rebase since the migration itself was merged separately.

@tomneedham
Copy link
Contributor Author

@PVince81 the oc_migration fix is unrelated - but is 'technically' the same issue -> too long columns

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81
Copy link
Contributor

@tomneedham please increase version.php then merge. This is needed to trigger new migrations else it won't check them at all.

@tomneedham
Copy link
Contributor Author

Set version to 10.0.2.3 because of #28182

@tomneedham
Copy link
Contributor Author

kicked jenkins

@PVince81 PVince81 merged commit 5f3e6b2 into master Jul 3, 2017
@PVince81 PVince81 deleted the terms-col-length branch July 3, 2017 12:29
@PVince81
Copy link
Contributor

PVince81 commented Jul 3, 2017

@tomneedham please backport

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants