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 delete entries in properties table with null value for fileid column and restrict null value in properties table #36084

Merged
merged 1 commit into from
Sep 6, 2019

Conversation

sharidas
Copy link
Contributor

New migrations have been added to the dav app to
acheive the following:

  1. Delete the existing entries in properties table
    which have null fileid entries
  2. Restrict null value entry for fileid in properties table.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

Restrict the usage of null value added to fileids in the properties table. When null value is allowed to enter, the background job like CleanProperties would end up in an infinite loop. Hence this PR addresses the problem by fixing it in the db, level. This introduces 2 migrations:

  1. Cleanup the properties table entries which have fileid with null value
  2. Alter the column fileid so that no null value is accepted.

Related Issue

Motivation and Context

To fix the problem caused by entry of null value in the fileid, its better to attack the problem or fix the problem in the db.

How Has This Been Tested?

  • Since this change invloves migration, I have used oc version 10.1.1 and 10.3.0 alpha
  • First install 10.1.1 oC instance. And then upload a file say a.txt
  • Now apply proppatch to the file.
  • Modify the properties entry in the table so that fileid is NULL
  • Add another entry in the table with fileid is NULL.
  • Now try to create another property where fileid is not NULL ( the idea here is after the migration this entry should be available to the user )
  • The table entry will look like this:
mysql> select * from oc_properties;
+----+--------+-----------------------------------------------+---------------+
| id | fileid | propertyname                                  | propertyvalue |
+----+--------+-----------------------------------------------+---------------+
|  1 |   NULL | {http://example.com/neon/litmus/}valnspace    | Object        |
|  2 |   NULL | {http://example.com/neon/litmus/}valnspace123 | Object        |
|  3 |     16 | somethingfunny                                | Truth         |
+----+--------+-----------------------------------------------+---------------+
3 rows in set (0.00 sec)

mysql>
  • Now follow the steps to upgrade. Remember that in this PR the dav version is upgraded to 0.5.0 hence the migration changes will be applied cleanly.
  • After the migration the table would will have contents:
mysql> select * from oc_properties;
+----+--------+----------------+---------------+
| id | fileid | propertyname   | propertyvalue |
+----+--------+----------------+---------------+
|  3 |     16 | somethingfunny | Truth         |
+----+--------+----------------+---------------+
1 row in set (0.00 sec)

mysql> describe oc_properties;
+---------------+---------------------+------+-----+---------+----------------+
| Field         | Type                | Null | Key | Default | Extra          |
+---------------+---------------------+------+-----+---------+----------------+
| id            | bigint(20)          | NO   | PRI | NULL    | auto_increment |
| fileid        | bigint(20) unsigned | NO   | MUL | NULL    |                |
| propertyname  | varchar(255)        | NO   |     |         |                |
| propertyvalue | varchar(255)        | NO   |     | NULL    |                |
+---------------+---------------------+------+-----+---------+----------------+
4 rows in set (0.00 sec)

mysql>
  • Try to insert null value to fileid as shown below, it wont allow:
mysql> insert into oc_properties (fileid, propertyname, propertyvalue) values (NULL, 'somethingfunny', 'Truth1');
ERROR 1048 (23000): Column 'fileid' cannot be null
mysql>

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@jvillafanez
Copy link
Member

In addition, double check that the migrations are always executed in the order we want. I think the MigrationService fetch the migrations sorted by file name, but I think this is something we want to make sure in order to prevent errors.

@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #36084 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #36084   +/-   ##
=======================================
  Coverage      54%      54%           
=======================================
  Files          63       63           
  Lines        7403     7403           
  Branches     1307     1307           
=======================================
  Hits         3998     3998           
  Misses       3019     3019           
  Partials      386      386
Flag Coverage Δ
#javascript 54% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86fd9ed...32b003d. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #36084 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #36084   +/-   ##
=======================================
  Coverage   54.01%   54.01%           
=======================================
  Files          63       63           
  Lines        7404     7404           
  Branches     1309     1309           
=======================================
  Hits         3999     3999           
  Misses       3019     3019           
  Partials      386      386
Flag Coverage Δ
#javascript 54.01% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f46d390...5d67c07. Read the comment docs.

@jvillafanez
Copy link
Member

I'll ping @DeepDiver1975 to check for the best practice here.
While I'd prefer to have only one migration, using \OC::$server there throws me off. Note that I'm not sure if we can rely on DI there to inject the connection.

@sharidas
Copy link
Contributor Author

I have tested with DI and it's working on my instance. I have updated this PR with DI.

New migrations have been added to the dav app to
acheive the following:
1. Delete the existing entries in properties table
   which have null fileid entries
2. Restrict null value entry for fileid in properties table.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@micbar
Copy link
Contributor

micbar commented Sep 5, 2019

@sharidas
Your approach is fine. Remove the corrupted entries before changing the schema.

@sharidas sharidas merged commit 39a6ce8 into master Sep 6, 2019
@delete-merged-branch delete-merged-branch bot deleted the migrations-dav-app branch September 6, 2019 12:11
@micbar micbar mentioned this pull request Sep 16, 2019
13 tasks
@sharidas sharidas changed the title Add new migrations to dav app Migration to delete entries in properties table with null value for fileid column and restrict null value in properties table Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cron.php runs forever
4 participants