-
Notifications
You must be signed in to change notification settings - Fork 3
Add reuse_prototype field to /wiki API endpoint
#1082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 14 commits
c43c6a4
73a92e7
07bb19b
9233389
69da736
a06042e
e7a36ef
84f1b51
a5e0f75
5c931e2
bdd52e8
0657731
a993cba
e48435f
0d93d7f
e937b5d
2eaab8b
16c8b67
fd0f8a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,14 @@ public function toArray($request): array { | |
| 'sitename' => $this->sitename, | ||
| 'wiki_site_stats' => $this->wikiSiteStats, | ||
| 'logo_url' => $logoSetting ? $logoSetting->value : null, | ||
|
|
||
| // Checking relation load state before reading it to avoid N+1 query | ||
| // This relies on the controller to eager load `wikiLatestProfile` relationship | ||
| 'reuse_prototype' => $this->wikiLatestProfile | ||
| ? $this->wikiLatestProfile->purpose === 'data_hub' | ||
| && $this->wikiLatestProfile->temporality === 'permanent' | ||
| && $this->wikiLatestProfile->audience === 'wide' | ||
| : null, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the Acceptance Criteria in the task, this should be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Summary:
|
||
| ]; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| use App\Http\Curl\CurlRequest; | ||
| use App\Http\Curl\HttpRequest; | ||
| use Illuminate\Database\Events\QueryExecuted; | ||
| use Illuminate\Queue\Events\JobFailed; | ||
| use Illuminate\Support\Facades\Queue; | ||
| use Illuminate\Support\ServiceProvider; | ||
|
|
@@ -25,5 +26,16 @@ public function boot(): void { | |
| $wrappedException = new \Exception("Executing Job '$name' failed.", 1, $event->exception); | ||
| report($wrappedException); | ||
| }); | ||
|
|
||
| // Local-only SQL query logging for debugging | ||
| if ($this->app->environment('local')) { | ||
| \Event::listen(QueryExecuted::class, function (QueryExecuted $query) { | ||
| \Log::debug('Query Executed: ', [ | ||
| 'sql' => $query->sql, | ||
| 'bindings' => $query->bindings, | ||
| 'connection' => $query->connectionName, | ||
| ]); | ||
| }); | ||
| } | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I got the above error again and tracked it down to:
I don't think it's an issue in staging/production as I don't think logging to a file is enabled.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah I just checked and had the same issue. But you've already specified "local"-only so I don't think it's going to be a problem on staging/prod
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think remove. We can add it when we want to use it. Not useful for day-to-day debugging. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,5 +57,6 @@ | |
| }); | ||
|
|
||
| $router->apiResource('wiki', 'PublicWikiController')->only(['index', 'show']); | ||
| $router->apiResource('reusePrototype', 'PublicWikiController')->only(['index', 'show']); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking we should just enable the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did it and it means I also had to update |
||
| $router->apiResource('wikiConversionData', 'ConversionMetricController')->only(['index']); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,66 @@ | ||||||
| <?php | ||||||
|
|
||||||
| namespace Tests\Http\Controllers; | ||||||
|
|
||||||
| use App\Http\Controllers\PublicWikiController; | ||||||
| use App\Http\Resources\PublicWikiResource; | ||||||
| use App\Wiki; | ||||||
| use App\WikiProfile; | ||||||
| use Illuminate\Database\Events\QueryExecuted; | ||||||
| use Illuminate\Foundation\Testing\DatabaseTransactions; | ||||||
| use Illuminate\Http\Request; | ||||||
| use Illuminate\Support\Facades\DB; | ||||||
| use Tests\TestCase; | ||||||
|
|
||||||
| class PublicWikiControllerTest extends TestCase { | ||||||
| use DatabaseTransactions; | ||||||
|
|
||||||
| public function testShowLoadsWikiLatestProfileForResource(): void { | ||||||
| $wiki = Wiki::factory()->create([ | ||||||
| 'domain' => 'controller-test.wikibase.cloud', | ||||||
| 'sitename' => 'controller-test', | ||||||
| ]); | ||||||
|
|
||||||
| WikiProfile::create([ | ||||||
| 'wiki_id' => $wiki->id, | ||||||
| 'purpose' => 'data_hub', | ||||||
| 'temporality' => 'permanent', | ||||||
| 'audience' => 'wide', | ||||||
| ]); | ||||||
|
|
||||||
| $controller = new PublicWikiController; | ||||||
| $resource = $controller->show($wiki->id); | ||||||
|
|
||||||
| $this->assertInstanceOf(PublicWikiResource::class, $resource); | ||||||
| $this->assertSame(true, $resource->toArray(new Request)['reuse_prototype']); | ||||||
| } | ||||||
|
|
||||||
| public function testIndexEagerLoadsWikiLatestProfileOnceForCollection(): void { | ||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add a test that checks
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2 tests added:
|
||||||
| for ($i = 1; $i <= 3; $i++) { | ||||||
| $wiki = Wiki::factory()->create([ | ||||||
| 'domain' => "index-eager-load-test-{$i}.wikibase.cloud", | ||||||
| 'sitename' => "Index Eager Load Test Site {$i}", | ||||||
| ]); | ||||||
|
|
||||||
| WikiProfile::create([ | ||||||
| 'wiki_id' => $wiki->id, | ||||||
| 'purpose' => 'data_hub', | ||||||
| 'temporality' => 'permanent', | ||||||
| 'audience' => 'wide', | ||||||
| ]); | ||||||
| } | ||||||
|
|
||||||
| $profileQueryCount = 0; | ||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick:
Suggested change
|
||||||
| DB::listen(function (QueryExecuted $query) use (&$profileQueryCount): void { | ||||||
| if (str_contains($query->sql, 'wiki_profiles')) { | ||||||
| $profileQueryCount++; | ||||||
| } | ||||||
| }); | ||||||
|
|
||||||
| $controller = new PublicWikiController; | ||||||
| $resourceCollection = $controller->index(new Request); | ||||||
|
|
||||||
| $this->assertSame(1, $profileQueryCount); | ||||||
| $this->assertTrue($resourceCollection->first()->relationLoaded('wikiLatestProfile')); | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is incorrect now that the
->relationLoaded()check has been moved to a test.I would remove this comment. If we want to mention eager loading in the code then add a comment above PublicWikiController.php#L32 as that is where the eager loading is happening.