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

IBX-6173: Added indexes to ezcontentobject_attribute and ezurl_object_link tables #376

Merged
merged 4 commits into from
Aug 21, 2023

Conversation

mateuszdebinski
Copy link
Contributor

@mateuszdebinski mateuszdebinski commented Jul 12, 2023

Question Answer
JIRA issue IBX-6173
Type improvement
Target Ibexa version v3.3
BC breaks no

Upgrade script: https://github.com/ibexa/installer/pull/112

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/engineering-team).

@mateuszdebinski mateuszdebinski added Improvement Changes not fixing or changing behavior Ready for review labels Jul 12, 2023
@mateuszdebinski mateuszdebinski requested a review from a team July 12, 2023 12:30
@mateuszdebinski mateuszdebinski self-assigned this Jul 12, 2023
@mateuszdebinski mateuszdebinski force-pushed the IBX-6173_url_management_sql_optimization branch 4 times, most recently from f2a5fee to bd732e3 Compare July 28, 2023 13:20
@mateuszdebinski mateuszdebinski force-pushed the IBX-6173_url_management_sql_optimization branch from bd732e3 to 3b6786e Compare July 28, 2023 13:25
@@ -445,10 +445,10 @@ public function testLoadFieldType($content)
{
$this->assertSame(
$this->getTypeName(),
$content->fields[1]->type
Copy link
Member

Choose a reason for hiding this comment

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

The extensive changes to data dumps and changes here suggest that adding this index makes Content Gateway very unstable. It uncovers the known issue that comes back from time to time - gateway doesn't order fields by their position (ezcontentclass_attribute.placement).

The issue of ordering fields is resolved on API level via ContentMapper, however Handler undergoes consumption BC promise, so it should return data in the same order as well. Whether this is important issue or something we can fix as a follow up, depends on if the issue appears when using MySQL and PostgreSQL. Unfortunately on CI those tests are executed on SQLIte.

Could you please check if 776052f change causes the same issue on MySQL and/or PostgreSQL? If it does then to fix it we need to improve \eZ\Publish\Core\Persistence\Legacy\Content\Gateway\DoctrineDatabase::internalLoadContent by adding

use eZ\Publish\Core\Persistence\Legacy\Content\Type\Gateway  as TypeGateway;
use eZ\Publish\SPI\Persistence\Content\Type;
// (...)
         ->innerJoin(
                'a',
                TypeGateway::FIELD_DEFINITION_TABLE,
                'f_def',
                $expr->and(
                    $expr->eq('a.contentclassattribute_id', 'f_def.id'),
                    $expr->eq('f_def.version', $queryBuilder->createNamedParameter(Type::STATUS_DEFINED))
                )
            )
            // (...)
            ->orderBy('f_def.placement', 'ASC');

I see further updates to legacy persistence tests would be needed, so it's not a quick fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on PostgreSQL and MySQL tests pass without changes

Copy link
Member

Choose a reason for hiding this comment

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

on PostgreSQL and MySQL tests pass without changes

Ok, let's fix SQLite as a follow-up. Alternatively as that follow-up you can make sure that use cases for failing SQLite tests are already covered by the proper integration tests (they should) and drop SQLite tests, because they will be dropped eventually, once I have some time to analyze every single one of them ™️ 😉

@alongosz alongosz requested a review from a team July 28, 2023 15:37
@alongosz alongosz requested a review from a team August 2, 2023 11:34
@sonarcloud
Copy link

sonarcloud bot commented Aug 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@webhdx webhdx requested a review from a team August 8, 2023 11:50
@Steveb-p Steveb-p merged commit a53f741 into 1.3 Aug 21, 2023
26 checks passed
@Steveb-p Steveb-p deleted the IBX-6173_url_management_sql_optimization branch August 21, 2023 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Changes not fixing or changing behavior QA approved
Development

Successfully merging this pull request may close these issues.

7 participants