Add `reuse_prototype` field to `/wiki` API endpoint by outdooracorn · Pull Request #1082 · wbstack/api

outdooracorn · 2026-04-11T00:00:05Z

When ->with('wikiLatestProfile') is used in the PublicWikiController this change only results in 1 extra database query per API request.

Bug: T421877

ranyardm-wmde · 2026-04-13T08:53:43Z

IMO given we don't have enough monitoring to predict how this will affect database load, this should ideally not be the accepted solution as it will create the multiple queries per search problem we discussed in ticket breakdown. Apparently I'm wrong here.

rosalieper

I am please with how this prototype did not turnout as a bigger change

I can't add a "request changes" review as I created the PR. Removing this approval instead. Will create my review comment now.

outdooracorn

From the task's Task Breakdown section:

We would like to provide an alternative API endpoint so that the Suite team don't rely on an internal API endpoint that we could change at any point. We agreed to use /reusePrototype which points at the same PublicWikiController as the internal /wiki endpoint. See routes/api.php#L59.

If this turns out to be more than copy and editing routes/api.php#L59 - 2hrs of work to write and review, then reconsider

I don't see any changes to routes/api.php or a comment saying that this was attempted but would take too long to implement.

@dati18 you were in the task breakdown meeting where we discussed this. How come it's not included in this PR?

@rosalieper remember, as the reviewer, it's important to read through the task so you know what the PR should contain.

outdooracorn · 2026-05-27T15:43:16Z

+            // 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->relationLoaded('wikiLatestProfile')


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 toArray() method is called, though. Can we check that PublicWikiController eager loads the relationship in a test instead?

We might want to create a PublicWikiControllerTest unit test for this check. The current PublicWikiTest looks more like an e2e test which is not where I would expect this type of check to go. My hunch is that it will be easier to test in a unit test as well.

I somehow stashed the change and didn't notice. I also made a change to show() to make sure wikiLatestProfile was eager-loaded...
This is a stupid fault. I apologize. :(

I added a unit test for the relation check

based on your comment, I did the following:

added the endpoint /reusePrototype and also made sure show() eager-loads 'wikiLatestProfile'

added PublicWikiControllerTest unit test

modified the reusePrototypeTest by changing the endpoint to reusePrototype instead of generic this->route, also extended it to also call GET /reusePrototype/id

If we can create a successful test to check for eager loading, then we can remove this check here.

rosalieper · 2026-05-27T17:20:48Z

@rosalieper remember, as the reviewer, it's important to read through the task so you know what the PR should contain.

@outdooracorn Thank you for your comment, I read through the task description twice actually because it has a long description with updates and followup comment. Thanks for catching this. It left my mind at some point during the review but I will remember this incident. :)

dati18 · 2026-05-27T18:56:50Z

     */
    public function show($id) {
-        return new PublicWikiResource(Wiki::findOrFail($id));
+        return new PublicWikiResource(Wiki::query()->with('wikiLatestProfile')->findOrFail($id));


Same as $query in index(), I think we can benefit from eager loading here

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 of Wikis, if the N+1 issue won't appear.

I do get this error when I checkout the PR, composer install, rebuild the container and then visit

Can you pasted the command? I just run composer install and didn't see similar error. I tore down everything and rebuilt/reinstalled also...

I'm wondering that as we are only getting a single Wiki, rather than a list of Wikis, if the N+1 issue won't appear.

Ah you're right, it got me right there... since show() returns a single wiki, worst-case scenario is wikiLatestProfile was lazy-loaded and we do 1 extra query.

So I have 2 options:

Keep it for the sake of explicitness/consistency and avoid hidden lazy loading.

Reverse it back because its impact is very much minimal

What do you think?

@rosalieper your opinion is also valuable as well :)

My vote is to remove it from this PR as:

it is not required

it is not related to the task (T421877)

It's good to keep PRs small and to the point where possible.

outdooracorn

