Increase size of media.field_file_size #829

seth-shaw-unlv · 2021-03-17T16:51:35Z

GitHub Ticket: Islandora/documentation#1771

What does this Pull Request do?

Increases the size and signed state of media.field_file_size to support files larger than 2.2GB.

What's new?

Changes field.storage.media.field_file_size to be big and unsigned.
Added islandora_core_feature.install hook 8001 to resize the field without losing existing data.

How should this be tested?

Create a media
Attempt to save a large file size value on a media. E.g. drush eval "\Drupal::entityTypeManager()->getStorage('media')->load(1)->set('field_file_size', 2400000000000)->save();" This will throw an error.
Apply the PR
Run updates drush updb -y
Clear cache drush cr
Attempt to save the large file size value again: drush eval "\Drupal::entityTypeManager()->getStorage('media')->load(1)->set('field_file_size', 2400000000000)->save();" will return successfully.

Additional Notes:

This PR does not include the bigint module. I can update the PR to include it if folks want it, but it does take some work. It requires updating composer.json and finding all the relevant field configs to update them.

Interested parties

@Islandora/8-x-committers

adam-vessey · 2021-03-17T17:54:28Z

modules/islandora_core_feature/islandora_core_feature.install

+      ->fetchAll();
+
+    // Clean it out for resizing.
+    $database->truncate($table)->execute();


This seems potentially catastrophic, if the process later fails for some reason... different DB with different syntax or whatever?

I attempted to resize without cleaning it out and Drupal simply would not let me. Cleaning it out is the only way to save it. I suppose we could create a temporary table instead of relying on an in-memory store... 🤔

According to the Drupal notes this is how you truncate a table.

Somewhat ambiguous due to nomenclature, but I'm assuming you don't mean a "temporary table" as supported by various DB engines which are automatically deleted at the end of the DB session, as they would exhibit the same issue of losing the stored data if something failed before getting it back in... but yeah, a short-lived table we create/check-for and populate, and delete when we're done... assuming success with in-place alteration with changeField() doesn't make it irrelevant.

Looks like Drupal won't let changeField update the table without cleaning it out first either: "[error] The SQL storage cannot change the schema for an existing field (field_file_size in media entity) with data."

I'll see about creating a table to keep this all in, should the update fail and need to be attempted again.

Instead of a new table I created a temporary file to stash a JSON copy of each table before truncating. I then re-read the file to insert the data back in after the schema is updated.

As for using public, I didn't like the alternatives. Not all sites have a private filesystem set (a non-starter for an update hook) and temporary is at the whims of the OS' clean-up, so I'd rather not trust it for keeping our data dump in the case of a failure. At least a site admin knows where public is if they have to go grab it should the site fail. Also, it is just a list of file sizes.... That said, if y'all prefer we use the temporary file-space, it is a simple change; just confirm either way.

Ah, update hook batches (are a pain to use properly). I recently fought with this on a separate internal migration. Batch works best when the first and third steps are performed once and the middle step is repeated. (E.g. moving data from one field to another where the first phase sets up the new field, the middle step gradually copies, and the final step cleans up. Or a single repeatable step like modifying values in place.) Our three phases—dump all the data, make the change, re-import everything—are the inverse where the first and third can be done in chunks but the middle phase can't. Batching isn't suitable for this scenario.

I've attempted to use the strategy suggested by that recipe for in-place editing, but Drupal still balks at it.

I should also note that the pain of creating new tables, copying the data in then out, then dropping them didn't seem worth it compared to doing the same with temporary files briefly in the public filesystem. The attack window (knowing it exists and when) and reward of this is really small.

This stuck in my head last night; I might be able to use batch on this after all by nesting the batches within stages...

Instead of dumping to files and what not, might it be simpler to:

