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-340: Allowed URL Gateway queries to not perform count operations #180

Merged
merged 14 commits into from
May 20, 2021

Conversation

Steveb-p
Copy link
Contributor

@Steveb-p Steveb-p commented May 6, 2021

Question Answer
JIRA issue IBX-340
Type improvement
Target eZ Platform version v3.3
BC breaks no
Doc needed no

Prevent multiple database count queries when paginating URL lists.

Without the patch:
image

With the patch:
image

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/php-dev-team).

@Steveb-p Steveb-p changed the base branch from master to 1.2 May 6, 2021 12:58
@Steveb-p Steveb-p marked this pull request as ready for review May 6, 2021 13:42
@lserwatka
Copy link
Member

@Steveb-p we need a JIRA ticket for this PR, so our automatic changelog generator can hook it.

@Steveb-p
Copy link
Contributor Author

Steveb-p commented May 7, 2021

@lserwatka a proper Jira ticket has been added: IBX-340

@Steveb-p Steveb-p changed the title Allowed URL Gateway queries to not perform count operations IBX-340: Allowed URL Gateway queries to not perform count operations May 7, 2021
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

+1 but please create/update integration test. It's PAPI change

@Steveb-p
Copy link
Contributor Author

@adamwojs tests have been added.

Unfortunately tests for URL Gateway itself were missing, so I had to add them - and they show that some of the load* family functions return result sets, even though they should always return only 1 row. I didn't change that as that would technically be a behavior change.

I added the additional argument into Handler test case - it should cause the test to fail in cases where it isn't passed, so I think that's enough for unit tests for this one?

@ezsystems ezsystems deleted a comment from sonarcloud bot May 10, 2021
@Steveb-p
Copy link
Contributor Author

I've added some additional changes that I wanted to make a different PR with, but considering they are related to the changes here I'm including them now.

  1. eZ\Publish\Core\FieldType\Url\UrlStorage\Gateway\DoctrineStorage now shares constants with eZ\Publish\Core\Persistence\Legacy\URL\Gateway\DoctrineDatabase (for discoverability and to prevent making changes to one but not the other).
  2. Tests do not make use of constants (to detect changes to those)
  3. I've added coverage annotation to test cases I've touched. I'd like to advocate usage of class-level coverage annotation instead of method based one, as they are not endorsed by phpunit. They are also not properly detected by phpstorm afaik; for example multiple DoctrineGateway are displayed when test cases are requested, even though they come from different classes (see ctrl+shift+T shortcut on implementation class).

I'm unsure what to do with duplicated code detected by sonar cloud - it seems to come from a fixture file, which to be honest I don't see duplicated anywhere in terms of data contained 😕

@Steveb-p Steveb-p requested review from adamwojs and a team May 10, 2021 07:26
@adamwojs
Copy link
Member

+1 but please create/update integration test. It's PAPI change

@Steveb-p I meant \eZ\Publish\API\Repository\Tests\URLServiceTest here 😉

@Steveb-p
Copy link
Contributor Author

@adamwojs URLService tests have been updated.

@Steveb-p
Copy link
Contributor Author

Steveb-p commented May 10, 2021

Sonar cloud analysis:

  1. Duplication detects same code in https://github.com/ezsystems/ezplatform-kernel/blob/ab7f16fca538dd0e0e157bfb0151d023da1931fd/eZ/Publish/Core/Persistence/Legacy/Tests/Content/UrlAlias/_fixtures/publish_multilingual.php, which is clearly an entirely different file 😖
  2. Security hotspots are related to used http protocol in links in tests. I could "upgrade" them to https, but that would be a lot of unrelated changed code updated to https in f8f6bd2.

@Steveb-p Steveb-p requested review from a team and adamwojs May 10, 2021 11:22
@sonarcloud
Copy link

sonarcloud bot commented May 10, 2021

SonarCloud Quality Gate failed.

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

No Coverage information No Coverage information
11.9% 11.9% Duplication

Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on Ibexa Exp 3.2.7-dev.

@Steveb-p Steveb-p merged commit 6eda10e into 1.2 May 20, 2021
@Steveb-p Steveb-p deleted the fix-url-count branch May 20, 2021 06:21
@Steveb-p
Copy link
Contributor Author

Merged up in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants