Skip to content
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/Http/Controllers/PublicWikiController.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public function index(Request $request) {
]);

$params = array_merge(self::$defaultParams, $request->input());
$query = Wiki::query();
$query = Wiki::query()->with('wikiLatestProfile');

if (array_key_exists('is_featured', $params)) {
$query = $query->where([
Expand Down Expand Up @@ -72,6 +72,6 @@ public function index(Request $request) {
* Display the specified resource.
*/
public function show($id) {
return new PublicWikiResource(Wiki::findOrFail($id));
return new PublicWikiResource(Wiki::query()->with('wikiLatestProfile')->findOrFail($id));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

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 :)

Copy link
Copy Markdown
Member Author

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 is not required
  • it is not related to the task (T421877)

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

}
}
10 changes: 10 additions & 0 deletions app/Http/Resources/PublicWikiResource.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Member Author

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.

'reuse_prototype' => $this->relationLoaded('wikiLatestProfile')
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@dati18 dati18 May 27, 2026

Choose a reason for hiding this comment

The 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 show() to make sure wikiLatestProfile was eager-loaded...
This is a stupid fault. I apologize. :(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a unit test for the relation check

Copy link
Copy Markdown
Contributor

@dati18 dati18 May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the Acceptance Criteria in the task, this should be false rather than null.

  • [] The flag is 1/true if the instance has:
    • purpose = data_hub AND
    • lifespan = permanent AND
    • audience = wide
  • [] The flag is 0/false otherwise.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Summary:

  • "non-reusable" wiki still returns false
  • "no-profile" wiki returns false instead of null

];
}
}
12 changes: 12 additions & 0 deletions app/Providers/AppServiceProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
]);
});
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

}
}
1 change: 1 addition & 0 deletions routes/api.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,5 +57,6 @@
});

$router->apiResource('wiki', 'PublicWikiController')->only(['index', 'show']);
$router->apiResource('reusePrototype', 'PublicWikiController')->only(['index', 'show']);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we should just enable the index method for the reuse prototype as that is all that is requested in the task.

Copy link
Copy Markdown
Contributor

@dati18 dati18 Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it and it means I also had to update testReusePrototype(), and we can't load a single wiki anymore aka reusePrototype/{$id} is no longer a valid route just so you know

$router->apiResource('wikiConversionData', 'ConversionMetricController')->only(['index']);
});
34 changes: 34 additions & 0 deletions tests/Http/Controllers/PublicWikiControllerTest.php
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 {
Copy link
Copy Markdown
Member Author

@outdooracorn outdooracorn Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@dati18 dati18 Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@outdooracorn can you test it again?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 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?

Copy link
Copy Markdown
Contributor

@dati18 dati18 Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fun read about relation loaded and stuff

https://laravel-news.com/relationloaded

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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:

  • 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

$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'));
}
}
51 changes: 51 additions & 0 deletions tests/Routes/Wiki/PublicWikiTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Tests\Routes\Wiki;

use App\Wiki;
use App\WikiProfile;
use App\WikiSetting;
use App\WikiSiteStats;
use Illuminate\Foundation\Testing\DatabaseTransactions;
Expand All @@ -18,12 +19,14 @@ class PublicWikiTest extends TestCase {
protected function setUp(): void {
parent::setUp();
Wiki::query()->delete();
WikiProfile::query()->delete();
WikiSiteStats::query()->delete();
WikiSetting::query()->delete();
}

protected function tearDown(): void {
Wiki::query()->delete();
WikiProfile::query()->delete();
WikiSiteStats::query()->delete();
WikiSetting::query()->delete();
parent::tearDown();
Expand Down Expand Up @@ -279,4 +282,52 @@ public function testLogoUrl() {
)
->assertJsonPath('data.1.logo_url', null);
}

public function testReusePrototype() {
$reusableWiki = Wiki::factory()->create([
'domain' => 'reusable.wikibase.cloud', 'sitename' => 'asite',
]);
WikiSiteStats::factory()->create([
'wiki_id' => $reusableWiki->id,
]);
WikiProfile::create([
'wiki_id' => $reusableWiki->id,
'purpose' => 'data_hub',
'temporality' => 'permanent',
'audience' => 'wide',
]);

$nonReusableWiki = Wiki::factory()->create([
'domain' => 'non-reusable.wikibase.cloud', 'sitename' => 'bsite',
]);
WikiSiteStats::factory()->create([
'wiki_id' => $nonReusableWiki->id,
]);
WikiProfile::create([
'wiki_id' => $nonReusableWiki->id,
'purpose' => 'other',
'temporality' => 'other',
'audience' => 'other',
]);

$noProfileWiki = Wiki::factory()->create([
'domain' => 'no-profile.wikibase.cloud', 'sitename' => 'csite',
]);
WikiSiteStats::factory()->create([
'wiki_id' => $noProfileWiki->id,
]);

$this->json('GET', 'reusePrototype')
->assertStatus(200)
->assertJsonPath('data.0.domain', 'reusable.wikibase.cloud')
->assertJsonPath('data.0.reuse_prototype', true)
->assertJsonPath('data.1.domain', 'non-reusable.wikibase.cloud')
->assertJsonPath('data.1.reuse_prototype', false)
->assertJsonPath('data.2.domain', 'no-profile.wikibase.cloud')
->assertJsonPath('data.2.reuse_prototype', null);

$this->json('GET', 'reusePrototype/' . $reusableWiki->id)
->assertStatus(200)
->assertJsonPath('data.reuse_prototype', true);
}
}
Loading