rename the target tables (assuming that renaming doesn't break similarly to the ::changeField() business.

create the new tables using the up-to-date schema

copy table content using insert/from, from the renamed to the newly created tables

drop the renamed copies

... if not, couldn't whatever batch structure be accomplished with multiple update hook implementations? A batch update hook followed by a non-batch, followed by a batch, kind of thing?

... any which way... still have the potential issue with the type change not being strictly widening with the move to exclude negative values... are we just ignoring this, or should we check for anything negative before trying to process, or... are we happy just letting it error how however it might?

Oh, nice. I had seen references to Schema::copyTable which was no longer available. I hadn't see the rename table function.

As I eluded to in my previous comment, I think we can do the batching in a single hook. Also, with the table rename tactic, we only really need to batch the insert/from, which is manageable.

I do think we need to error, or at least warn, on negative file sizes. I mean, semantically speaking, those values are already broken.

modules/islandora_core_feature/islandora_core_feature.install

dannylamb · 2021-03-17T18:28:06Z

@seth-shaw-unlv I looked into the test fails and it appears to be just syntax: https://github.com/Islandora/islandora/pull/829/checks?check_run_id=2132855766#step:13:13

seth-shaw-unlv · 2021-03-17T19:07:04Z

@dannylamb , huh... it was failing on something else earlier. Anyway, did we change something for phpcs? Running it locally with Drupal and DrupalPractice didn't throw that error.

seth-shaw-unlv · 2021-03-17T21:54:55Z

Tagging @kayakr and @mjordan since you both commented on the issue.

mjordan · 2021-03-25T02:18:01Z

Smoke tested this and it works as advertised. Table media__field_file_size before running the update hook:

mysql> describe media__field_file_size;
+-----------------------+------------------+------+-----+---------+-------+
| Field                 | Type             | Null | Key | Default | Extra |
+-----------------------+------------------+------+-----+---------+-------+
| bundle                | varchar(128)     | NO   | MUL |         |       |
| deleted               | tinyint(4)       | NO   | PRI | 0       |       |
| entity_id             | int(10) unsigned | NO   | PRI | NULL    |       |
| revision_id           | int(10) unsigned | NO   | MUL | NULL    |       |
| langcode              | varchar(32)      | NO   | PRI |         |       |
| delta                 | int(10) unsigned | NO   | PRI | NULL    |       |
| field_file_size_value | int(11)          | NO   |     | NULL    |       |
+-----------------------+------------------+------+-----+---------+-------+
7 rows in set (0.01 sec)

and after running it:

mysql> describe media__field_file_size;
+-----------------------+---------------------+------+-----+---------+-------+
| Field                 | Type                | Null | Key | Default | Extra |
+-----------------------+---------------------+------+-----+---------+-------+
| bundle                | varchar(128)        | NO   | MUL |         |       |
| deleted               | tinyint(4)          | NO   | PRI | 0       |       |
| entity_id             | int(10) unsigned    | NO   | PRI | NULL    |       |
| revision_id           | int(10) unsigned    | NO   | MUL | NULL    |       |
| langcode              | varchar(32)         | NO   | PRI |         |       |
| delta                 | int(10) unsigned    | NO   | PRI | NULL    |       |
| field_file_size_value | bigint(20) unsigned | NO   |     | NULL    |       |
+-----------------------+---------------------+------+-----+---------+-------+
7 rows in set (0.00 sec)

No warnings/errors anywhere, including when creating a media after the update.

mjordan

~~Smoke tested successfully.~~

See below...

mjordan · 2021-03-25T02:19:31Z

@adam-vessey OK for me to merge?

mjordan · 2021-03-25T03:13:54Z

Halt! When I'm in the issue-1771 branch, viewing nodes and media result in a "Client error" , with this error in the watchdog:

Symfony\Component\HttpKernel\Exception\PreconditionFailedHttpException: Unable to instantiate GeminiClient, missing Gemini URI in Islandora setting. in Drupal\islandora\GeminiClientFactory::create() (line 43 of /var/www/html/drupal/web/modules/contrib/islandora/src/GeminiClientFactory.php).

Curl shows HTTP/1.1 412 Precondition Failed.

This error is preceded in the watchdog by the following notice:

Attempted to create Gemini client without a Gemini URL defined.

Switching back to 8.x-1.x and drush cring puts things right. Behavior is consistenly replicable.

seth-shaw-unlv · 2021-03-25T05:55:19Z

@mjordan, that would be related to @dannylamb's purge of Gemini that was recently merged. I would talk to either him or @elizoller. It isn't related to this PR at all.

That said... give me a day or two to see if I can incorporate @adam-vessey's table-rename suggestion to work.

seth-shaw-unlv · 2021-03-26T06:00:34Z

This new version uses the table renaming strategy recommended by @adam-vessey. Coming up with the full table definition took some work as I couldn't find good documentation on creating it and needed to read through the createTable code to figure it out.

It works, but it would create a monster query for existing sites with lots of existing media, so we probably should incorporate batching. I should be able to do that tomorrow; I just wanted this bit safely stowed off my machine before I went to bed. 💤

seth-shaw-unlv · 2021-03-26T18:31:13Z

@adam-vessey & @mjordan the PR now includes batch support and should be ready to go. Give it a whirl.

mjordan · 2021-03-26T18:37:45Z

I can ingest a bunch o'objects to test the batching. Any suggestions on how many would be a miniumum, given that I'll be doing this on a Playbook VM running an a laptop with 16GB of physical RAM?

seth-shaw-unlv · 2021-03-26T19:07:26Z

@mjordan, it should work on any size system that can run Islandora. The main limitation isn't the box's resources, but your PHP memory limit and time-outs. I hopefully set the batch size to fit the default settings. I ran this on a vanilla islandora-playbook but, granted, I didn't have a lot of media in this test site.

Running this as it is on a box with a bunch o'objects to make sure it doesn't die would be great.

seth-shaw-unlv · 2021-03-27T01:35:53Z

@mjordan, hold the phone! I'm dropping the table too early. Patch coming.

seth-shaw-unlv · 2021-03-27T01:42:04Z

@mjordan, hopefully this is finally ready to go. 🤦‍♂️

mjordan · 2021-03-27T02:42:25Z

Will test on Sunday (👀)....

mjordan · 2021-03-27T22:21:07Z

@seth-shaw-unlv I already installed islandora_core_feature_update_8001() when I tested this the first time. Is it OK to change islandora_core_feature_update_8001(&$sandbox) to islandora_core_feature_update_8002(&$sandbox) before I run drush updb for the purposes of this second test?

seth-shaw-unlv · 2021-03-27T23:02:45Z

That should be fine, although you will eventually want to revert the version back using drupal_set_installed_schema_version(), in case we ever need to add an actual 8002.

mjordan · 2021-03-28T23:59:02Z

To work around my slightly botched second test from earlier today, I built a brand new Playbook and ingested around 100 images objects. The update hook worked, but on applying the PR, I got an exception related to #826 as documented below.

Before running update:

mysql> describe media__field_file_size;
+-----------------------+------------------+------+-----+---------+-------+
| Field                 | Type             | Null | Key | Default | Extra |
+-----------------------+------------------+------+-----+---------+-------+
| bundle                | varchar(128)     | NO   | MUL |         |       |
| deleted               | tinyint(4)       | NO   | PRI | 0       |       |
| entity_id             | int(10) unsigned | NO   | PRI | NULL    |       |
| revision_id           | int(10) unsigned | NO   | MUL | NULL    |       |
| langcode              | varchar(32)      | NO   | PRI |         |       |
| delta                 | int(10) unsigned | NO   | PRI | NULL    |       |
| field_file_size_value | int(11)          | NO   |     | NULL    |       |
+-----------------------+------------------+------+-----+---------+-------+
7 rows in set (0.00 sec)

mysql> select count(*) from media__field_file_size;
+----------+
| count(*) |
+----------+
|      431 |
+----------+
1 row in set (0.00 sec)

After running update:

mysql> describe media__field_file_size;
+-----------------------+---------------------+------+-----+---------+-------+
| Field                 | Type                | Null | Key | Default | Extra |
+-----------------------+---------------------+------+-----+---------+-------+
| bundle                | varchar(128)        | NO   | MUL |         |       |
| deleted               | tinyint(4)          | NO   | PRI | 0       |       |
| entity_id             | int(10) unsigned    | NO   | PRI | NULL    |       |
| revision_id           | int(10) unsigned    | NO   | MUL | NULL    |       |
| langcode              | varchar(32)         | NO   | PRI |         |       |
| delta                 | int(10) unsigned    | NO   | PRI | NULL    |       |
| field_file_size_value | bigint(20) unsigned | NO   |     | NULL    |       |
+-----------------------+---------------------+------+-----+---------+-------+
7 rows in set (0.00 sec)

mysql> select count(*) from media__field_file_size;
+----------+
| count(*) |
+----------+
|      431 |
+----------+
1 row in set (0.00 sec)

Here's the exception related to #826

vagrant@islandora8:/var/www/html/drupal/web/modules/contrib/islandora$ git checkout issue-1771
Branch 'issue-1771' set up to track remote branch 'issue-1771' from 'seth'.
Switched to a new branch 'issue-1771'
vagrant@islandora8:/var/www/html/drupal/web/modules/contrib/islandora$ drush updb -y
 ------------------------ ----------- --------------- ------------------------- 
  Module                   Update ID   Type            Description              
 ------------------------ ----------- --------------- ------------------------- 
  islandora_core_feature   8001        hook_update_n   Updates Media file size  
                                                       field storage for        
                                                       larger files.            
 ------------------------ ----------- --------------- ------------------------- 


 // Do you wish to run the specified pending updates?: yes.                                                             

>  [notice] Update started: islandora_core_feature_update_8001
>  [notice] Length of media.field_file_size updated to an unsigned big int
>  [notice] Update completed: islandora_core_feature_update_8001
>  [error]  Drupal\Component\Plugin\Exception\PluginNotFoundException while adding Views handlers for field Content » Author name on index Default Solr content index: Unable to determine class for field type 'media_track' found in the 'field.field.media.audio.field_track' configuration in Drupal\Core\Field\FieldConfigStorageBase->mapFromStorageRecords() (line 31 of /var/www/html/drupal/web/core/lib/Drupal/Core/Field/FieldConfigStorageBase.php). 
>  [error]  Drupal\search_api\SearchApiException while adding Views handlers for field Content » Edited Text on index Default Solr content index: Could not retrieve data definition for field 'Edited Text' on index 'Default Solr content index'. in Drupal\search_api\Item\Field->getDataDefinition() (line 441 of /var/www/html/drupal/web/modules/contrib/search_api/src/Item/Field.php). 
>  [error]  Drupal\search_api\SearchApiException while computing Views data for index Default Solr content index: Could not retrieve data definition for field 'Edited Text' on index 'Default Solr content index'. in Drupal\search_api\Item\Field->getDataDefinition() (line 441 of /var/www/html/drupal/web/modules/contrib/search_api/src/Item/Field.php). 
> 
> In FieldStorageConfigStorage.php line 167:
>                                                                                
>   Unable to determine class for field type 'media_track' found in the 'field.  
>   storage.media.field_track' configuration                                     
>                                                                                
> 
> In DiscoveryTrait.php line 53:
>                                                                                
>   The "media_track" plugin does not exist. Valid plugin IDs for Drupal\Core\F  
>   ield\FieldTypePluginManager are: comment, edtf, authority_link, typed_relat  
>   ion, datetime, file, file_uri, geolocation, image, link, list_integer, list  
>   _float, list_string, path, text_with_summary, text, text_long, created, map  
>   , uri, boolean, entity_reference, string, password, changed, timestamp, int  
>   eger, email, language, decimal, uuid, string_long, float                     
>                                                                                
> 
> updatedb:batch-process [--format [FORMAT]] [--fields FIELDS] [--field FIELD] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-d|--debug] [-y|--yes] [--no] [--remote-host REMOTE-HOST] [--remote-user REMOTE-USER] [-r|--root ROOT] [-l|--uri URI] [--simulate] [--pipe] [-D|--define DEFINE] [--xh-link XH-LINK] [--druplicon] [--notify [NOTIFY]] [--] <command> <batch_id>
> 