Sorry, I didn't have enough time to provide a full review 🥲

I do get this error when I checkout the PR, composer install, rebuild the container and then visit http://localhost:8082/wiki?sort=pages&direction=desc&page=1&per_page=10:

public/index.php:55
The stream or file "/var/www/html/storage/logs/laravel-2026-05-29.log" could not be opened in append mode: Failed to open stream: Permission denied The exception occurred while attempting to log: The stream or file "/var/www/html/storage/logs/laravel-2026-05-29.log" could not be opened in append mode: Failed to open stream: Permission denied The exception occurred while attempting to log: The stream or file "/var/www/html/storage/logs/laravel-2026-05-29.log" could not be opened in append mode: Failed to open stream: Permission denied The exception occurred while attempting to log: The stream or file "/var/www/html/storage/logs/laravel-2026-05-29.log" could not be opened in append mode: Failed to open stream: Permission denied The exception occurred while attempting to log: The stream or file "/var/www/html/storage/logs/laravel-2026-05-29.log" could not be opened in append mode: Failed to open stream: Permission denied The exception occurred while attempting to log: The stream or file "/var/www/html/storage/logs/laravel-2026-05-29.log" could not be opened in append mode: Failed to open stream: Permission denied The exception occurred while attempting to log: Query Executed: Context: {"sql":"select count(*) as aggregate from `wikis` where `wikis`.`deleted_at` is null","bindings":[],"connection":"mysql"} Context: {"exception":{}} Context: {"exception":{}} Context: {"exception":{}} Context: {"exception":{}} Context: {"exception":{}}

This doesn't happen when I checkout origin/main, composer install, rebuild the container, and then visit http://localhost:8082/wiki?sort=pages&direction=desc&page=1&per_page=10. So there is likely something for you to look into here. Hopefully I haven't done anything silly locally! 🙈

outdooracorn · 2026-05-29T14:29:38Z

     */
    public function show($id) {
-        return new PublicWikiResource(Wiki::findOrFail($id));
+        return new PublicWikiResource(Wiki::query()->with('wikiLatestProfile')->findOrFail($id));


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 of Wikis, if the N+1 issue won't appear.

outdooracorn

I tested again locally and couldn't reproduce my previous error, so not sure what the issue was there. Sorry for the noise.

outdooracorn · 2026-06-01T09:18:45Z

+class PublicWikiControllerTest extends TestCase {
+    use DatabaseTransactions;
+
+    public function testShowEagerLoadsWikiLatestProfileForResource(): void {


~~This test passes even if wikiLatestProfile isn't eager loaded.~~

~~The PublicWikiResource will lazy load the model if it hasn't already been loaded so $this->assertTrue($resource->resource->relationLoaded('wikiLatestProfile')); will always be true.~~

~~Looking at PublicWikiController a bit closer, it might be harder than I initially thought to test this.~~

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.

Are you sure?
If I change show() to lazy load the test fails... :(

public function show($id) { return new PublicWikiResource(Wiki::query()->findOrFail($id)); }

public function show($id) { return new PublicWikiResource(Wiki::findOrFail($id)); }

@outdooracorn can you test it again?

Oh! I see what happened here, I removed the eager loading from the index() method as that is the method that requires eager loading to avoid N+1 issues. I hadn't noticed you wrote this test only for the show() method. Why aren't testing the index() method?

Ah yes... index() returns a collection of wikis so it's worth testing than show()
I just added a test for index() by by generating a random n number of wikis (from 3 to 100) and checking for the number of queries (it should be 1) and making sure wikiLatestProfile is eager-loaded.

fun read about relation loaded and stuff

https://laravel-news.com/relationloaded

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:

Tests should be deterministic - given the same code and environment, they should behave the same way every time they run. Creating a random number of Wikis is not deterministic. I would create 3 - 5 wikis for this test.

Tests should not echo or otherwise print output. Instead, they should communicate results through assertions, exceptions, and PHPUnit's output mechanisms

outdooracorn · 2026-06-01T09:31:08Z

+            // 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->relationLoaded('wikiLatestProfile')


If we can create a successful test to check for eager loading, then we can remove this check here.

Bug: T421877

outdooracorn · 2026-06-03T16:03:25Z

+        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,
+                ]);
+            });
+        }


