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

Use jsonb to avoid problems on batch delete #2267

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

jordisala1991
Copy link
Member

Subject

I am targeting this branch, because this is the safe branch to do it.

Changelog

### Fixed
- Fixed usage of this bundle with PostgreSQL.

@jordisala1991
Copy link
Member Author

@lukepass do you use MySQL? if so, can you check if this PR breaks your database schema in any way?

@jordisala1991 jordisala1991 requested review from VincentLanglet and a team January 20, 2022 15:15
@jordisala1991
Copy link
Member Author

Maybe it is better to just add the jsonb option to not do more hard BC breaks. @VincentLanglet

(we have more usages of json fields in SonataPageBundle)
(The roles field will become an array again , that was a mistake when removing FOSUserBundle)

@lukepass
Copy link

Should I just go on this branch and launch migrations or make some media upload?

@jordisala1991
Copy link
Member Author

Yes please, go to this branch and try to launch migrations. Also if you could remove your media table and try to update the schema again, to ensure nothing changes, would be helpful.

Thank you :)

<field name="providerMetadata" column="provider_metadata" type="json" nullable="true"/>
<field name="providerMetadata" column="provider_metadata" type="json" nullable="true">
<options>
<option name="jsonb">true</option>
Copy link
Member

Choose a reason for hiding this comment

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

Should we add tests for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it is really easy to test. The problem with the distinct query is only when you do a batch delete.

I think we might start with testing on different databases instead of trying to reproduce the problem that this PR solves.

Anyway we shouldn't block the PR for its test IMO. I guess if it does not break with Sqlite we are fine.

Copy link
Member

Choose a reason for hiding this comment

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

I personally don't block the PR for unit/functional test, but for real tests.

I'd like to avoid to release the 4.0 with a major bug for mysql user and then be in the situation where reverting the change introduce a bug for mongo users.

Copy link
Member

@core23 core23 left a comment

Choose a reason for hiding this comment

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

Did not test this, but looks okay so far

@core23
Copy link
Member

core23 commented Jan 22, 2022

IMHO we can release the stable 4.0 release after merging this. The last RC release was in November.

@core23 core23 requested a review from a team January 22, 2022 15:54
@VincentLanglet
Copy link
Member

Yes, I just wait for someone, like lukepass, to test this.

@lukepass
Copy link

lukepass commented Jan 22, 2022 via email

@lukepass
Copy link

It generated a migration:

<?php

declare(strict_types=1);

namespace DoctrineMigrations;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

/**
 * Auto-generated Migration: Please modify to your needs!
 */
final class Version20220124085858 extends AbstractMigration
{
    public function getDescription(): string
    {
        return '';
    }

    public function up(Schema $schema): void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE media__media CHANGE provider_metadata provider_metadata JSON DEFAULT NULL');
    }

    public function down(Schema $schema): void
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE media__media CHANGE provider_metadata provider_metadata JSON DEFAULT NULL');
    }
}

I executed it and made some tests, it seems all ok! The schema didn't break and the previous data didn't corrupt.

@jordisala1991 jordisala1991 merged commit 3c5f798 into sonata-project:4.x Jan 24, 2022
@jordisala1991 jordisala1991 deleted the hotfix/jsonb_type branch January 24, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants