feat(speakers): add unique activities count endpoints for speakers an…#539
feat(speakers): add unique activities count endpoints for speakers an…#539
Conversation
…d submitters
- Add GET /summits/{id}/speakers/all/events/count and GET /summits/{id}/submitters/all/events/count controller methods with OpenAPI attributes and route registrations
- Implement getUniqueActivitiesCountBySummit in DoctrineSpeakerRepository and DoctrineMemberRepository using two-stage DQL→raw SQL COUNT(DISTINCT)
- Add interface declarations to ISpeakerRepository and IMemberRepository
- Register both endpoints in ApiEndpointsSeeder with ReadSummitData scopes
- Add PHPUnit tests for both HTTP endpoints and repository-level method
- Fix null-guard bug in DoctrineMemberRepository::applyExtraJoins
📝 WalkthroughWalkthroughThis PR adds two new API endpoints for counting unique activities: ChangesUnique Activities Count Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-539/ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.php (1)
174-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
str_containssubstring match allows origin-check bypass — replace with exact allowlist comparison.The
rtrimnormalization on line 176 is correct for its stated purpose, but the underlyingstr_containsapproach exposes a well-known security vulnerability: it performs a plain substring search, so an attacker can trivially bypass it.Concrete bypass examples (assuming
getAllowedOrigins()returns"https://trusted.com"):
- Send
Origin: trusted.com(scheme stripped) →str_contains("https://trusted.com", "trusted.com")→true✅ bypassed- Send
Origin: https://trusted.com.evil.io→str_contains("https://trusted.com", "https://trusted.com.evil.io")→false(safe in this direction), but the reverse layout can still be abused depending on how origins are stored.The safest origin validation is the simplest: an exact string comparison against a static set of allowed origins. Any parsing logic — substring checks, regex, URL component extraction — introduces edge cases that can be exploited.
If
getAllowedOrigins()returns a delimited string (space or comma separated), the correct fix is to parse it into an array and use an exact match:🔒 Proposed fix: exact-match against a parsed allowlist
-&& (is_null($origin) || empty($origin)|| str_contains($token_info->getAllowedOrigins(), rtrim($origin, '/')) === false ) +&& (is_null($origin) || empty($origin) || !in_array(rtrim($origin, '/'), array_map('trim', explode(' ', $token_info->getAllowedOrigins())), true))Adjust the
explodedelimiter to match whatever separatorgetAllowedOrigins()uses (space, comma, etc.). This guarantees only exact origin values pass, with no substring leakage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.php` around lines 174 - 184, The current origin check in OAuth2BearerAccessTokenRequestValidator.php uses str_contains with rtrim($origin, '/') which allows substring matches; replace this with an exact allowlist comparison: call $token_info->getAllowedOrigins(), split the returned string into an array using the correct delimiter (comma/space as used by your system), normalize each allowed entry (trim and rtrim trailing '/'), normalize the incoming $origin the same way (rtrim($origin, '/')), and then use strict equality/in_array to verify the origin only passes if it exactly matches one of the allowed origins; keep the existing exception throw (OAuth2ResourceServerException with OAuth2Protocol::OAuth2Protocol_Error_UnauthorizedClient) when the exact-match check fails.
🧹 Nitpick comments (5)
app/ModelSerializers/Summit/SummitSerializer.php (2)
323-343: ⚡ Quick winConsider catching
\Throwableinstead of\Exceptionto also guard against fatal errors.
\Exceptiondoes not catch\Error,\TypeError, or\ParseError(PHP 7+). If$build_default_payment_gateway_profile_strategy->build(...)or the serializer call triggers a type error or other engine-level error (e.g., passing wrong argument types), it will still propagate and break serialization. Since the intent is to never let a missing default profile crash the overall response, catching\Throwableis the safer choice.🔧 Suggested improvement
- } catch (\Exception $ex) { + } catch (\Throwable $ex) { Log::warning($ex->getMessage()); }Apply the same change to both catch blocks (lines 328 and 340).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/ModelSerializers/Summit/SummitSerializer.php` around lines 323 - 343, The two catch blocks wrapping calls to $build_default_payment_gateway_profile_strategy->build(...), SerializerRegistry::getInstance()->getSerializer(...), and $serializer->serialize(...) should catch \Throwable instead of \Exception so engine-level errors (TypeError, ParseError, Error) are also handled; update both catch clauses to catch \Throwable $ex and keep the existing Log::warning($ex->getMessage()) behavior to ensure serialization failures for default payment profiles do not abort the response.
328-330: ⚡ Quick winLog the exception with stack trace and include the profile type for debuggability.
Log::warning($ex->getMessage())discards the stack trace and provides no context about which profile type failed. In production, a swallowed exception with only a message string is hard to diagnose.🔧 Suggested improvement
- } catch (\Exception $ex) { - Log::warning($ex->getMessage()); + } catch (\Exception $ex) { + Log::warning(sprintf("SummitSerializer::serialize - failed to build default registration payment profile: %s", $ex->getMessage()), ['exception' => $ex]); }- } catch (\Exception $ex) { - Log::warning($ex->getMessage()); + } catch (\Exception $ex) { + Log::warning(sprintf("SummitSerializer::serialize - failed to build default bookable rooms payment profile: %s", $ex->getMessage()), ['exception' => $ex]); }Also applies to: 340-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/ModelSerializers/Summit/SummitSerializer.php` around lines 328 - 330, Replace the current catch blocks that only call Log::warning($ex->getMessage()) so they log the full exception and include the profile type context; specifically, in the catch (\Exception $ex) blocks in SummitSerializer.php (the ones around the profile-processing code) update the Log::warning call to include the exception object and a profile type identifier (e.g., the loop variable $type or $profileType) in the context array so the log contains the stack trace and which profile type failed; apply the same change to the second catch block noted (lines ~340-342).app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php (1)
549-597: ⚡ Quick winFilter map and validation rules are duplicated four times across this class
The
FilterParser::parseallowed-fields array (21 entries) and the$filter->validate()rules array (21 entries) are copied verbatim ingetAllBySummit,getAllBySummitCSV,send, and nowgetSubmittersActivitiesCount. Extract them into private helper methods (or class constants) to avoid the DRY violation and reduce the risk of future divergence between the copies.♻️ Proposed refactor: extract private helpers
+ private function submitterFilterFields(): array + { + return [ + 'id' => ['=='], + 'not_id' => ['=='], + 'first_name' => ['=@', '@@', '=='], + 'last_name' => ['=@', '@@', '=='], + 'email' => ['=@', '@@', '=='], + 'full_name' => ['=@', '@@', '=='], + 'member_id' => ['=='], + 'member_user_external_id' => ['=='], + 'has_accepted_presentations' => ['=='], + 'has_alternate_presentations' => ['=='], + 'has_rejected_presentations' => ['=='], + 'presentations_track_id' => ['=='], + 'presentations_selection_plan_id' => ['=='], + 'presentations_type_id' => ['=='], + 'presentations_title' => ['=@', '@@', '=='], + 'presentations_abstract' => ['=@', '@@', '=='], + 'presentations_submitter_full_name' => ['=@', '@@', '=='], + 'presentations_submitter_email' => ['=@', '@@', '=='], + 'is_speaker' => ['=='], + 'has_media_upload_with_type' => ['=='], + 'has_not_media_upload_with_type' => ['=='], + ]; + } + + private function submitterFilterValidationRules(): array + { + return [ + 'id' => 'sometimes|integer', + 'not_id' => 'sometimes|integer', + 'first_name' => 'sometimes|string', + 'last_name' => 'sometimes|string', + 'email' => 'sometimes|string', + 'full_name' => 'sometimes|string', + 'member_id' => 'sometimes|integer', + 'member_user_external_id' => 'sometimes|integer', + 'has_accepted_presentations' => 'sometimes|string|in:true,false', + 'has_alternate_presentations' => 'sometimes|string|in:true,false', + 'has_rejected_presentations' => 'sometimes|string|in:true,false', + 'presentations_track_id' => 'sometimes|integer', + 'presentations_selection_plan_id' => 'sometimes|integer', + 'presentations_type_id' => 'sometimes|integer', + 'presentations_title' => 'sometimes|string', + 'presentations_abstract' => 'sometimes|string', + 'presentations_submitter_full_name' => 'sometimes|string', + 'presentations_submitter_email' => 'sometimes|string', + 'is_speaker' => 'sometimes|string|in:true,false', + 'has_media_upload_with_type' => 'sometimes|integer', + 'has_not_media_upload_with_type' => 'sometimes|integer', + ]; + }Then replace each inline array in
getSubmittersActivitiesCount(and the other three methods) with:- $filter = FilterParser::parse(Request::input('filter'), [ - 'id' => ['=='], - // ... 20 more entries - ]); + $filter = FilterParser::parse(Request::input('filter'), $this->submitterFilterFields());- $filter->validate([ - 'id' => 'sometimes|integer', - // ... 20 more entries - ]); + $filter->validate($this->submitterFilterValidationRules());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php` around lines 549 - 597, The filter allowed-fields array passed to FilterParser::parse and the validation rules passed to $filter->validate are duplicated across getAllBySummit, getAllBySummitCSV, send, and getSubmittersActivitiesCount; extract them into reusable private helpers or constants (e.g., private static function submitterFilterMap(): array and private static function submitterFilterRules(): array or class constants) and replace the inline arrays in each method with calls to those helpers; update getSubmittersActivitiesCount to call FilterParser::parse(Request::input('filter'), self::submitterFilterMap()) (or the helper) and $filter->validate(self::submitterFilterRules()) (or the helper) so all four methods share the single source of truth.tests/oauth2/OAuth2SummitSubmittersApiTest.php (1)
272-302: ⚡ Quick winStrengthen filtered-count test to verify the filter is actually applied.
At Line 297 onward, the test only checks that
countexists and is non-negative, so it can still pass if the endpoint ignoresfilter. Compare against an unfiltered baseline and assert filteredcountis less than or equal to baseline.Suggested test hardening
public function testGetCurrentSummitSubmittersActivitiesCountWithAcceptedPresentations() { + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $baselineResponse = $this->action( + "GET", + "OAuth2SummitSubmittersApiController@getSubmittersActivitiesCount", + ['id' => self::$summit->getId()], + [], + [], + [], + $headers + ); + $this->assertResponseStatus(200); + $baselineData = json_decode($baselineResponse->getContent()); + $this->assertNotNull($baselineData); + $this->assertTrue(isset($baselineData->count)); + $params = [ 'id' => self::$summit->getId(), 'filter' => [ 'has_accepted_presentations==true', ], ]; - - $headers = [ - "HTTP_Authorization" => " Bearer " . $this->access_token, - "CONTENT_TYPE" => "application/json" - ]; $response = $this->action( "GET", "OAuth2SummitSubmittersApiController@getSubmittersActivitiesCount", $params, @@ $data = json_decode($content); $this->assertNotNull($data); $this->assertTrue(isset($data->count)); - $this->assertGreaterThanOrEqual(0, $data->count); + $this->assertLessThanOrEqual($baselineData->count, $data->count); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/oauth2/OAuth2SummitSubmittersApiTest.php` around lines 272 - 302, The test testGetCurrentSummitSubmittersActivitiesCountWithAcceptedPresentations currently only asserts the filtered response has a non-negative count; update it to call getSubmittersActivitiesCount twice: once with $params including the 'has_accepted_presentations==true' filter and once with a baseline $params_no_filter (same summit id but no filter) to get an unfiltered count, decode both responses and assert filtered_count is set and filtered_count <= baseline_count; keep using the same headers and response assertions (status 200, json decode) and reference the existing $params and getSubmittersActivitiesCount call sites to implement the second request and the new comparison assertion.app/Repositories/Summit/DoctrineSpeakerRepository.php (1)
731-755: 🏗️ Heavy liftConsider avoiding full speaker-ID materialization for large summits.
Line 731 loads all matching speaker IDs into memory, and Lines 742-747 pass them as a large
IN (:speaker_ids)list. This can become slow (or hit DB/driver limits) for large datasets. A single-query count strategy (join/subquery) would scale better.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Repositories/Summit/DoctrineSpeakerRepository.php` around lines 731 - 755, The code in DoctrineSpeakerRepository materializes $speakerIds via $qb->getQuery()->getArrayResult() and then uses IN (:speaker_ids), which can blow up for large summits; instead change the count query to avoid loading IDs by embedding the speaker-selection query as a subquery or join (reuse the original $qb filter logic rather than $speakerIds) so the final SQL counts DISTINCT p.ID where se.SummitID = :summit_id AND (p.ModeratorID IN (SELECT s.ID FROM /* speaker table or the same qb filters */) OR EXISTS (SELECT 1 FROM Presentation_Speakers ps WHERE ps.PresentationID = p.ID AND ps.PresentationSpeakerID IN (SELECT s.ID FROM /* same speaker subquery */))); update the code that currently calls getEntityManager()->getConnection()->executeQuery(...) to bind only summit_id and remove the array parameter for speaker_ids so no large PHP-array is passed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php`:
- Around line 520-521: The OpenAPI description string in
OAuth2SummitSubmittersApiController that documents filterable fields is out of
sync with the actual parser used (FilterParser::parse) — update the description
string to include the missing filter fields: not_id, member_user_external_id,
presentations_title, presentations_abstract, presentations_submitter_full_name,
and presentations_submitter_email so the spec matches the implementation (or
alternatively remove those keys from the parse map if they should not be
supported); locate the description value near the filter schema definition in
the controller and make the textual list consistent with FilterParser::parse.
In `@app/Repositories/Summit/DoctrineMemberRepository.php`:
- Around line 647-689: The current getUniqueActivitiesCountBySummit method loads
all matching member IDs into PHP and uses them in an IN(...) clause which is a
scalability hazard; replace the two-step approach with a single DB-side query
(either DQL or raw SQL) that counts DISTINCT Presentation IDs using a correlated
subquery or EXISTS to restrict p.CreatedByID to members who match the
summit+filter, so no IDs are materialized in PHP. Locate
getUniqueActivitiesCountBySummit and remove the array_column/getArrayResult
logic; build a single query that keeps :summit_id as a parameter and expresses
the member filter via an EXISTS (or a subselect from Member) so you can still
reuse the filter mapping logic (applyExtraJoins / getFilterMappings /
Filter::apply2Query) by applying it into the subquery/DQL where appropriate;
preserve parameter typing for summit_id and avoid passing large arrays to the DB
driver.
In `@docker-compose.override.testing.yml`:
- Around line 4-16: The Compose override mistakenly uses the YAML tag !reset for
the ports lists (the two ports blocks shown), which clears the base ports
instead of replacing them; change the tag to !override so the provided port
mappings replace the base values (or remove the tag to append if that's desired)
for the ports entries shown (the first ports block with 1889:1888 etc. and the
elasticsearch ports block 9202:9200, 9302:9300); also ensure your environment
uses Docker Compose v2+ or remove these tags if you must remain compatible with
legacy docker-compose v1.
---
Outside diff comments:
In `@app/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.php`:
- Around line 174-184: The current origin check in
OAuth2BearerAccessTokenRequestValidator.php uses str_contains with
rtrim($origin, '/') which allows substring matches; replace this with an exact
allowlist comparison: call $token_info->getAllowedOrigins(), split the returned
string into an array using the correct delimiter (comma/space as used by your
system), normalize each allowed entry (trim and rtrim trailing '/'), normalize
the incoming $origin the same way (rtrim($origin, '/')), and then use strict
equality/in_array to verify the origin only passes if it exactly matches one of
the allowed origins; keep the existing exception throw
(OAuth2ResourceServerException with
OAuth2Protocol::OAuth2Protocol_Error_UnauthorizedClient) when the exact-match
check fails.
---
Nitpick comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php`:
- Around line 549-597: The filter allowed-fields array passed to
FilterParser::parse and the validation rules passed to $filter->validate are
duplicated across getAllBySummit, getAllBySummitCSV, send, and
getSubmittersActivitiesCount; extract them into reusable private helpers or
constants (e.g., private static function submitterFilterMap(): array and private
static function submitterFilterRules(): array or class constants) and replace
the inline arrays in each method with calls to those helpers; update
getSubmittersActivitiesCount to call
FilterParser::parse(Request::input('filter'), self::submitterFilterMap()) (or
the helper) and $filter->validate(self::submitterFilterRules()) (or the helper)
so all four methods share the single source of truth.
In `@app/ModelSerializers/Summit/SummitSerializer.php`:
- Around line 323-343: The two catch blocks wrapping calls to
$build_default_payment_gateway_profile_strategy->build(...),
SerializerRegistry::getInstance()->getSerializer(...), and
$serializer->serialize(...) should catch \Throwable instead of \Exception so
engine-level errors (TypeError, ParseError, Error) are also handled; update both
catch clauses to catch \Throwable $ex and keep the existing
Log::warning($ex->getMessage()) behavior to ensure serialization failures for
default payment profiles do not abort the response.
- Around line 328-330: Replace the current catch blocks that only call
Log::warning($ex->getMessage()) so they log the full exception and include the
profile type context; specifically, in the catch (\Exception $ex) blocks in
SummitSerializer.php (the ones around the profile-processing code) update the
Log::warning call to include the exception object and a profile type identifier
(e.g., the loop variable $type or $profileType) in the context array so the log
contains the stack trace and which profile type failed; apply the same change to
the second catch block noted (lines ~340-342).
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Around line 731-755: The code in DoctrineSpeakerRepository materializes
$speakerIds via $qb->getQuery()->getArrayResult() and then uses IN
(:speaker_ids), which can blow up for large summits; instead change the count
query to avoid loading IDs by embedding the speaker-selection query as a
subquery or join (reuse the original $qb filter logic rather than $speakerIds)
so the final SQL counts DISTINCT p.ID where se.SummitID = :summit_id AND
(p.ModeratorID IN (SELECT s.ID FROM /* speaker table or the same qb filters */)
OR EXISTS (SELECT 1 FROM Presentation_Speakers ps WHERE ps.PresentationID = p.ID
AND ps.PresentationSpeakerID IN (SELECT s.ID FROM /* same speaker subquery
*/))); update the code that currently calls
getEntityManager()->getConnection()->executeQuery(...) to bind only summit_id
and remove the array parameter for speaker_ids so no large PHP-array is passed.
In `@tests/oauth2/OAuth2SummitSubmittersApiTest.php`:
- Around line 272-302: The test
testGetCurrentSummitSubmittersActivitiesCountWithAcceptedPresentations currently
only asserts the filtered response has a non-negative count; update it to call
getSubmittersActivitiesCount twice: once with $params including the
'has_accepted_presentations==true' filter and once with a baseline
$params_no_filter (same summit id but no filter) to get an unfiltered count,
decode both responses and assert filtered_count is set and filtered_count <=
baseline_count; keep using the same headers and response assertions (status 200,
json decode) and reference the existing $params and getSubmittersActivitiesCount
call sites to implement the second request and the new comparison assertion.
🪄 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: 45e856b5-750a-458d-9444-98a31bf6e5c7
📒 Files selected for processing (17)
.env.example.gitignoreapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.phpapp/ModelSerializers/Summit/SummitSerializer.phpapp/Models/Foundation/Main/Repositories/IMemberRepository.phpapp/Models/Foundation/Summit/Repositories/ISpeakerRepository.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ConfigSeeder.phpdocker-compose.override.testing.ymlroutes/api_v1.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
| description: "Filter query (supports multiple operators). Filterable fields: id, first_name, last_name, email, full_name, member_id, has_accepted_presentations, has_alternate_presentations, has_rejected_presentations, presentations_track_id, presentations_selection_plan_id, presentations_type_id, is_speaker, has_media_upload_with_type, has_not_media_upload_with_type.", | ||
| schema: new OA\Schema(type: "string", example: "has_accepted_presentations==true") |
There was a problem hiding this comment.
OpenAPI filter description lists fewer filterable fields than the implementation accepts
The description string on line 520 explicitly names the accepted filter fields but omits six fields that are in fact parsed by FilterParser::parse: not_id, member_user_external_id, presentations_title, presentations_abstract, presentations_submitter_full_name, and presentations_submitter_email. Clients reading the spec will believe those fields are unsupported and won't use them, and any contract-test tooling that enforces the documented field list will reject valid requests.
Either update the description to list all accepted fields, or intentionally remove the undocumented fields from the parse map (if they truly shouldn't be filterable on this endpoint).
📝 Proposed fix: align description with implementation
- description: "Filter query (supports multiple operators). Filterable fields: id, first_name, last_name, email, full_name, member_id, has_accepted_presentations, has_alternate_presentations, has_rejected_presentations, presentations_track_id, presentations_selection_plan_id, presentations_type_id, is_speaker, has_media_upload_with_type, has_not_media_upload_with_type.",
+ description: "Filter query (supports multiple operators). Filterable fields: id, not_id, first_name, last_name, email, full_name, member_id, member_user_external_id, has_accepted_presentations, has_alternate_presentations, has_rejected_presentations, presentations_track_id, presentations_selection_plan_id, presentations_type_id, presentations_title, presentations_abstract, presentations_submitter_full_name, presentations_submitter_email, is_speaker, has_media_upload_with_type, has_not_media_upload_with_type.",Also applies to: 549-571
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php`
around lines 520 - 521, The OpenAPI description string in
OAuth2SummitSubmittersApiController that documents filterable fields is out of
sync with the actual parser used (FilterParser::parse) — update the description
string to include the missing filter fields: not_id, member_user_external_id,
presentations_title, presentations_abstract, presentations_submitter_full_name,
and presentations_submitter_email so the spec matches the implementation (or
alternatively remove those keys from the parse map if they should not be
supported); locate the description value near the filter schema definition in
the controller and make the textual list consistent with FilterParser::parse.
| public function getUniqueActivitiesCountBySummit(Summit $summit, Filter $filter = null): int | ||
| { | ||
| // Step 1: collect distinct member IDs matching the summit + filter using the | ||
| // same base query / filter mappings as getSubmittersBySummit. | ||
| $qb = $this->getEntityManager()->createQueryBuilder() | ||
| ->distinct(true) | ||
| ->select("e.id") | ||
| ->from($this->getBaseEntity(), "e") | ||
| ->where(" | ||
| EXISTS ( | ||
| SELECT __p.id FROM models\summit\Presentation __p | ||
| JOIN __p.created_by __cb93 WITH __cb93 = e.id | ||
| WHERE __p.summit = :summit | ||
| )") | ||
| ->setParameter("summit", $summit); | ||
|
|
||
| $qb = $this->applyExtraJoins($qb, $filter); | ||
|
|
||
| if (!is_null($filter)) { | ||
| $filter->apply2Query($qb, $this->getFilterMappings($filter)); | ||
| } | ||
|
|
||
| $memberIds = array_column($qb->getQuery()->getArrayResult(), 'id'); | ||
|
|
||
| if (empty($memberIds)) return 0; | ||
|
|
||
| // Step 2: count distinct presentations submitted by those members. | ||
| $sql = <<<SQL | ||
| SELECT COUNT(DISTINCT p.ID) AS cnt | ||
| FROM Presentation p | ||
| INNER JOIN SummitEvent se ON se.ID = p.ID | ||
| WHERE se.SummitID = :summit_id | ||
| AND p.CreatedByID IN (:member_ids) | ||
| SQL; | ||
|
|
||
| $stm = $this->getEntityManager()->getConnection()->executeQuery( | ||
| $sql, | ||
| ['summit_id' => $summit->getId(), 'member_ids' => $memberIds], | ||
| ['summit_id' => \Doctrine\DBAL\ParameterType::INTEGER, 'member_ids' => \Doctrine\DBAL\ArrayParameterType::INTEGER] | ||
| ); | ||
|
|
||
| return intval($stm->fetchOne()); | ||
| } |
There was a problem hiding this comment.
Loading all member IDs into PHP for the IN clause is a scalability hazard.
Step 1 fetches every distinct matching member ID in one unbounded query (getArrayResult() with no LIMIT). For a large summit with thousands of submitters, this loads a potentially very large array into PHP memory and then constructs a correspondingly long raw-SQL IN (...) list. MySQL (and most RDBMS) experience query-plan degradation at high IN-list cardinalities, and the intermediate PHP array also grows proportionally.
The analogous getSubmittersIdsBySummit and getSubmittersCountBySummit-style methods in this repo are paginated; this method has no safety valve. The same concern applies to the DoctrineSpeakerRepository::getUniqueActivitiesCountBySummit implementation that is not in this review.
For typical summit sizes (hundreds to a few thousand submitters) this is acceptable, but it should be acknowledged as a scale limit. A correlated-subquery approach that stays within the database would eliminate the round-trip:
SELECT COUNT(DISTINCT p.ID) AS cnt
FROM Presentation p
INNER JOIN SummitEvent se ON se.ID = p.ID
WHERE se.SummitID = :summit_id
AND p.CreatedByID IN (
SELECT m.ID
FROM Member m
WHERE EXISTS (
SELECT 1
FROM Presentation p2
WHERE p2.CreatedByID = m.ID
AND p2.SummitID = :summit_id
-- filter conditions translated here
)
)This is high-effort to implement given the complex DQL filter system, but worth tracking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Repositories/Summit/DoctrineMemberRepository.php` around lines 647 - 689,
The current getUniqueActivitiesCountBySummit method loads all matching member
IDs into PHP and uses them in an IN(...) clause which is a scalability hazard;
replace the two-step approach with a single DB-side query (either DQL or raw
SQL) that counts DISTINCT Presentation IDs using a correlated subquery or EXISTS
to restrict p.CreatedByID to members who match the summit+filter, so no IDs are
materialized in PHP. Locate getUniqueActivitiesCountBySummit and remove the
array_column/getArrayResult logic; build a single query that keeps :summit_id as
a parameter and expresses the member filter via an EXISTS (or a subselect from
Member) so you can still reuse the filter mapping logic (applyExtraJoins /
getFilterMappings / Filter::apply2Query) by applying it into the subquery/DQL
where appropriate; preserve parameter typing for summit_id and avoid passing
large arrays to the DB driver.
| ports: !reset | ||
| - "1889:1888" # pprof extension | ||
| - "8890:8888" # Prometheus metrics exposed by the Collector | ||
| - "8891:8889" # Prometheus exporter metrics | ||
| - "13134:13133" # health_check extension | ||
| - "4319:4317" # OTLP gRPC receiver | ||
| - "4320:4318" # OTLP http receiver | ||
| - "55680:55679" # zpages extension | ||
|
|
||
| elasticsearch: | ||
| ports: !reset | ||
| - "9202:9200" | ||
| - "9302:9300" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Docker Compose !reset !override merge tags documentation
💡 Result:
In Docker Compose, when merging multiple compose files (e.g., base compose.yaml and override compose.override.yaml), special YAML tags!reset and!override control how configurations are handled beyond standard merging rules [1][2].!reset: Used in an override file to remove or clear an attribute from the base configuration. It sets the target attribute to its type's default value or null, ignoring the provided value. For readability, use!reset null for scalars or!reset [] for lists/arrays. Example in override file: services: app: ports:!reset [] environment: FOO:!reset null This clears all ports and the FOO environment variable [1][2].!override: Used to fully replace an attribute in the override file, bypassing merge rules. The entire value is substituted. Example: services: app: ports:!override - "8443:443" This replaces any existing ports (e.g., "8080:80") with just the new one, instead of merging [1][2]. These tags apply during multi-file merges, processed in the order specified (e.g., via -f flags). They are part of the official Compose Specification (version 13 as of sources) and supported in Docker Compose [1][2].
Citations:
- 1: https://compose-spec.github.io/compose-spec/13-merge.html
- 2: https://docs.docker.com/reference/compose-file/merge/
🌐 Web query:
Docker Compose v1 vs v2 !reset !override tag support compatibility
💡 Result:
Docker Compose v1 (docker-compose, Python-based, deprecated and removed from official support by 2025 [1][2]) vs v2/v5 (docker compose, Go-based, current as of 2026 [1][2][3]). v1 is no longer supported; migrate to v2+ which uses the Compose Specification and ignores top-level 'version' [1][2][4].!reset and!override tags: Custom YAML tags for merge/override files (compose.override.yaml), supported in v2+ per Compose Specification [3][5].!reset clears/resets an attribute to default/null (e.g., ports:!reset [] [3]);!override fully replaces an attribute, bypassing merge (e.g., ports:!override ["8443:443"] [3]). Used in overrides to remove or replace values like ports, environment [6][3]. Tags are merge directives, removed post-merge (e.g., not in docker compose config output) [7]. Limitation:!reset on environment vars requires mapping syntax (key:!reset null), not short KEY=VAL sequences [6]. Tag support (image/build.tags): Image is a scalar string (e.g., nginx:1.25), overridden directly in overrides [8]. No special 'tag support' difference; v2+ supports long syntax image: {name: nginx, tag: "1.25"} [9][10], but standard override works (image: httpd:2.4 overrides httpd:latest [8]). Build.tags is a list, merges by append unless!override [3][11]. Compatibility: v2+ backward compatible with most v1 files (except format 1 [4]), but use docker compose (no hyphen) [12][13]. Multiple files merge: later override earlier [14]. For image tags, use env vars like ${TAG:-latest} for easy override [15][16].!reset/!override apply to mappings/attributes like image (scalar: full override), ports (list: reset/override), environment [3].
Citations:
- 1: https://bdteo.com/docker-compose-major-changes-since-october-2023/
- 2: https://docs.docker.com/compose/intro/history/
- 3: https://docs.docker.com/reference/compose-file/merge
- 4: https://medium.com/@jerome.devops/the-end-of-docker-compose-e3b7d15330e2
- 5: https://compose-spec.github.io/compose-spec/13-merge.html
- 6: [BUG] override file !reset for environment vars only works if both compose and override use colon-style syntax docker/compose#13600
- 7: [BUG]? !reset & !override tags are not retained in merged configs docker/compose#12162
- 8: https://stackoverflow.com/questions/37111425/overriding-image-version-in-docker-compose-file
- 9: feat: support long syntax for image names and build tags docker/compose#13709
- 10: docker/compose@e776ed4
- 11: https://github.com/compose-spec/compose-spec/blob/main/spec.md
- 12: https://www.docker.com/blog/new-docker-compose-v2-and-v1-deprecation/
- 13: https://docker.com/blog/announcing-compose-v2-general-availability
- 14: https://docs.docker.com/compose/how-tos/multiple-compose-files/merge
- 15: Proposal: Adding Support for Alternative Image Tags in Docker Compose docker/compose#11726
- 16: Request for allowing separate definition of image
tagunderservicesdocker/compose#11743
!reset discards the provided list — use !override to replace ports.
The !reset tag clears an attribute from the base configuration by setting it to its type's default value or null, and the provided value is explicitly ignored. This means the port lists you've specified here would be silently discarded and both services would end up with no port bindings at all, rather than the remapped non-conflicting ports intended by the comment.
To fully replace an attribute's value and bypass standard merge rules, use !override instead. With standard merge semantics (no tag), Compose would append the new ports to the existing ones, exposing both old and new ports.
🔧 Proposed fix
services:
otel-collector:
- ports: !reset
+ ports: !override
- "1889:1888"
- "8890:8888"
- "8891:8889"
- "13134:13133"
- "4319:4317"
- "4320:4318"
- "55680:55679"
elasticsearch:
- ports: !reset
+ ports: !override
- "9202:9200"
- "9302:9300"Both !reset and !override are Docker Compose v2+ tags and are not supported by the legacy docker-compose v1 binary.
📝 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.
| ports: !reset | |
| - "1889:1888" # pprof extension | |
| - "8890:8888" # Prometheus metrics exposed by the Collector | |
| - "8891:8889" # Prometheus exporter metrics | |
| - "13134:13133" # health_check extension | |
| - "4319:4317" # OTLP gRPC receiver | |
| - "4320:4318" # OTLP http receiver | |
| - "55680:55679" # zpages extension | |
| elasticsearch: | |
| ports: !reset | |
| - "9202:9200" | |
| - "9302:9300" | |
| ports: !override | |
| - "1889:1888" # pprof extension | |
| - "8890:8888" # Prometheus metrics exposed by the Collector | |
| - "8891:8889" # Prometheus exporter metrics | |
| - "13134:13133" # health_check extension | |
| - "4319:4317" # OTLP gRPC receiver | |
| - "4320:4318" # OTLP http receiver | |
| - "55680:55679" # zpages extension | |
| elasticsearch: | |
| ports: !override | |
| - "9202:9200" | |
| - "9302:9300" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker-compose.override.testing.yml` around lines 4 - 16, The Compose
override mistakenly uses the YAML tag !reset for the ports lists (the two ports
blocks shown), which clears the base ports instead of replacing them; change the
tag to !override so the provided port mappings replace the base values (or
remove the tag to append if that's desired) for the ports entries shown (the
first ports block with 1889:1888 etc. and the elasticsearch ports block
9202:9200, 9302:9300); also ensure your environment uses Docker Compose v2+ or
remove these tags if you must remain compatible with legacy docker-compose v1.
1 participant
…d submitters
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Configuration