In ProcessBase.php line 155:
                    
  Output is empty.  
                    

updatedb [--cache-clear [CACHE-CLEAR]] [--entity-updates] [--post-updates [POST-UPDATES]] [--no-cache-clear] [--no-post-updates] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-d|--debug] [-y|--yes] [--no] [--remote-host REMOTE-HOST] [--remote-user REMOTE-USER] [-r|--root ROOT] [-l|--uri URI] [--simulate] [--pipe] [-D|--define DEFINE] [--xh-link XH-LINK] [--druplicon] [--notify [NOTIFY]] [--] <command>

seth-shaw-unlv · 2021-03-29T16:37:02Z

@mjordan, as far as I'm concerned that error is unrelated to this PR and shouldn't impact whether or not we merge it.

I think it is ready to merge, but that is up to you and @adam-vessey.

mjordan · 2021-03-29T21:38:11Z

@seth-shaw-unlv yes, your code worked, that exception is not related to this PR. @adam-vessey OK to merge? If so, I will do it.

elizoller · 2021-03-29T23:14:00Z

There is one, somewhat unrelated, issue. This branch isn't up to date with 8.x-1.x and it doesn't have the recent captions work. So if you have the most recent islandora_defaults and this branch of islandora, things might be funky such as

Unable to determine class for field type 'media_track'

