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.

Sign up for GitHub

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

Jump to bottom

Migrate metadata JSON column to new value TEXT column #37146

Merged
merged 1 commit into from
Apr 4, 2023
Merged

Migrate metadata JSON column to new value TEXT column #37146

merged 1 commit into from
Apr 4, 2023

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Mar 9, 2023 •
edited
Loading

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
Copy link
Contributor Author

artonge commented Mar 9, 2023

/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
core/Migrations/Version24000Date20220404230027.php Outdated
@@ -52,11 +52,14 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt
'notnull' => true,
'length' => 50,
]);
$table->addColumn('metadata', Types::JSON, [
$table->addColumn('value', Types::STRING, [
Copy link
Contributor Author

@artonge artonge Mar 9, 2023 •
edited
Loading

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 nickvergessen, a team, ArtificialOwl, icewind1991 and blizzz and removed request for a team March 13, 2023 11:28
@ArtificialOwl
Copy link
Member

ArtificialOwl commented Mar 13, 2023 •
edited
Loading

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

@blizzz
Copy link
Member

blizzz commented Mar 13, 2023

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

@artonge
Copy link
Contributor Author

artonge commented Mar 13, 2023

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

No reason, why use one over the other ?

@nickvergessen
Copy link
Member

String is limited to 4000 chars IIRC

@blizzz
Copy link
Member

blizzz commented Mar 13, 2023 •
edited
Loading

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?

@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
Copy link
Contributor Author

artonge commented Mar 15, 2023

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
Copy link
Contributor Author

artonge commented Mar 27, 2023

Ping :)

nickvergessen
nickvergessen approved these changes Mar 27, 2023
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

LGTM

@szaimen
Copy link
Contributor

szaimen commented Mar 30, 2023

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
Copy link
Contributor Author

artonge commented Mar 30, 2023

Autoloaders seem to not be up-to-date?

Indeed, fixed :)

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

@datenangebot datenangebot left a comment

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.

@artonge artonge force-pushed the artonge/feat/migrate_metadata_to_value branch from 5a79e56 to 1a6709c Compare April 3, 2023 12:06
@artonge
Copy link
Contributor Author

artonge commented Apr 4, 2023

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
Copy link
Member

Gnaa. This breaks recognize: nextcloud/recognize#806

@Andreaux
Copy link

Andreaux commented Apr 25, 2023 •
edited
Loading

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
Copy link

umgfoin commented Apr 27, 2023

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 was automatically assigned from nextcloud/server-backend

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 was automatically assigned from nextcloud/server-backend

@blizzz blizzz Awaiting requested review from blizzz blizzz was 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