fix(stats): filter registration stats by SummitAttendeeTicket.SummitID#535
Open
fix(stats): filter registration stats by SummitAttendeeTicket.SummitID#535
Conversation
Replace JOIN SummitOrder + SummitOrder.SummitID = :summit_id with a direct SummitAttendeeTicket.SummitID = :summit_id filter across the registration stats queries. Adds supporting indexes via migration Version20260429120000 and unit/integration tests covering the SQL shape and the new index presence.
📝 WalkthroughWalkthroughThis PR optimizes registration-stats query performance by refactoring SQL to scope results directly via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.51)PHPStan was skipped because the sandbox runner could not parse its output. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-535/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@database/migrations/model/Version20260429120000.php`:
- Around line 39-40: The index lookup is case-sensitive but Doctrine DBAL
returns index arrays keyed by lowercased names, so normalize keys before using
array_key_exists: transform $ticketIndexes and $attendeeIndexes to
lowercase-keyed arrays (e.g., via array_change_key_case or rebuilding keys) in
Version20260429120000 and then perform index existence checks in up() and down()
using strtolower(indexName) against those normalized arrays; update all index
checks that reference $ticketIndexes/$attendeeIndexes to use the lowercased keys
so ADD INDEX/DROP INDEX guards work reliably.
In `@tests/oauth2/OAuth2SummitApiTest.php`:
- Around line 1260-1268: The test currently self-heals by creating the snapshot
when $snapshotPath doesn't exist (code using mkdir + file_put_contents and
$this->addToAssertionCount), which masks regressions; change this behaviour in
OAuth2SummitApiTest (around the $snapshotPath check) so that when
file_exists($snapshotPath) is false the test fails explicitly (use
$this->fail("Missing snapshot: please add
tests/Fixtures/registration_stats_snapshot.json with expected output") or an
assertion like $this->assertFileExists($snapshotPath, "Missing snapshot...") )
instead of creating directories or writing the file; remove the
mkdir/file_put_contents branch and ensure any setup of fixtures is done manually
outside the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e39eda7c-f062-48cd-ae7b-158ba7723584
📒 Files selected for processing (5)
app/Models/Foundation/Summit/SummitRegistrationStats.phpdatabase/migrations/model/Version20260429120000.phptests/Migrations/RegistrationStatsIndexesTest.phptests/Unit/RegistrationStatsSqlPatternTest.phptests/oauth2/OAuth2SummitApiTest.php
Comment on lines
+39
to
+40
| $ticketIndexes = $this->sm->listTableIndexes('SummitAttendeeTicket'); | ||
| $attendeeIndexes = $this->sm->listTableIndexes('SummitAttendee'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Doctrine DBAL, does SchemaManager::listTableIndexes() return an array keyed by normalized/lowercased index names, making case-sensitive array_key_exists checks with mixed-case index names unreliable?
💡 Result:
Yes, in Doctrine DBAL, SchemaManager::listTableIndexes returns an array of Index objects keyed by normalized (lowercased) index names. This makes case-sensitive array_key_exists checks with mixed-case index names unreliable, as the keys are always lowercase regardless of the actual database index name casing.
Citations:
- 1: https://phpdox.net/demo/Symfony2/classes/Doctrine_DBAL_Schema_AbstractSchemaManager/listTableIndexes.xhtml
- 2: https://github.com/webmozart/doctrine-dbal/blob/master/lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php
🏁 Script executed:
cd database/migrations/model && wc -l Version20260429120000.phpRepository: OpenStackweb/summit-api
Length of output: 96
🏁 Script executed:
cd database/migrations/model && sed -n '30,90p' Version20260429120000.phpRepository: OpenStackweb/summit-api
Length of output: 2726
Normalize index-name lookups before array_key_exists.
Doctrine DBAL returns index arrays keyed by lowercased names. Case-sensitive lookups with mixed-case index names will always fail, causing guards to be ineffective: up() may attempt duplicate ADD INDEX operations, and down() may skip DROP INDEX operations.
Suggested fix
- $ticketIndexes = $this->sm->listTableIndexes('SummitAttendeeTicket');
- $attendeeIndexes = $this->sm->listTableIndexes('SummitAttendee');
+ $ticketIndexes = array_change_key_case(
+ $this->sm->listTableIndexes('SummitAttendeeTicket'),
+ CASE_LOWER
+ );
+ $attendeeIndexes = array_change_key_case(
+ $this->sm->listTableIndexes('SummitAttendee'),
+ CASE_LOWER
+ );
- if (!array_key_exists('IDX_SummitAttendeeTicket_Stats', $ticketIndexes)) {
+ if (!array_key_exists(strtolower('IDX_SummitAttendeeTicket_Stats'), $ticketIndexes)) {
$this->addSql(
'ALTER TABLE SummitAttendeeTicket ADD INDEX IDX_SummitAttendeeTicket_Stats (SummitID, Status, IsActive)'
);
}
- if (!array_key_exists('IDX_SummitAttendeeTicket_BoughtDate', $ticketIndexes)) {
+ if (!array_key_exists(strtolower('IDX_SummitAttendeeTicket_BoughtDate'), $ticketIndexes)) {
$this->addSql(
'ALTER TABLE SummitAttendeeTicket ADD INDEX IDX_SummitAttendeeTicket_BoughtDate (SummitID, Status, IsActive, TicketBoughtDate)'
);
}
- if (!array_key_exists('IDX_SummitAttendee_HallCheckIn', $attendeeIndexes)) {
+ if (!array_key_exists(strtolower('IDX_SummitAttendee_HallCheckIn'), $attendeeIndexes)) {
$this->addSql(
'ALTER TABLE SummitAttendee ADD INDEX IDX_SummitAttendee_HallCheckIn (SummitID, SummitHallCheckedIn, SummitHallCheckedInDate)'
);
}
- if (!array_key_exists('IDX_SummitAttendee_VirtualCheckIn', $attendeeIndexes)) {
+ if (!array_key_exists(strtolower('IDX_SummitAttendee_VirtualCheckIn'), $attendeeIndexes)) {
$this->addSql(
'ALTER TABLE SummitAttendee ADD INDEX IDX_SummitAttendee_VirtualCheckIn (SummitID, SummitVirtualCheckedInDate)'
);
}
@@
- $ticketIndexes = $this->sm->listTableIndexes('SummitAttendeeTicket');
- $attendeeIndexes = $this->sm->listTableIndexes('SummitAttendee');
+ $ticketIndexes = array_change_key_case(
+ $this->sm->listTableIndexes('SummitAttendeeTicket'),
+ CASE_LOWER
+ );
+ $attendeeIndexes = array_change_key_case(
+ $this->sm->listTableIndexes('SummitAttendee'),
+ CASE_LOWER
+ );
- if (array_key_exists('IDX_SummitAttendeeTicket_Stats', $ticketIndexes)) {
+ if (array_key_exists(strtolower('IDX_SummitAttendeeTicket_Stats'), $ticketIndexes)) {
$this->addSql('ALTER TABLE SummitAttendeeTicket DROP INDEX IDX_SummitAttendeeTicket_Stats');
}
- if (array_key_exists('IDX_SummitAttendeeTicket_BoughtDate', $ticketIndexes)) {
+ if (array_key_exists(strtolower('IDX_SummitAttendeeTicket_BoughtDate'), $ticketIndexes)) {
$this->addSql('ALTER TABLE SummitAttendeeTicket DROP INDEX IDX_SummitAttendeeTicket_BoughtDate');
}
- if (array_key_exists('IDX_SummitAttendee_HallCheckIn', $attendeeIndexes)) {
+ if (array_key_exists(strtolower('IDX_SummitAttendee_HallCheckIn'), $attendeeIndexes)) {
$this->addSql('ALTER TABLE SummitAttendee DROP INDEX IDX_SummitAttendee_HallCheckIn');
}
- if (array_key_exists('IDX_SummitAttendee_VirtualCheckIn', $attendeeIndexes)) {
+ if (array_key_exists(strtolower('IDX_SummitAttendee_VirtualCheckIn'), $attendeeIndexes)) {
$this->addSql('ALTER TABLE SummitAttendee DROP INDEX IDX_SummitAttendee_VirtualCheckIn');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@database/migrations/model/Version20260429120000.php` around lines 39 - 40,
The index lookup is case-sensitive but Doctrine DBAL returns index arrays keyed
by lowercased names, so normalize keys before using array_key_exists: transform
$ticketIndexes and $attendeeIndexes to lowercase-keyed arrays (e.g., via
array_change_key_case or rebuilding keys) in Version20260429120000 and then
perform index existence checks in up() and down() using strtolower(indexName)
against those normalized arrays; update all index checks that reference
$ticketIndexes/$attendeeIndexes to use the lowercased keys so ADD INDEX/DROP
INDEX guards work reliably.
Comment on lines
+1260
to
+1268
| $snapshotPath = base_path('tests/Fixtures/registration_stats_snapshot.json'); | ||
| if (!file_exists($snapshotPath)) { | ||
| if (!is_dir(dirname($snapshotPath))) { | ||
| mkdir(dirname($snapshotPath), 0755, true); | ||
| } | ||
| file_put_contents($snapshotPath, json_encode($actualStats, JSON_PRETTY_PRINT)); | ||
| $this->addToAssertionCount(1); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Snapshot test currently self-heals instead of enforcing regression detection.
If the fixture is absent, the test writes a new snapshot and passes, which can hide unintended output changes in clean CI runs.
Suggested fix
- if (!file_exists($snapshotPath)) {
- if (!is_dir(dirname($snapshotPath))) {
- mkdir(dirname($snapshotPath), 0755, true);
- }
- file_put_contents($snapshotPath, json_encode($actualStats, JSON_PRETTY_PRINT));
- $this->addToAssertionCount(1);
- return;
- }
+ $this->assertFileExists(
+ $snapshotPath,
+ 'Missing snapshot fixture at tests/Fixtures/registration_stats_snapshot.json. ' .
+ 'Commit the fixture so this test can enforce regressions in CI.'
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $snapshotPath = base_path('tests/Fixtures/registration_stats_snapshot.json'); | |
| if (!file_exists($snapshotPath)) { | |
| if (!is_dir(dirname($snapshotPath))) { | |
| mkdir(dirname($snapshotPath), 0755, true); | |
| } | |
| file_put_contents($snapshotPath, json_encode($actualStats, JSON_PRETTY_PRINT)); | |
| $this->addToAssertionCount(1); | |
| return; | |
| } | |
| $snapshotPath = base_path('tests/Fixtures/registration_stats_snapshot.json'); | |
| $this->assertFileExists( | |
| $snapshotPath, | |
| 'Missing snapshot fixture at tests/Fixtures/registration_stats_snapshot.json. ' . | |
| 'Commit the fixture so this test can enforce regressions in CI.' | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/oauth2/OAuth2SummitApiTest.php` around lines 1260 - 1268, The test
currently self-heals by creating the snapshot when $snapshotPath doesn't exist
(code using mkdir + file_put_contents and $this->addToAssertionCount), which
masks regressions; change this behaviour in OAuth2SummitApiTest (around the
$snapshotPath check) so that when file_exists($snapshotPath) is false the test
fails explicitly (use $this->fail("Missing snapshot: please add
tests/Fixtures/registration_stats_snapshot.json with expected output") or an
assertion like $this->assertFileExists($snapshotPath, "Missing snapshot...") )
instead of creating directories or writing the file; remove the
mkdir/file_put_contents branch and ensure any setup of fixtures is done manually
outside the test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/86b9nvavq
Replace JOIN SummitOrder + SummitOrder.SummitID = :summit_id with a direct SummitAttendeeTicket.SummitID = :summit_id filter across the registration stats queries. Adds supporting indexes via migration Version20260429120000 and unit/integration tests covering the SQL shape and the new index presence.
Summary by CodeRabbit
Chores
Tests