adam-vessey · 2021-03-30T12:11:39Z

@mjordan : Should be, I think.

mjordan · 2021-03-30T13:44:03Z

To confirm, I just ran this update on a very recent Playbook (built last night) and it installed cleanly:

vagrant@islandora8:/var/www/html/drupal/web/modules/contrib/islandora$ git pull
remote: Enumerating objects: 39, done.
remote: Counting objects: 100% (39/39), done.
remote: Compressing objects: 100% (24/24), done.
remote: Total 39 (delta 24), reused 22 (delta 13), pack-reused 0
Unpacking objects: 100% (39/39), done.
From https://github.com/Islandora/islandora
   bdf9937..47213e2  8.x-1.x    -> origin/8.x-1.x
Updating bdf9937..47213e2
Fast-forward
 modules/islandora_core_feature/config/install/field.storage.media.field_file_size.yml |   4 +--
 modules/islandora_core_feature/islandora_core_feature.install                         | 137 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 139 insertions(+), 2 deletions(-)
 create mode 100644 modules/islandora_core_feature/islandora_core_feature.install
vagrant@islandora8:/var/www/html/drupal/web/modules/contrib/islandora$ drush updb -y
 ------------------------ ----------- --------------- ------------------------- 
  Module                   Update ID   Type            Description              
 ------------------------ ----------- --------------- ------------------------- 
  islandora_core_feature   8001        hook_update_n   Updates Media file size  
                                                       field storage for        
                                                       larger files.            
 ------------------------ ----------- --------------- ------------------------- 


 // Do you wish to run the specified pending updates?: yes.                                                             

