feat: dropbox_sync_enabled flag + materializer proxy routes#520
Closed
caseylocker wants to merge 3 commits intomainfrom
Closed
feat: dropbox_sync_enabled flag + materializer proxy routes#520caseylocker wants to merge 3 commits intomainfrom
caseylocker wants to merge 3 commits intomainfrom
Conversation
Add Summit model flag to control Dropbox sync activation per-summit, and proxy routes that forward admin requests to the dropbox-materializer microservice through Summit API's OAuth2 auth layer. - Doctrine migration for DropboxSyncEnabled column - Summit model property, serializer, validation, factory - DropboxMaterializerApi Guzzle client with HMAC X-Internal-Key auth - Controller with admin permission checks and sync-enabled gating - Routes: materialize, materializeRoom, backfill, rebuild, preflight, status Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Dropbox Materializer integration: new API client and interface, service binding, OAuth2-protected controller and routes, Summit model flag plus serializer/factory/validation updates, config/env entries, and a migration/schema change to add the flag. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as OAuth2Summit<br/>DropboxSyncApiController
participant ResourceServer as Resource<br/>ServerContext
participant SummitRepo as Summit<br/>Repository
participant Materializer as Dropbox<br/>MaterializerAPI
Client->>Controller: POST /summits/{id}/dropbox-sync/materialize
Controller->>ResourceServer: getCurrentUser()
ResourceServer-->>Controller: user
Controller->>SummitRepo: findById(summit_id)
SummitRepo-->>Controller: summit
Controller->>Controller: checkAdminPermission / requireSyncEnabled
Controller->>Materializer: materialize(summit_id)
Materializer-->>Controller: result (JSON)
Controller-->>Client: 200 JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-520/ This page is automatically updated on each push to this PR. |
…ons.sql Doctrine ORM reads column annotations at migration time, so the column must exist in the initial schema to avoid 'Unknown column' errors when earlier migrations touch the Summit entity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-520/ 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: 7
🧹 Nitpick comments (1)
config/dropbox_materializer.php (1)
6-6: Normalize timeout to a safe numeric value.
env(...)values are stringly typed; coercing/clampingtimeoutavoids accidental invalid or zero timeout behavior.♻️ Proposed fix
- 'timeout' => env('DROPBOX_MATERIALIZER_TIMEOUT', 10), + 'timeout' => max(1, (int) env('DROPBOX_MATERIALIZER_TIMEOUT', 10)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/dropbox_materializer.php` at line 6, The timeout config value currently reads from env('DROPBOX_MATERIALIZER_TIMEOUT', 10) which can be a string; coerce and clamp it to a safe numeric value by parsing the environment value (e.g., using intval or (int)), ensure a minimum positive value (e.g., max(parsed, 1)) and use the default only if parsing fails; update the 'timeout' entry (the key 'timeout' in config/dropbox_materializer.php) to perform this cast-and-clamp so the application always receives a valid numeric timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 271-273: The dotenv-linter warning is caused by the ordering of
the new environment keys; reorder the DROPBOX_MATERIALIZER_* variables to match
the repository's dotenv key-order convention (alphabetical within the block) so
the keys DROPBOX_MATERIALIZER_KEY, DROPBOX_MATERIALIZER_TIMEOUT, and
DROPBOX_MATERIALIZER_URL appear in the correct sorted order; update the
.env.example entry for these three variables accordingly.
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitDropboxSyncApiController.php`:
- Around line 94-100: The controller currently coerces route params using intval
(e.g. in the block calling findSummit, checkAdminPermission, requireSyncEnabled
and materializer_api->materialize) which accepts malformed IDs like "1foo";
replace the coercion with strict validation: verify the incoming $summit_id (and
other route IDs in the other handlers) are pure integers (e.g. ctype_digit or
filter_var with FILTER_VALIDATE_INT) and return a 400/validation error if not,
then pass the validated int to findSummit (and similar methods) instead of
intval; alternatively add numeric route constraints so these handlers never
receive malformed IDs.
- Around line 57-64: checkAdminPermission currently allows a null member to pass
due to the compound condition; explicitly reject a missing user by checking
resource_server_context->getCurrentUser() and throwing an AuthzException if it
is null before evaluating isAdmin() or hasPermissionForOnGroup; update
checkAdminPermission (the method name) to first set $current_member =
$this->resource_server_context->getCurrentUser() and immediately throw a clear
AuthzException (same type used elsewhere) when $current_member is null, then
continue with the existing admin and IGroup::SummitAdministrators permission
checks.
In `@app/Services/Apis/DropboxMaterializerApi.php`:
- Around line 50-60: In DropboxMaterializerApi, don't allow an empty internal
key: after retrieving $this->internal_key =
Config::get('dropbox_materializer.internal_key', '') in the constructor,
validate that it's non-empty and throw a clear exception (e.g.,
InvalidArgumentException or RuntimeException) if missing so the app fails fast
on misconfiguration; keep the headers() method as-is to use $this->internal_key
once validated.
- Around line 70-97: The request() method in DropboxMaterializerApi can drop the
upstream HTTP status when decoding an error response body; modify the
catch(RequestException $ex) branch so that when $response exists and
json_decode($body, true) yields $decoded (even if truthy), you always attach or
override a 'status' key with $response->getStatusCode() before returning; keep
the existing fallback return values for when there is no response or decoding
fails so callers always receive an array that includes the upstream HTTP status.
- Around line 39-40: The retry middleware currently created with
GuzzleRetryMiddleware::factory() will retry POSTs (causing duplicate side
effects); update the creation/configuration of GuzzleRetryMiddleware so it
excludes non-idempotent methods (at minimum exclude "POST") or only allows
retries for safe/idempotent methods (e.g., GET/HEAD/PUT/DELETE/OPTIONS). Locate
the HandlerStack usage and replace the plain GuzzleRetryMiddleware::factory()
call (the $stack->push line and surrounding code that configures $stack) with a
configured factory/instance that sets allowed retry methods or a retry predicate
that returns false for POST and other non-idempotent operations used by
materialize/backfill/rebuild.
In `@app/Services/BaseServicesProvider.php`:
- Around line 145-148: Add IDropboxMaterializerApi::class to the service
provider's provides() method so the deferred binding is discoverable: update the
provides() implementation to return/include the IDropboxMaterializerApi::class
entry (matching the App::singleton registration of
DropboxMaterializerApi::class) while leaving protected $defer = true intact,
ensuring the container can resolve the binding when deferred.
---
Nitpick comments:
In `@config/dropbox_materializer.php`:
- Line 6: The timeout config value currently reads from
env('DROPBOX_MATERIALIZER_TIMEOUT', 10) which can be a string; coerce and clamp
it to a safe numeric value by parsing the environment value (e.g., using intval
or (int)), ensure a minimum positive value (e.g., max(parsed, 1)) and use the
default only if parsing fails; update the 'timeout' entry (the key 'timeout' in
config/dropbox_materializer.php) to perform this cast-and-clamp so the
application always receives a valid numeric timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd5b2978-9f2a-4e73-9953-25e9418e27fb
📒 Files selected for processing (12)
.env.exampleapp/Http/Controllers/Apis/Protected/Summit/Factories/SummitValidationRulesFactory.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitDropboxSyncApiController.phpapp/ModelSerializers/Summit/SummitSerializer.phpapp/Models/Foundation/Summit/Factories/SummitFactory.phpapp/Models/Foundation/Summit/Summit.phpapp/Services/Apis/DropboxMaterializerApi.phpapp/Services/Apis/IDropboxMaterializerApi.phpapp/Services/BaseServicesProvider.phpconfig/dropbox_materializer.phpdatabase/migrations/model/Version20260318120000.phproutes/api_v1.php
Comment on lines
+271
to
+273
| DROPBOX_MATERIALIZER_URL=http://localhost:8100 | ||
| DROPBOX_MATERIALIZER_KEY= | ||
| DROPBOX_MATERIALIZER_TIMEOUT=10 |
There was a problem hiding this comment.
Resolve dotenv key-order warnings for the new materializer vars.
dotenv-linter flags the current order; this can create avoidable CI noise.
♻️ Proposed fix
-DROPBOX_MATERIALIZER_URL=http://localhost:8100
DROPBOX_MATERIALIZER_KEY=
DROPBOX_MATERIALIZER_TIMEOUT=10
+DROPBOX_MATERIALIZER_URL=http://localhost:8100📝 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
| DROPBOX_MATERIALIZER_URL=http://localhost:8100 | |
| DROPBOX_MATERIALIZER_KEY= | |
| DROPBOX_MATERIALIZER_TIMEOUT=10 | |
| DROPBOX_MATERIALIZER_KEY= | |
| DROPBOX_MATERIALIZER_TIMEOUT=10 | |
| DROPBOX_MATERIALIZER_URL=http://localhost:8100 |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 272-272: [UnorderedKey] The DROPBOX_MATERIALIZER_KEY key should go before the DROPBOX_MATERIALIZER_URL key
(UnorderedKey)
[warning] 273-273: [UnorderedKey] The DROPBOX_MATERIALIZER_TIMEOUT key should go before the DROPBOX_MATERIALIZER_URL key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 271 - 273, The dotenv-linter warning is caused by
the ordering of the new environment keys; reorder the DROPBOX_MATERIALIZER_*
variables to match the repository's dotenv key-order convention (alphabetical
within the block) so the keys DROPBOX_MATERIALIZER_KEY,
DROPBOX_MATERIALIZER_TIMEOUT, and DROPBOX_MATERIALIZER_URL appear in the correct
sorted order; update the .env.example entry for these three variables
accordingly.
Comment on lines
+57
to
+64
| private function checkAdminPermission(Summit $summit): void | ||
| { | ||
| $current_member = $this->resource_server_context->getCurrentUser(); | ||
| if (!is_null($current_member) && !$current_member->isAdmin() && !$current_member->hasPermissionForOnGroup($summit, IGroup::SummitAdministrators)) | ||
| throw new AuthzException( | ||
| sprintf("Member %s has not permission for this Summit", $current_member->getId()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fail-closed authorization required when member context is missing.
The checkAdminPermission() method has a fail-open vulnerability: when getCurrentUser() returns null, the compound condition evaluates to false, allowing the request to proceed without any authorization check. This bypasses the admin and group permission validation entirely. Reject null context upfront with a dedicated check.
Suggested fix
private function checkAdminPermission(Summit $summit): void
{
$current_member = $this->resource_server_context->getCurrentUser();
+ if (is_null($current_member)) {
+ throw new AuthzException('A current member context is required.');
+ }
- if (!is_null($current_member) && !$current_member->isAdmin() && !$current_member->hasPermissionForOnGroup($summit, IGroup::SummitAdministrators))
+ if (!$current_member->isAdmin() && !$current_member->hasPermissionForOnGroup($summit, IGroup::SummitAdministrators))
throw new AuthzException(
sprintf("Member %s has not permission for this Summit", $current_member->getId())
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitDropboxSyncApiController.php`
around lines 57 - 64, checkAdminPermission currently allows a null member to
pass due to the compound condition; explicitly reject a missing user by checking
resource_server_context->getCurrentUser() and throwing an AuthzException if it
is null before evaluating isAdmin() or hasPermissionForOnGroup; update
checkAdminPermission (the method name) to first set $current_member =
$this->resource_server_context->getCurrentUser() and immediately throw a clear
AuthzException (same type used elsewhere) when $current_member is null, then
continue with the existing admin and IGroup::SummitAdministrators permission
checks.
Comment on lines
+94
to
+100
| return $this->processRequest(function () use ($summit_id) { | ||
| $summit = $this->findSummit(intval($summit_id)); | ||
| $this->checkAdminPermission($summit); | ||
| $this->requireSyncEnabled($summit); | ||
|
|
||
| $result = $this->materializer_api->materialize($summit->getId()); | ||
| return $this->ok($result); |
There was a problem hiding this comment.
Reject malformed IDs instead of coercing them.
All handlers use intval() on route parameters. Values like 1foo become 1, so malformed admin requests can target the wrong summit, venue, or room instead of failing fast. Add numeric route constraints or strict validation before lookup.
One way to harden the parsing
+ private function parsePositiveInt($value, string $field): int
+ {
+ $value = (string)$value;
+ if (!ctype_digit($value)) {
+ throw new ValidationException(sprintf('%s must be a positive integer.', $field));
+ }
+ return (int)$value;
+ }
...
- $summit = $this->findSummit(intval($summit_id));
+ $summit = $this->findSummit($this->parsePositiveInt($summit_id, 'summit_id'));Also applies to: 109-125, 136-142, 151-156, 165-170, 179-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitDropboxSyncApiController.php`
around lines 94 - 100, The controller currently coerces route params using
intval (e.g. in the block calling findSummit, checkAdminPermission,
requireSyncEnabled and materializer_api->materialize) which accepts malformed
IDs like "1foo"; replace the coercion with strict validation: verify the
incoming $summit_id (and other route IDs in the other handlers) are pure
integers (e.g. ctype_digit or filter_var with FILTER_VALIDATE_INT) and return a
400/validation error if not, then pass the validated int to findSummit (and
similar methods) instead of intval; alternatively add numeric route constraints
so these handlers never receive malformed IDs.
Comment on lines
+50
to
+60
| $this->internal_key = Config::get('dropbox_materializer.internal_key', ''); | ||
| } | ||
|
|
||
| /** | ||
| * @return array | ||
| */ | ||
| private function headers(): array | ||
| { | ||
| return [ | ||
| 'X-Internal-Key' => $this->internal_key, | ||
| 'Accept' => 'application/json', |
There was a problem hiding this comment.
Fail fast when DROPBOX_MATERIALIZER_KEY is missing.
Allowing an empty key at Line 50 weakens internal-auth posture and makes misconfiguration harder to detect.
🔐 Proposed fix
$this->internal_key = Config::get('dropbox_materializer.internal_key', '');
+ if (trim($this->internal_key) === '') {
+ throw new \InvalidArgumentException('dropbox_materializer.internal_key is required.');
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Services/Apis/DropboxMaterializerApi.php` around lines 50 - 60, In
DropboxMaterializerApi, don't allow an empty internal key: after retrieving
$this->internal_key = Config::get('dropbox_materializer.internal_key', '') in
the constructor, validate that it's non-empty and throw a clear exception (e.g.,
InvalidArgumentException or RuntimeException) if missing so the app fails fast
on misconfiguration; keep the headers() method as-is to use $this->internal_key
once validated.
Comment on lines
+70
to
+97
| private function request(string $method, string $uri): array | ||
| { | ||
| try { | ||
| $response = $this->client->request($method, $uri, [ | ||
| 'headers' => $this->headers(), | ||
| ]); | ||
|
|
||
| $body = $response->getBody()->getContents(); | ||
| return json_decode($body, true) ?? []; | ||
| } catch (RequestException $ex) { | ||
| Log::warning( | ||
| sprintf( | ||
| "DropboxMaterializerApi::request %s %s error: %s", | ||
| $method, | ||
| $uri, | ||
| $ex->getMessage() | ||
| ) | ||
| ); | ||
|
|
||
| $response = $ex->getResponse(); | ||
| if ($response) { | ||
| $body = $response->getBody()->getContents(); | ||
| $decoded = json_decode($body, true); | ||
| return $decoded ?? ['error' => $ex->getMessage(), 'status' => $response->getStatusCode()]; | ||
| } | ||
|
|
||
| return ['error' => $ex->getMessage()]; | ||
| } |
There was a problem hiding this comment.
Preserve upstream HTTP status in the proxy contract.
At Line 93, when upstream error JSON is returned, status can be lost. Combined with controller ok(...) wrappers, upstream failures can be surfaced as 200 responses.
🩹 Proposed direction
- private function request(string $method, string $uri): array
+ private function request(string $method, string $uri): array
{
try {
$response = $this->client->request($method, $uri, [
'headers' => $this->headers(),
]);
$body = $response->getBody()->getContents();
- return json_decode($body, true) ?? [];
+ return [
+ 'status' => $response->getStatusCode(),
+ 'body' => json_decode($body, true) ?? [],
+ ];
} catch (RequestException $ex) {
@@
$response = $ex->getResponse();
if ($response) {
$body = $response->getBody()->getContents();
$decoded = json_decode($body, true);
- return $decoded ?? ['error' => $ex->getMessage(), 'status' => $response->getStatusCode()];
+ return [
+ 'status' => $response->getStatusCode(),
+ 'body' => $decoded ?? ['error' => $ex->getMessage()],
+ ];
}
- return ['error' => $ex->getMessage()];
+ return ['status' => 0, 'body' => ['error' => $ex->getMessage()]];
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Services/Apis/DropboxMaterializerApi.php` around lines 70 - 97, The
request() method in DropboxMaterializerApi can drop the upstream HTTP status
when decoding an error response body; modify the catch(RequestException $ex)
branch so that when $response exists and json_decode($body, true) yields
$decoded (even if truthy), you always attach or override a 'status' key with
$response->getStatusCode() before returning; keep the existing fallback return
values for when there is no response or decoding fails so callers always receive
an array that includes the upstream HTTP status.
- Register in deferred provider's provides() so DI resolution works - Limit GuzzleRetryMiddleware to GET requests only (POST endpoints are idempotent but no reason to retry them on 429/503) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-520/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/Services/Apis/DropboxMaterializerApi.php (2)
91-96:⚠️ Potential issue | 🟠 MajorPreserve upstream HTTP status even when error JSON decodes.
At Line 95, if
$decodedis non-null, upstream status is dropped. With controllerok(...)wrappers, this can mask upstream failures.🩹 Proposed fix
$response = $ex->getResponse(); if ($response) { $body = $response->getBody()->getContents(); $decoded = json_decode($body, true); - return $decoded ?? ['error' => $ex->getMessage(), 'status' => $response->getStatusCode()]; + if (is_array($decoded)) { + $decoded['status'] = $response->getStatusCode(); + return $decoded; + } + return ['error' => $ex->getMessage(), 'status' => $response->getStatusCode()]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Apis/DropboxMaterializerApi.php` around lines 91 - 96, The current exception handling in DropboxMaterializerApi (when $ex->getResponse() returns $response) decodes the response body into $decoded but drops the upstream HTTP status; change the return so the upstream status code is always included alongside the decoded payload (e.g., merge or wrap $decoded with a 'status' => $response->getStatusCode()), so callers receive both the error body and the original status even when json_decode($body, true) returns non-null; update the return logic around $response->getBody()->getContents() and json_decode to ensure status preservation.
52-53:⚠️ Potential issue | 🟠 MajorFail fast when internal key is missing.
At Line 52, an empty
dropbox_materializer.internal_keyis still accepted. That weakens internal auth and hides misconfiguration until runtime.🔐 Proposed fix
$this->internal_key = Config::get('dropbox_materializer.internal_key', ''); + if (trim($this->internal_key) === '') { + throw new \InvalidArgumentException('dropbox_materializer.internal_key is required.'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/Apis/DropboxMaterializerApi.php` around lines 52 - 53, The constructor in DropboxMaterializerApi currently accepts an empty internal key from Config::get('dropbox_materializer.internal_key', ''), so update the constructor (class DropboxMaterializerApi) to fail fast: after assigning $this->internal_key, validate it is non-empty and if missing throw a clear exception (e.g., RuntimeException or InvalidArgumentException) with a message indicating the missing dropbox_materializer.internal_key so the app fails at startup rather than allowing silent misconfiguration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Services/Apis/DropboxMaterializerApi.php`:
- Around line 79-80: The json_decode(..., true) results in request methods
(e.g., the method that uses $response->getBody()->getContents()) may return
non-array scalar values and violate the declared array return type; after
decoding (at the blocks around the $body = $response->getBody()->getContents();
and the other block at 93-95) assign the result to a variable (e.g., $data =
json_decode($body, true)), then return is_array($data) ? $data : [] to guarantee
an array is always returned and avoid TypeError from the declared array return
type.
---
Duplicate comments:
In `@app/Services/Apis/DropboxMaterializerApi.php`:
- Around line 91-96: The current exception handling in DropboxMaterializerApi
(when $ex->getResponse() returns $response) decodes the response body into
$decoded but drops the upstream HTTP status; change the return so the upstream
status code is always included alongside the decoded payload (e.g., merge or
wrap $decoded with a 'status' => $response->getStatusCode()), so callers receive
both the error body and the original status even when json_decode($body, true)
returns non-null; update the return logic around
$response->getBody()->getContents() and json_decode to ensure status
preservation.
- Around line 52-53: The constructor in DropboxMaterializerApi currently accepts
an empty internal key from Config::get('dropbox_materializer.internal_key', ''),
so update the constructor (class DropboxMaterializerApi) to fail fast: after
assigning $this->internal_key, validate it is non-empty and if missing throw a
clear exception (e.g., RuntimeException or InvalidArgumentException) with a
message indicating the missing dropbox_materializer.internal_key so the app
fails at startup rather than allowing silent misconfiguration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e3fd3e2-cbf2-4afa-af4c-730e4c7f2f97
📒 Files selected for processing (2)
app/Services/Apis/DropboxMaterializerApi.phpapp/Services/BaseServicesProvider.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Services/BaseServicesProvider.php
Comment on lines
+79
to
+80
| $body = $response->getBody()->getContents(); | ||
| return json_decode($body, true) ?? []; |
There was a problem hiding this comment.
Guard json_decode result type to satisfy declared array return type.
json_decode(..., true) can return scalar values; returning that directly can trigger a runtime TypeError against request(...): array.
🛠️ Proposed fix
$body = $response->getBody()->getContents();
- return json_decode($body, true) ?? [];
+ $decoded = json_decode($body, true);
+ return is_array($decoded) ? $decoded : [];
@@
$body = $response->getBody()->getContents();
$decoded = json_decode($body, true);
- return $decoded ?? ['error' => $ex->getMessage(), 'status' => $response->getStatusCode()];
+ return is_array($decoded)
+ ? $decoded
+ : ['error' => $ex->getMessage(), 'status' => $response->getStatusCode()];Also applies to: 93-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Services/Apis/DropboxMaterializerApi.php` around lines 79 - 80, The
json_decode(..., true) results in request methods (e.g., the method that uses
$response->getBody()->getContents()) may return non-array scalar values and
violate the declared array return type; after decoding (at the blocks around the
$body = $response->getBody()->getContents(); and the other block at 93-95)
assign the result to a variable (e.g., $data = json_decode($body, true)), then
return is_array($data) ? $data : [] to guarantee an array is always returned and
avoid TypeError from the declared array return type.
Contributor
|
@caseylocker I thought the idea was to not touch the Summit API and have these end points exist in the Materializer microservice that would get called by show admin. |
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
2 participants
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.
Summary
DropboxSyncEnabledboolean flag to Summit model (Doctrine migration, ORM column, serializer, validation rules, factory)DropboxMaterializerApiGuzzle service class that proxies admin requests to the dropbox-materializer microservice using HMACX-Internal-KeyauthOAuth2SummitDropboxSyncApiControllerwith 6 proxy endpoints under/api/v1/summits/{id}/dropbox-sync/materialize,materializeRoom,backfillare gated ondropbox_sync_enabled(422 if disabled)rebuild,preflight,statuswork regardless of flag (admin diagnostic tools)materializeRoomresolves venue/room IDs to names server-side before forwardingNew routes
summits/{id}/dropbox-sync/materializesummits/{id}/dropbox-sync/materialize/{location_id}/{room_id}summits/{id}/dropbox-sync/backfillsummits/{id}/dropbox-sync/rebuildsummits/{id}/dropbox-sync/preflightsummits/{id}/dropbox-sync/statusNew env vars
Related
docs/cross-repo-integration.md(in materializer repo)Test plan
php artisan doctrine:migrations:migratedropbox_sync_enabledappears inGET /api/v1/summits/{id}response asfalsePUT /api/v1/summits/{id}with{"dropbox_sync_enabled": true}— should persistPOST .../dropbox-sync/materializewithdropbox_sync_enabled=falseGET .../dropbox-sync/statusworks regardless of flagphp artisan route:list | grep dropbox🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration
Data & Serialization