The stream or file "/var/www/html/storage/logs/laravel-2026-05-29.log" could not be opened in append mode: Failed to open stream: Permission denied

I got the above error again and tracked it down to:

some of the logs in /var/www/html/storage/logs/ were owned by root:root rather than www-data:www-data

this event listening doing a log

I don't think it's an issue in staging/production as I don't think logging to a file is enabled.

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

outdooracorn

As requested by @dati18, not a full review. Mostly comments around the PublicWikiControllerTest.

outdooracorn · 2026-06-04T12:25:03Z

+        });
+
+        $controller = new PublicWikiController;
+        $resourceCollection = $controller->index(request());


The request() method this for getting "an instance of the current request or an input item from the request" which does feel quite right to use here. I'd prefer to create an empty Request instance instead.

Suggested change

$resourceCollection = $controller->index(request());

$resourceCollection = $controller->index(new Request());

outdooracorn · 2026-06-04T12:27:26Z

+        $resourceCollection = $controller->index(request());
+
+        $this->assertSame(1, $profileQueryCount);
+        $this->assertTrue($resourceCollection->collection[0]->resource->relationLoaded('wikiLatestProfile'));


Optional nitpick: if you want, you can use first() instead of collection[0] and ->resource isn't required.

Suggested change

$this->assertTrue($resourceCollection->collection[0]->resource->relationLoaded('wikiLatestProfile'));

$this->assertTrue($resourceCollection->first()->relationLoaded('wikiLatestProfile'));

outdooracorn · 2026-06-04T12:49:27Z

