-
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 11 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,16 @@ 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 | ||
|
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. This comment is incorrect now that the 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. |
||
| 'reuse_prototype' => $this->relationLoaded('wikiLatestProfile') | ||
|
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. This is good to think about to avoid accidentally creating an N+1 query issue in the future. I don't think it requires us to check this every time the
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. We might want to create a
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 somehow stashed the change and didn't notice. I also made a change to
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 added a unit test for the relation check
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. based on your comment, I did the following:
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. If we can create a successful test to check for eager loading, then we can remove this check here. |
||
| ? ($this->wikiLatestProfile | ||
| ? $this->wikiLatestProfile->purpose === 'data_hub' | ||
| && $this->wikiLatestProfile->temporality === 'permanent' | ||
| && $this->wikiLatestProfile->audience === 'wide' | ||
| : null) | ||
| : 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,34 @@ | ||
| <?php | ||
|
|
||
| namespace Tests\Http\Controllers; | ||
|
|
||
| use App\Http\Controllers\PublicWikiController; | ||
| use App\Http\Resources\PublicWikiResource; | ||
| use App\Wiki; | ||
| use App\WikiProfile; | ||
| use Illuminate\Foundation\Testing\DatabaseTransactions; | ||
| use Tests\TestCase; | ||
|
|
||
| class PublicWikiControllerTest extends TestCase { | ||
| use DatabaseTransactions; | ||
|
|
||
| public function testShowEagerLoadsWikiLatestProfileForResource(): 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.
EDIT: the above is incorrect. You mentioned in another comment about creating a test queries counter and ensure 'wiki_profiles' is hit only once. Is this something that Laravel provides helpers for? This might be useful if not too difficult to implement.
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. Are you sure? public function show($id) {
return new PublicWikiResource(Wiki::query()->findOrFail($id));
}public function show($id) {
return new PublicWikiResource(Wiki::findOrFail($id));
}
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. @outdooracorn can you test it again?
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. Oh! I see what happened here, I removed the eager loading from 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. Ah yes...
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. fun read about relation loaded and stuff
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. Cool that you have figured out how to count the SQL queries executed. However, there are some things I don't like about this new test:
|
||
| $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->assertTrue($resource->resource->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.
Same as
$queryinindex(), I think we can benefit from eager loading hereThere 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.
What makes you think that? Did you count the number of DB queries like I did in https://phabricator.wikimedia.org/T421877#11810168?
I'm wondering that as we are only getting a single
Wiki, rather than a list ofWikis, if the N+1 issue won't appear.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.
Can you pasted the command? I just run composer install and didn't see similar error. I tore down everything and rebuilt/reinstalled also...
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.
Ah you're right, it got me right there... since
show()returns a single wiki, worst-case scenario iswikiLatestProfilewas lazy-loaded and we do 1 extra query.So I have 2 options:
What do you think?
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.
@rosalieper your opinion is also valuable as well :)
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.
My vote is to remove it from this PR as:
It's good to keep PRs small and to the point where possible.