Skip to content

Migrate metadata JSON column to new value TEXT column #37146

Merged
artonge merged 1 commit into
masterfrom
artonge/feat/migrate_metadata_to_value
Apr 4, 2023
Merged

Migrate metadata JSON column to new value TEXT column #37146
artonge merged 1 commit into
masterfrom
artonge/feat/migrate_metadata_to_value

Conversation

@artonge

@artonge artonge commented Mar 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

oc_files_metadata.metadata is created as Types::JSON. Beside not bringing any advantages, this creates the limitation of not being able to use the DISTINCT operator on it when using PostgreSQL.

The DISTINCT operator is for example used by photos: https://github.com/nextcloud/photos/blob/65d222db10ab5f005125d56576b8bd223b4dd054/lib/DB/Place/PlaceMapper.php#L56

This PR migrates the metadata column to a new value column as Types:STRING
It also renames and adapt the code accordingly.

Photos PR to adjust: nextcloud/photos#1692

Tested both on fresh install and after a migration.

@artonge artonge changed the title Artonge/feat/migrate_metadata_to_value Migrate metadata JSON column to new value STRING column Mar 9, 2023
@artonge artonge mentioned this pull request Mar 9, 2023
Merged
@artonge

artonge commented Mar 9, 2023

Copy link
Copy Markdown
Collaborator Author

/backport to stable26

@artonge artonge self-assigned this Mar 9, 2023
@artonge artonge added php Pull requests that update Php code 3. to review Waiting for reviews 2. developing Work in progress and removed backport-request 3. to review Waiting for reviews labels Mar 9, 2023
@artonge artonge added this to the Nextcloud 27 milestone Mar 9, 2023
@artonge artonge force-pushed the artonge/feat/migrate_metadata_to_value branch 2 times, most recently from d74d606 to 3481821 Compare March 9, 2023 12:10
artonge
artonge commented Mar 9, 2023
View reviewed changes
Comment thread core/Migrations/Version24000Date20220404230027.php Outdated
'length' => 50,
]);
$table->addColumn('metadata', Types::JSON, [
$table->addColumn('value', Types::STRING, [

@artonge artonge Mar 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok to simply edit the column, instead of commenting the migration and creating new one ?

@artonge artonge force-pushed the artonge/feat/migrate_metadata_to_value branch 4 times, most recently from 12753e3 to 9d238b3 Compare March 13, 2023 10:00
@artonge artonge added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 13, 2023
@artonge artonge marked this pull request as ready for review March 13, 2023 10:06
@artonge artonge requested review from a team, ArtificialOwl, blizzz, icewind1991 and nickvergessen and removed request for a team March 13, 2023 11:28
@ArtificialOwl

ArtificialOwl commented Mar 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

why Types::STRING and not Types::TEXT ?

@blizzz

blizzz commented Mar 13, 2023

Copy link
Copy Markdown
Member

@nickvergessen i recall, DISTINCT should be avoided for performance reasons?

@artonge

artonge commented Mar 13, 2023

Copy link
Copy Markdown
Collaborator Author

why Types::STRING and not Types::TEXT ?

No reason, why use one over the other ?

@nickvergessen

nickvergessen commented Mar 13, 2023

Copy link
Copy Markdown
Member

String is limited to 4000 chars IIRC

@blizzz

blizzz commented Mar 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

String translates to varchar, while TEXT translates up to Longtext (Mysql), respectively TEXT or CLOB on other DBs. Json translates to Json native types, apart of Sqlite and Oracle where it CLOB. So, going to TEXT would be the sorta 1:1 option, otherwise it may fail loosing data (MySQL?) or erroring out on migration.

→ https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/types.html#mapping-matrix

Still, the question on distinct?

nickvergessen
nickvergessen reviewed Mar 13, 2023
View reviewed changes
Comment thread core/Migrations/Version27000Date20230309104325.php Outdated
@artonge artonge force-pushed the artonge/feat/migrate_metadata_to_value branch 2 times, most recently from d805c04 to 0f882bc Compare March 15, 2023 14:43
@artonge

artonge commented Mar 15, 2023

Copy link
Copy Markdown
Collaborator Author

Squashed, any other comments ?

@blizzz blizzz changed the title Migrate metadata JSON column to new value STRING column Migrate metadata JSON column to new value TEXT column Mar 15, 2023
@artonge artonge requested a review from nickvergessen March 16, 2023 09:12
@artonge

artonge commented Mar 27, 2023

Copy link
Copy Markdown
Collaborator Author

Ping :)

nickvergessen
nickvergessen approved these changes Mar 27, 2023
View reviewed changes

@nickvergessen nickvergessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@szaimen

szaimen commented Mar 30, 2023

Copy link
Copy Markdown
Contributor

Autoloaders seem to not be up-to-date?

@artonge artonge force-pushed the artonge/feat/migrate_metadata_to_value branch from 0f882bc to 5a79e56 Compare March 30, 2023 13:58
@artonge

artonge commented Mar 30, 2023

Copy link
Copy Markdown
Collaborator Author

Autoloaders seem to not be up-to-date?

Indeed, fixed :)

datenangebot
datenangebot approved these changes Mar 30, 2023
View reviewed changes

@datenangebot datenangebot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good, did not test it.

Comment thread lib/private/Metadata/FileMetadata.php Outdated
@artonge
Migrate metadata as JSON to value as STRING
1a6709c 
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the artonge/feat/migrate_metadata_to_value branch from 5a79e56 to 1a6709c Compare April 3, 2023 12:06
@artonge

artonge commented Apr 4, 2023

Copy link
Copy Markdown
Collaborator Author

CI failure unrelated

@artonge artonge merged commit 7ab44b2 into master Apr 4, 2023
@artonge artonge deleted the artonge/feat/migrate_metadata_to_value branch April 4, 2023 08:23
@backportbot-nextcloud backportbot-nextcloud Bot mentioned this pull request Apr 4, 2023
Merged
@marcelklehr

marcelklehr commented Apr 20, 2023

Copy link
Copy Markdown
Member

Gnaa. This breaks recognize: nextcloud/recognize#806

@Andreaux

Andreaux commented Apr 25, 2023 •
edited
Loading

Copy link
Copy Markdown

Gnaa. This breaks recognize: nextcloud/recognize#806

Hahh. I'm looking into fixing recognize on my instance and I landed here. Has anyone got a suggestion?

@umgfoin

umgfoin commented Apr 27, 2023

Copy link
Copy Markdown
Contributor

Hahh. I'm looking into fixing recognize on my instance and I landed here. Has anyone got a suggestion?

Fixed with Fix BadFunctionCall Exception in Nextcloud 26

@joshtrichards joshtrichards mentioned this pull request Jul 16, 2023
Closed
@juliusknorr juliusknorr mentioned this pull request Feb 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@datenangebot datenangebot datenangebot approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl is a code owner automatically assigned from nextcloud/server-backend

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 is a code owner automatically assigned from nextcloud/server-backend

@blizzz blizzz Awaiting requested review from blizzz blizzz is a code owner automatically assigned from nextcloud/server-backend

Assignees

@artonge artonge

Labels

3. to review Waiting for reviews php Pull requests that update Php code

Projects

None yet

Milestone

Nextcloud 27

Development

Successfully merging this pull request may close these issues.

9 participants

@artonge @ArtificialOwl @blizzz @nickvergessen @szaimen @marcelklehr @Andreaux @umgfoin @datenangebot