+class PublicWikiControllerTest extends TestCase {
+    use DatabaseTransactions;
+
+    public function testShowEagerLoadsWikiLatestProfileForResource(): void {


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:

Tests should be deterministic - given the same code and environment, they should behave the same way every time they run. Creating a random number of Wikis is not deterministic. I would create 3 - 5 wikis for this test.

Tests should not echo or otherwise print output. Instead, they should communicate results through assertions, exceptions, and PHPUnit's output mechanisms

outdooracorn · 2026-06-04T12:53:43Z

+                'domain' => 'index-eager-load-test-' . $i . '.wikibase.cloud',
+                'sitename' => 'Index Eager Load Test Site ' . $i,


Nitpick: We are using a PHP version that supports string interpolation so lets use it as it reduces the line length. I also find it much easier to read.

Suggested change

'domain' => 'index-eager-load-test-' . $i . '.wikibase.cloud',

'sitename' => 'Index Eager Load Test Site ' . $i,

'domain' => "index-eager-load-test-{$i}.wikibase.cloud",

'sitename' => "Index Eager Load Test Site {$i}",

outdooracorn · 2026-06-04T12:56:38Z

     */
    public function show($id) {
-        return new PublicWikiResource(Wiki::findOrFail($id));
+        return new PublicWikiResource(Wiki::query()->with('wikiLatestProfile')->findOrFail($id));


My vote is to remove it from this PR as:

it is not required

it is not related to the task (T421877)

It's good to keep PRs small and to the point where possible.

outdooracorn self-assigned this Apr 11, 2026

dati18 force-pushed the ollie/reuse-prototype branch from 7c192cd to 148925e Compare May 22, 2026 10:43

dati18 marked this pull request as ready for review May 22, 2026 10:48

dati18 reviewed May 22, 2026

View reviewed changes

Comment thread app/Providers/AppServiceProvider.php Outdated

dati18 force-pushed the ollie/reuse-prototype branch from 1e2b090 to 8ac9b3d Compare May 22, 2026 15:12

dati18 changed the title ~~PoC: Add reuse_prototype field to /wiki API endpoint~~ Add reuse_prototype field to /wiki API endpoint May 26, 2026

rosalieper previously approved these changes May 27, 2026

View reviewed changes

Comment thread app/Providers/AppServiceProvider.php Outdated

dati18 dismissed rosalieper’s stale review via 3ab8465 May 27, 2026 14:26

dati18 requested review from dati18 and rosalieper May 27, 2026 14:30

rosalieper previously approved these changes May 27, 2026

View reviewed changes

outdooracorn commented May 27, 2026

View reviewed changes

dati18 force-pushed the ollie/reuse-prototype branch from 0d903dc to 976f2d9 Compare May 27, 2026 17:16

dati18 reviewed May 27, 2026

View reviewed changes

rosalieper reviewed May 28, 2026

View reviewed changes

Comment thread tests/Http/Controllers/PublicWikiControllerTest.php Outdated

dati18 requested a review from rosalieper May 29, 2026 07:56

outdooracorn commented May 29, 2026

View reviewed changes

outdooracorn commented Jun 1, 2026

View reviewed changes

dati18 force-pushed the ollie/reuse-prototype branch from a714fa9 to e9c7a31 Compare June 3, 2026 10:01

outdooracorn and others added 8 commits June 3, 2026 13:55

PoC: Add reuse_prototype field to /wiki API endpoint

c43c6a4

Bug: T421877

add test for reuse_prototype

73a92e7

Add load-state-aware reuse_prototype resource logic

07bb19b

Remove temporary reuse_prototype debug fields

9233389

Replace TODO with a purpose-focus explaintion for the local listener

69da736

Add /reusePrototype endpoint

a06042e

Eager-load Wiki model and preloads WikiLatestProfile in show()

e7a36ef

Add PublicWikiControllerTest unit test for loaded relation

84f1b51

dati18 added 3 commits June 3, 2026 13:55

Change endpoint to reusePrototype to explicitly verify that route

a5e0f75

extend testReusePrototype to also call GET /reusePrototype/{id}

5c931e2

Check the model has the relation loaded

bdd52e8

dati18 force-pushed the ollie/reuse-prototype branch from e9c7a31 to bdd52e8 Compare June 3, 2026 11:55

outdooracorn commented Jun 3, 2026

View reviewed changes

add unit test and query count for index()

0657731

dati18 force-pushed the ollie/reuse-prototype branch from 6d415c7 to 0657731 Compare June 3, 2026 21:25

remove debugging "echo"

a993cba

outdooracorn commented Jun 4, 2026

View reviewed changes

Apply suggestions from reviews

e48435f

dati18 requested review from dati18, deer-wmde and tarrow June 4, 2026 16:23

	$resourceCollection = $controller->index(request());
	$resourceCollection = $controller->index(new Request());

	$this->assertTrue($resourceCollection->collection[0]->resource->relationLoaded('wikiLatestProfile'));
	$this->assertTrue($resourceCollection->first()->relationLoaded('wikiLatestProfile'));

		'domain' => 'index-eager-load-test-' . $i . '.wikibase.cloud',
		'sitename' => 'Index Eager Load Test Site ' . $i,

Conversation

outdooracorn commented Apr 11, 2026

Uh oh!

ranyardm-wmde commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rosalieper left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

outdooracorn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dati18 May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dati18 May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosalieper commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

outdooracorn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

outdooracorn left a comment

Choose a reason for hiding this comment

Uh oh!

outdooracorn Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dati18 Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dati18 Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranyardm-wmde commented Apr 13, 2026 •

edited

Loading

dati18 May 27, 2026 •

edited

Loading

dati18 May 27, 2026 •

edited

Loading

rosalieper commented May 27, 2026 •

edited

Loading

outdooracorn Jun 1, 2026 •

edited

Loading

dati18 Jun 3, 2026 •

edited

Loading

dati18 Jun 3, 2026 •

edited

Loading