Skip to content

Conversation

@JeroenDeDauw
Copy link
Member

Fixes #271

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.70%. Comparing base (2c81e00) to head (c25f7be).

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #272      +/-   ##
============================================
+ Coverage     73.37%   73.70%   +0.33%     
- Complexity      399      401       +2     
============================================
  Files            47       47              
  Lines          1292     1297       +5     
============================================
+ Hits            948      956       +8     
+ Misses          344      341       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

private function getPropertiesToIndex( ItemId $itemType, StatementList $statements ): array {
if ( $this->config->indexAllProperties ) {
// TODO: is this sufficient, or do we need to end up explicitly indexing properties not present as empty?
$properties = $statements->getPropertyIds();
Copy link
Member Author

Choose a reason for hiding this comment

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

As per https://github.com/ProfessionalWiki/WikibaseFacetedSearch/pull/51/files#r1901363923, it seems we should explicitly tell elastic "there is no value" for all properties not present in the statement list, to avoid keeping data for removed statements in the index.

So we need to replace this "IDs of all properties present in the statement list" with "IDs of all properties in the wiki"

Copy link
Member Author

Choose a reason for hiding this comment

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

@malberts makes sense to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds correct. And also relates to defining all properties in #272 (comment)

<p>
Indexing values for all properties is useful
when you want to be able to run structured queries for properties not shown in the UI. It is also
useful to avoid having to rebuild the search index whenever you add a new property to the UI.
Copy link
Contributor

@malberts malberts Jun 9, 2025

Choose a reason for hiding this comment

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

add a new property to the UI.

I think this can be misunderstood. I get this refers to the faceted search UI, but I can see how this might look like it's referring to the general UI (including Wikibase).

Adding an existing property (already indexed) in the faceted search config works automatically. But when a new property is added on the wiki and the config, then the index must still be rebuilt.

The hook for defining the fields to be indexed is separate from the hook returning the data.

@malberts
Copy link
Contributor

malberts commented Jun 9, 2025

Does this code actually work as intended? (I did not check manually.)

I just realised that I didn't mention SearchIndexFieldsBuilder on the original issue. Specifically, the fields to be indexed are defined according to the configured facets:

public function createFieldObjects(): array {
return $this->makeItemTypeSearchFieldMapping( $this->config->getItemTypeProperty() )
+ $this->makeFacetSearchFieldMappings( $this->config->getFacets()->asArray() );
}

My understanding of the code in this PR is that it will index all defined facet properties on all items, but if a property is not defined as a facet then it will not get indexed.

I believe we need to return all the wiki properties there.

(Unfortunately, an integration test for this is outstanding: #256)

@JeroenDeDauw
Copy link
Member Author

I did not test this yet and did not look at SearchIndexFieldsBuilder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Index all supported properties automatically

4 participants