feat(events): fix and complete get-overflow-published-events endpoint#573
feat(events): fix and complete get-overflow-published-events endpoint#573smarcet wants to merge 4 commits into
Conversation
- Scope RetrieveAllOverflowPublishedSummitEventsStrategy to the requesting summit by extending RetrievePublishedSummitEventsBySummitStrategy instead of the non-summit-scoped RetrieveAllPublishedSummitEventsStrategy. - Map 'occupancy' in DoctrineSummitEventRepository::getFilterMappings() so the occupancy==OVERFLOW filter condition actually reaches the SQL query instead of being silently dropped. - Register the missing route (events/all/published/occupancy/overflow) and fix the controller's strategy constructor call, OpenAPI tags bug, the ocuppancy->occupancy typo, and an unused import. - Seed the new endpoint in ApiEndpointsSeeder (fresh installs) and add Version20260715120000 migration to register it for already-deployed environments (k8s), verified up/down/up against the config DB. - Generalize APIEndpointsMigrationHelper with registerEndpoint(s)/ unregisterEndpoint(s) so future endpoint migrations don't need to repeat the insertEndpoint/insertEndpointScope/insertEndpointAuthzGroup boilerplate. - Add a functional test covering the endpoint end-to-end.
📝 WalkthroughWalkthroughChangesThe summit API adds a route for retrieving published overflow-occupancy events. It introduces occupancy filtering, retrieval strategy inheritance, authorization and endpoint registration, uncached overflow serialization, and integration tests. Overflow published events
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuth2SummitEventsApiController
participant RetrieveAllOverflowPublishedSummitEventsStrategy
participant DoctrineSummitEventRepository
participant SerializerUtils
Client->>OAuth2SummitEventsApiController: Request overflow published events
OAuth2SummitEventsApiController->>RetrieveAllOverflowPublishedSummitEventsStrategy: Retrieve summit events
RetrieveAllOverflowPublishedSummitEventsStrategy->>DoctrineSummitEventRepository: Query occupancy overflow
DoctrineSummitEventRepository-->>RetrieveAllOverflowPublishedSummitEventsStrategy: Return matching events
RetrieveAllOverflowPublishedSummitEventsStrategy-->>OAuth2SummitEventsApiController: Return published overflow events
OAuth2SummitEventsApiController->>SerializerUtils: Serialize OverflowStream
SerializerUtils-->>OAuth2SummitEventsApiController: Return serialized payload
OAuth2SummitEventsApiController-->>Client: Return OK response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.2.5)/vendor/psr/cache/src/CacheItemPoolInterface.php ... [truncated 80210 characters] ... /src/Carbon/Traits/StaticOptions.php PHPStan process crashed because it reached configured PHP memory limit: 8192M 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-573/ 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.
🧹 Nitpick comments (2)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php (2)
2914-2920: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
filter[]parameter to OpenAPI documentation.The underlying event strategy supports filtering. Consider exposing the
filter[]parameter in the OpenAPI schema to match the other event list endpoints.💡 Proposed fix
new OA\Parameter(name: 'page', in: 'query', required: false, description: 'Page number', schema: new OA\Schema(type: 'integer', default: 1)), new OA\Parameter(name: 'per_page', in: 'query', required: false, description: 'Items per page', schema: new OA\Schema(type: 'integer', default: 10, maximum: 100)), + new OA\Parameter(name: 'filter[]', in: 'query', required: false, description: 'Filter expressions', style: 'form', explode: true, schema: new OA\Schema(type: 'array', items: new OA\Items(type: 'string'))), new OA\Parameter(name: 'order', in: 'query', required: false, description: 'Order by field(s)', schema: new OA\Schema(type: 'string')), new OA\Parameter(name: 'expand', in: 'query', required: false, description: 'Expand relationships', schema: new OA\Schema(type: 'string')),🤖 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/OAuth2SummitEventsApiController.php` around lines 2914 - 2920, Add the missing filter[] query parameter to the OpenAPI parameters array for the event list endpoint in OAuth2SummitEventsApiController, matching the filter parameter definition used by the other event list endpoints and preserving the existing pagination, ordering, and expansion parameters.
2935-2935: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typographical error in method name.
The controller method name
getOverflowPublishedEventsEventscontains a duplicate "Events" suffix.
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php#L2935-L2935: rename the controller method togetOverflowPublishedEvents.app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php#L2909-L2909: update the OpenAPIoperationIdto match.routes/api_v1.php#L674-L674: update the route definition to reference the corrected method name.tests/oauth2/OAuth2SummitEventsApiTest.php#L2265-L2265: update the action call in the test to use the corrected method name.🤖 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/OAuth2SummitEventsApiController.php` at line 2935, The method getOverflowPublishedEventsEvents contains a duplicated suffix; rename it to getOverflowPublishedEvents and update all references consistently: set the OpenAPI operationId in app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php at lines 2909-2909, update the route in routes/api_v1.php at lines 674-674, and update the test action call in tests/oauth2/OAuth2SummitEventsApiTest.php at lines 2265-2265.
🤖 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.
Nitpick comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php`:
- Around line 2914-2920: Add the missing filter[] query parameter to the OpenAPI
parameters array for the event list endpoint in OAuth2SummitEventsApiController,
matching the filter parameter definition used by the other event list endpoints
and preserving the existing pagination, ordering, and expansion parameters.
- Line 2935: The method getOverflowPublishedEventsEvents contains a duplicated
suffix; rename it to getOverflowPublishedEvents and update all references
consistently: set the OpenAPI operationId in
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php
at lines 2909-2909, update the route in routes/api_v1.php at lines 674-674, and
update the test action call in tests/oauth2/OAuth2SummitEventsApiTest.php at
lines 2265-2265.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a047e9ea-af9e-4caf-a858-54290205d3bf
📒 Files selected for processing (11)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.phpapp/Http/Controllers/Apis/Protected/Summit/Strategies/events/RetrieveAllOverflowPublishedSummitEventsStrategy.phpapp/Http/Controllers/Apis/Protected/Summit/Strategies/events/RetrieveAllPublishedSummitEventsStrategy.phpapp/Http/Controllers/Apis/Protected/Summit/Strategies/events/RetrievePublishedSummitEventsBySummitStrategy.phpapp/Http/Controllers/Apis/Protected/Summit/Strategies/events/RetrieveSummitEventsStrategy.phpapp/Repositories/Summit/DoctrineSummitEventRepository.phpdatabase/migrations/config/APIEndpointsMigrationHelper.phpdatabase/migrations/config/Version20260715120000.phpdatabase/seeders/ApiEndpointsSeeder.phproutes/api_v1.phptests/oauth2/OAuth2SummitEventsApiTest.php
get-overflow-published-events was authorized with the broad ReadSummitData/ReadAllSummitData scopes shared by 36+ unrelated read endpoints. Its payload includes Mux overflow_tokens (signed playback JWTs), which the public single-event overflow endpoint only ever hands out after verifying possession of a per-event unguessable key (SDS: guides/streaming-virtual-and-overflow-guide.md). ClickUp 86baxyy0u explicitly left the scope choice open for the API owner to decide. - Add SummitScopes::ReadOverflowEvents (.../summits/events/overflow/read) - Require it in place of ReadSummitData/ReadAllSummitData on the endpoint's OA security attribute, migration, and seeder (scope checks are OR-based, so the broad scopes had to be replaced, not just supplemented) - Grant the new scope on both mock access tokens in ProtectedApiTestCase so functional tests keep authorizing Also add negative-case coverage to testGetOverflowPublishedEvents: an unpublished overflow event on the same summit, and a published overflow event on a different summit, both proving the published/ summit_id filters correctly exclude them.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-573/ This page is automatically updated on each push to this PR. |
- Rename getOverflowPublishedEventsEvents -> getOverflowPublishedEvents (controller operationId/method, route, and functional test) to drop the duplicated 'Events' in the name. - Document the filter[] query parameter on the OA\Get attribute.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-573/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
database/seeders/ApiScopesSeeder.php (1)
66-70: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDefine one idempotent owner for
ReadOverflowEvents.The seeder and migration both provision the same scope, while rollback unconditionally deletes it. Confirm these lifecycle paths cannot overlap, or make creation and rollback ownership-aware.
database/seeders/ApiScopesSeeder.php#L66-L70: avoid unconditional insertion when the scope already exists.database/migrations/config/Version20260715120000.php#L46-L51: preserve the migration's insert-only-if-missing behavior.database/migrations/config/Version20260715120000.php#L64-L67: only remove the scope if this migration created it.🤖 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 `@database/seeders/ApiScopesSeeder.php` around lines 66 - 70, Make ReadOverflowEvents provisioning idempotent across all listed sites: in database/seeders/ApiScopesSeeder.php lines 66-70, avoid inserting the scope when it already exists; preserve the existing insert-if-missing behavior in database/migrations/config/Version20260715120000.php lines 46-51; and update that migration’s rollback at lines 64-67 to delete the scope only when this migration created it.
🤖 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.
Nitpick comments:
In `@database/seeders/ApiScopesSeeder.php`:
- Around line 66-70: Make ReadOverflowEvents provisioning idempotent across all
listed sites: in database/seeders/ApiScopesSeeder.php lines 66-70, avoid
inserting the scope when it already exists; preserve the existing
insert-if-missing behavior in
database/migrations/config/Version20260715120000.php lines 46-51; and update
that migration’s rollback at lines 64-67 to delete the scope only when this
migration created it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 157db3b9-17a5-4e68-af2b-a92cb4d793fd
📒 Files selected for processing (8)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.phpapp/Security/SummitScopes.phpdatabase/migrations/config/Version20260715120000.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ApiScopesSeeder.phproutes/api_v1.phptests/ProtectedApiTestCase.phptests/oauth2/OAuth2SummitEventsApiTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- routes/api_v1.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php
- database/seeders/ApiEndpointsSeeder.php
- tests/oauth2/OAuth2SummitEventsApiTest.php
SummitEventOverflowStreamingSerializer wrapped its whole payload in RequestCache::cache(), which calls Cache::tags($scope)->add($key, ...) with no TTL - Laravel's Repository::add()/put() treat a null TTL as "store forever". The Mux playback JWTs inside overflow_tokens carry their own exp claim (SummitEvent::JWT_TTL, 6h) and are already cached with a correct TTL one layer down in StreamableEventTrait::getStreamingTokens(). The outer wrapper only cached cheap in-memory getters on top of that, but froze the whole response - including stale tokens - forever once warmed, with no periodic refresh path. This was contained before the getOverflowPublishedEvents endpoint started reusing the serializer in bulk as a cold-start reconcile endpoint (PR #573), where an overflow event left untouched by an admin for longer than the JWT's lifetime would keep serving dead tokens indefinitely. Remove the outer cache: the inner token cache already has correct TTL semantics, and every other field is a free property access on an already-hydrated entity, so nothing meaningful was being saved.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-573/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ModelSerializers/Summit/SummitEventOverflowStreamingSerializer.php`:
- Around line 38-39: Update the start_date and end_date assignments in
SummitEventOverflowStreamingSerializer to use null-safe timestamp access on the
values returned by getStartDate() and getEndDate(), preserving null when either
date is unavailable.
🪄 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: d61677c6-9289-4348-95f8-4a109e0a32dc
📒 Files selected for processing (2)
app/ModelSerializers/Summit/SummitEventOverflowStreamingSerializer.phptests/SerializerTests.php
| $values['start_date'] = $event->getStartDate()->getTimestamp(); | ||
| $values['end_date'] = $event->getEndDate()->getTimestamp(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent fatal errors on null dates.
getStartDate() and getEndDate() can return null if the event type does not allow publishing dates. Calling ->getTimestamp() on null will cause a fatal error. Use the nullsafe operator (?->) to handle potentially missing dates gracefully.
🛡️ Proposed fix
- $values['start_date'] = $event->getStartDate()->getTimestamp();
- $values['end_date'] = $event->getEndDate()->getTimestamp();
+ $values['start_date'] = $event->getStartDate()?->getTimestamp();
+ $values['end_date'] = $event->getEndDate()?->getTimestamp();📝 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.
| $values['start_date'] = $event->getStartDate()->getTimestamp(); | |
| $values['end_date'] = $event->getEndDate()->getTimestamp(); | |
| $values['start_date'] = $event->getStartDate()?->getTimestamp(); | |
| $values['end_date'] = $event->getEndDate()?->getTimestamp(); |
🤖 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/SummitEventOverflowStreamingSerializer.php`
around lines 38 - 39, Update the start_date and end_date assignments in
SummitEventOverflowStreamingSerializer to use null-safe timestamp access on the
values returned by getStartDate() and getEndDate(), preserving null when either
date is unavailable.
ref: https://app.clickup.com/t/9014802374/86baxyy0u
Summary
Completes and fixes the
GET /api/v1/summits/{id}/events/all/published/occupancy/overflowendpoint, which returns published events withoccupancy == OVERFLOWfor a given summit.Security hardening (post-review)
The endpoint's response reuses
SummitEventOverflowStreamingSerializer, which includesoverflow_tokens(signed Mux playback JWTs) — by design per ClickUp 86baxyy0u, which proposes this as an authenticated reconcile endpoint for cold-started clients. That ticket explicitly left the authorization scope as an open API-owner decision ("whether it should be attendee-gated or plain auth.user"). This PR originally answered that with the broadReadSummitData/ReadAllSummitDatascopes, shared by 36+ unrelated read endpoints — any client holding either one could pull live playback tokens for every overflow event in a summit in a single call.Resolved by adding a dedicated scope instead:
SummitScopes::ReadOverflowEvents(.../summits/events/overflow/read).securityattribute, migration, and seeder updated in lockstep — scope checks are OR-based, so the broad scopes had to be replaced, not just supplemented).Version20260715120000now also inserts theapi_scopesrow for the new scope before associating it with the endpoint (reversible indown()).Deploy action required:
Version20260715120000only registers the scope + endpoint in summit-api's own API/scope catalog (config DB) — it does not grant the scope to any OAuth2 client. Any client application that needs to call this endpoint must be explicitly grantedSummitScopes::ReadOverflowEvents(.../summits/events/overflow/read) on its OAuth2 client record (IDP-side) before/at deploy. HoldingReadSummitDataorReadAllSummitDataalone is no longer sufficient — those clients will get403 Insufficient Scopeon this endpoint after this ships.Deployment
ApiEndpointsSeeder(fresh installs), gated byReadOverflowEvents.Version20260715120000migration to register the scope + endpoint (name, route, scope) for already-deployed environments, including k8s. Verified up/down/up against the config DB.APIEndpointsMigrationHelperwithregisterEndpoint(s)/unregisterEndpoint(s)composite helpers so future endpoint migrations need one call per endpoint instead of repeating insert/scope/authz-group boilerplate.Testing
testGetOverflowPublishedEventsextended with negative cases: an unpublished overflow event on the same summit, and a published overflow event on a different summit, proving thepublished/summit_idfilters correctly exclude both.OAuth2SummitEventsApiTest.phpsuite: 56 tests, 127 assertions, 0 failures.OAuth2PresentationApiTest.php(shares the modified filter-mapping code path): 42 tests, 0 failures.Summary by CodeRabbit
GET /api/v1/summits/{id}/events/all/published/occupancy/overflowendpoint to retrieve overflow occupancy published events.occupancyfiltering support for summit event queries.