>  [notice] Update started: islandora_core_feature_update_8001
>  [notice] Length of media.field_file_size updated to an unsigned big int
>  [notice] Update completed: islandora_core_feature_update_8001

 [success] Finished performing updates.

Increase size of media.field_file_size.

2bed2bc

adam-vessey reviewed Mar 17, 2021

View reviewed changes

seth-shaw-unlv added 2 commits March 17, 2021 19:12

coding style

781d0c4

Use temporary files for update and schema::changeField()

f0d0d90

seth-shaw-unlv requested a review from adam-vessey March 23, 2021 22:19

mjordan approved these changes Mar 25, 2021

View reviewed changes

Rename, recreate, and reload tables instead of using temporary files.

a52617e

Batch the update

a8f8e40

drop renamed tables *after* we are done copying

4a0a47c

mjordan merged commit 47213e2 into Islandora:8.x-1.x Mar 30, 2021

mjordan mentioned this pull request Mar 30, 2021

Media File Size field is too small Islandora/documentation#1771

Closed

seth-shaw-unlv deleted the issue-1771 branch March 30, 2021 15:03

jordandukart mentioned this pull request Apr 7, 2021

Standard mapping discoverygarden/dgi_migrate#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase size of media.field_file_size #829

Increase size of media.field_file_size #829

