feat: add new endpoint to retrieve member by external id#526
feat: add new endpoint to retrieve member by external id#526
Conversation
GET /api/v1/members/external/{external_id}
📝 WalkthroughWalkthroughA new OAuth2-protected API endpoint is added to retrieve members by external ID. The implementation includes a controller method, database migration, seeder configuration, and route definition with appropriate authorization constraints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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-526/ 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 `@app/Http/Controllers/Apis/Protected/Main/OAuth2MembersApiController.php`:
- Around line 539-544: The code is serializing a fetched member with
SerializerRegistry::SerializerType_Private which exposes self-profile fields;
change the serializer to use the public/admin policy instead (e.g., replace
SerializerRegistry::SerializerType_Private with
SerializerRegistry::SerializerType_Public to match getById) in
OAuth2MembersApiController where getSerializer($member, ...) is called so
cross-member lookups don't use private serialization.
In `@routes/api_v1.php`:
- Around line 27-30: The route parameter {external_id} is unconstrained and
should be limited to digits to reject invalid paths before controller handling;
update the routing to add a numeric where constraint (e.g. apply
where('external_id', '[0-9]+') either on the inner Route::group for the
'{external_id}' prefix or chain ->where(...) on the Route::get) so that
OAuth2MembersApiController@getMemberByIdExternalId only receives numeric
external_id values.
🪄 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: c306c054-d5b7-4f23-b045-04aee98eedbe
📒 Files selected for processing (4)
app/Http/Controllers/Apis/Protected/Main/OAuth2MembersApiController.phpdatabase/migrations/config/Version20260410172200.phpdatabase/seeders/ApiEndpointsSeeder.phproutes/api_v1.php
| return $this->ok(SerializerRegistry::getInstance()->getSerializer($member, SerializerRegistry::SerializerType_Private)->serialize | ||
| ( | ||
| SerializerUtils::getExpand(), | ||
| SerializerUtils::getFields(), | ||
| SerializerUtils::getRelations() | ||
| )); |
There was a problem hiding this comment.
Use admin/public serialization policy, not private serialization, for cross-member lookup.
This endpoint fetches another member record, but it currently serializes with SerializerType_Private (self-profile level). That is inconsistent with getById and can overexpose member fields.
Suggested change
- return $this->ok(SerializerRegistry::getInstance()->getSerializer($member, SerializerRegistry::SerializerType_Private)->serialize
+ return $this->ok(SerializerRegistry::getInstance()->getSerializer($member, SerializerRegistry::SerializerType_Admin)->serialize
(
SerializerUtils::getExpand(),
SerializerUtils::getFields(),
SerializerUtils::getRelations()
));📝 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.
| return $this->ok(SerializerRegistry::getInstance()->getSerializer($member, SerializerRegistry::SerializerType_Private)->serialize | |
| ( | |
| SerializerUtils::getExpand(), | |
| SerializerUtils::getFields(), | |
| SerializerUtils::getRelations() | |
| )); | |
| return $this->ok(SerializerRegistry::getInstance()->getSerializer($member, SerializerRegistry::SerializerType_Admin)->serialize | |
| ( | |
| SerializerUtils::getExpand(), | |
| SerializerUtils::getFields(), | |
| SerializerUtils::getRelations() | |
| )); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Http/Controllers/Apis/Protected/Main/OAuth2MembersApiController.php`
around lines 539 - 544, The code is serializing a fetched member with
SerializerRegistry::SerializerType_Private which exposes self-profile fields;
change the serializer to use the public/admin policy instead (e.g., replace
SerializerRegistry::SerializerType_Private with
SerializerRegistry::SerializerType_Public to match getById) in
OAuth2MembersApiController where getSerializer($member, ...) is called so
cross-member lookups don't use private serialization.
| Route::group(['prefix' => 'external'], function () { | ||
| Route::group(['prefix' => '{external_id}'], function () { | ||
| Route::get('', ['middleware' => 'auth.user', 'uses' => 'OAuth2MembersApiController@getMemberByIdExternalId']); | ||
| }); |
There was a problem hiding this comment.
Constrain external_id to numeric values at the routing boundary.
{external_id} is currently unconstrained. Add a where clause so invalid path values are rejected before hitting the controller.
Suggested change
- Route::group(['prefix' => '{external_id}'], function () {
+ Route::group(['prefix' => '{external_id}', 'where' => ['external_id' => '[0-9]+']], function () {
Route::get('', ['middleware' => 'auth.user', 'uses' => 'OAuth2MembersApiController@getMemberByIdExternalId']);
});📝 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.
| Route::group(['prefix' => 'external'], function () { | |
| Route::group(['prefix' => '{external_id}'], function () { | |
| Route::get('', ['middleware' => 'auth.user', 'uses' => 'OAuth2MembersApiController@getMemberByIdExternalId']); | |
| }); | |
| Route::group(['prefix' => 'external'], function () { | |
| Route::group(['prefix' => '{external_id}', 'where' => ['external_id' => '[0-9]+']], function () { | |
| Route::get('', ['middleware' => 'auth.user', 'uses' => 'OAuth2MembersApiController@getMemberByIdExternalId']); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@routes/api_v1.php` around lines 27 - 30, The route parameter {external_id} is
unconstrained and should be limited to digits to reject invalid paths before
controller handling; update the routing to add a numeric where constraint (e.g.
apply where('external_id', '[0-9]+') either on the inner Route::group for the
'{external_id}' prefix or chain ->where(...) on the Route::get) so that
OAuth2MembersApiController@getMemberByIdExternalId only receives numeric
external_id values.
1 participant
GET /api/v1/members/external/{external_id}
Summary by CodeRabbit