Add entry for total number of results to 'meta' data in API modules #5332
Conversation
Useful information to be included and sent to the Api (when action is ask or askargs). SemanticMediaWiki#4857
Fix for queries that don't support format=count, e.g. compound queries with SCQ (+ update DISerializer > SMW\Serializers\QueryResultSerializer).
|
Thanks for your PR! Can you please add tests to your code? |
|
Well, I've never written tests but first things first: I made a mistake in the error handling ( |
src/Query/QueryResult.php
Outdated
| try { | ||
| $test = $store->getQueryResult( $query ); | ||
| } catch (\Exception|\Throwable $e) { | ||
| return null; |
There was a problem hiding this comment.
I'm dubious about the return value being nullable. That makes things more difficult for the callers.
Catching exceptions also makes it impossible for them to handle exceptions in the way they want.
Is there some reason to not always return an int, and to let exceptions bubble up to the caller?
There was a problem hiding this comment.
My main concern here is/was that queries without support for the count format, such as queries produced by Semantic Compound Queries and possibly others I'm not aware of, should still be able to produce Api data, without ApiQueryResultFormatter::doFormat throwing an error message instead.
On the other hand, you could argue that relying on an error is a little hackish and unsound if the use case (SCQ) is legitimate, which is why I hoped there might be a cleaner solution that actually checks in advance if the count format is supported for a particular query. That and you're quite right to point out that there may well be genuine errors that would fail to get exposed if the output is nullable.
Do you have any solution or recommendations in mind?
There was a problem hiding this comment.
Returning 0 for "unsupported formats" seems like a better move than returning null.
Why would compound query results cause an error/exception?
There was a problem hiding this comment.
I suspect the reason the method wouldn’t work for SCQ is because the query object available at this point is unsuitable, e.g. it doesn’t have an equivalent query condition if you look at the serialized version. That's probably why $this->getStore()->getQueryResult( $query ); fails.[0]
Instead, I ended up with an approach not too dissimilar from what’s been done for the #rowcount parameter, creating a new query through calls to SMWQueryProcessor class methods. Still hesitant because SCQ comes with its own query processor.
It works in my test cases, but I'm not a PHP developer and I’m not familiar enough with the architecture to know whether those calls run the risk of circularity. Please let me know if you think this is the wrong place in the code to retrieve and add a result count (I actually started by editing Query.php and ApiQueryResultFormatter.php, but then reconsidered because it seemed to me late-stage intervention).
[0]Exception caught: Argument 1 passed to SMW\\SQLStore\\QueryEngine\\ConditionBuilder::buildFromDescription() must be an instance of SMW\\Query\\Language\\Description, null given, called in .../extensions/SemanticMediaWiki/src/SQLStore/QueryEngine/ConditionBuilder.php on line 211
src/Query/QueryResult.php
Outdated
| */ | ||
| public function getTotalCount() { | ||
| $query = $this->mQuery; | ||
| $query->querymode = \SMWQuery::MODE_COUNT; //change query format to count |
There was a problem hiding this comment.
This side-effect is not undone, so calls to other methods might no longer be correct once you call this new method
Will comment elsewhere.
Per discussion.
Additional parameter holding the total number of query results regardless of maximum to retrieval (see #4857). Useful for inclusion in ask and askargs API modules. Relies on output of query with format=count (cf. rowcount); if the query happens not to support the format, null is returned.