seth-shaw-unlv commented Mar 17, 2021

adam-vessey Mar 17, 2021

seth-shaw-unlv Mar 17, 2021

adam-vessey Mar 17, 2021

seth-shaw-unlv Mar 17, 2021

seth-shaw-unlv Mar 17, 2021

seth-shaw-unlv Mar 18, 2021

seth-shaw-unlv Mar 18, 2021

seth-shaw-unlv Mar 24, 2021

adam-vessey Mar 24, 2021 •

edited

Loading

seth-shaw-unlv Mar 24, 2021

dannylamb commented Mar 17, 2021

seth-shaw-unlv commented Mar 17, 2021

seth-shaw-unlv commented Mar 17, 2021

mjordan commented Mar 25, 2021

mjordan left a comment •

edited

Loading

mjordan commented Mar 25, 2021

mjordan commented Mar 25, 2021

seth-shaw-unlv commented Mar 25, 2021

seth-shaw-unlv commented Mar 26, 2021

seth-shaw-unlv commented Mar 26, 2021

mjordan commented Mar 26, 2021 •

edited

Loading

seth-shaw-unlv commented Mar 26, 2021

seth-shaw-unlv commented Mar 27, 2021

seth-shaw-unlv commented Mar 27, 2021

mjordan commented Mar 27, 2021

mjordan commented Mar 27, 2021 •

edited

Loading

seth-shaw-unlv commented Mar 27, 2021

mjordan commented Mar 28, 2021

seth-shaw-unlv commented Mar 29, 2021

mjordan commented Mar 29, 2021

elizoller commented Mar 29, 2021

adam-vessey commented Mar 30, 2021

mjordan commented Mar 30, 2021

Increase size of media.field_file_size #829

Increase size of media.field_file_size #829

Conversation

seth-shaw-unlv commented Mar 17, 2021

What does this Pull Request do?

What's new?

How should this be tested?

Additional Notes:

Interested parties

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-vessey Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannylamb commented Mar 17, 2021

seth-shaw-unlv commented Mar 17, 2021

seth-shaw-unlv commented Mar 17, 2021

mjordan commented Mar 25, 2021

mjordan left a comment • edited Loading

Choose a reason for hiding this comment

mjordan commented Mar 25, 2021

mjordan commented Mar 25, 2021

seth-shaw-unlv commented Mar 25, 2021

seth-shaw-unlv commented Mar 26, 2021

seth-shaw-unlv commented Mar 26, 2021

mjordan commented Mar 26, 2021 • edited Loading

seth-shaw-unlv commented Mar 26, 2021

seth-shaw-unlv commented Mar 27, 2021

seth-shaw-unlv commented Mar 27, 2021

mjordan commented Mar 27, 2021

mjordan commented Mar 27, 2021 • edited Loading

seth-shaw-unlv commented Mar 27, 2021

mjordan commented Mar 28, 2021

seth-shaw-unlv commented Mar 29, 2021

mjordan commented Mar 29, 2021

elizoller commented Mar 29, 2021

adam-vessey commented Mar 30, 2021

mjordan commented Mar 30, 2021

adam-vessey Mar 24, 2021 •

edited

Loading

mjordan left a comment •

edited

Loading

mjordan commented Mar 26, 2021 •

edited

Loading

mjordan commented Mar 27, 2021 •

edited

Loading