Skip to content

Commit 3179ab3

Browse files
mneudertmneudert
committed
Handle empty report tool apiParameters more gracefully
1 parent 876df57 commit 3179ab3

8 files changed

Lines changed: 323 additions & 9 deletions

File tree

‎McpTools/ReportMetadata.php‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use Piwik\Plugins\McpServer\Contracts\Records\Reports\ReportMetadataRecord;
1919
use Piwik\Plugins\McpServer\Contracts\Ports\Reports\ReportMetadataQueryServiceInterface;
2020
use Piwik\Plugins\McpServer\Schemas\Reports\ReportMetadataToolOutputSchema;
21+
use Piwik\Plugins\McpServer\Support\Normalization\ToolDataNormalizer;
2122

2223
/**
2324
* @phpstan-import-type ReportMetadataArray from ReportMetadataRecord
@@ -76,9 +77,18 @@ public function __construct(private ReportMetadataQueryServiceInterface $querySe
7677
'description' => 'Date for module/action metadata lookup (default today).',
7778
],
7879
'apiParameters' => [
79-
'type' => 'object',
80-
'description' => 'Optional report parameter object used with module/action lookup.',
81-
'additionalProperties' => true,
80+
'oneOf' => [
81+
[
82+
'type' => 'object',
83+
'description' => 'Optional report parameter object used with module/action lookup.',
84+
'additionalProperties' => true,
85+
],
86+
[
87+
'type' => 'array',
88+
'maxItems' => 0,
89+
'description' => 'Empty array is accepted and normalized as no apiParameters.',
90+
],
91+
],
8292
],
8393
],
8494
'required' => ['idSite'],
@@ -100,7 +110,21 @@ public function __construct(private ReportMetadataQueryServiceInterface $querySe
100110
],
101111
['required' => ['reportUniqueId', 'apiModule']],
102112
['required' => ['reportUniqueId', 'apiAction']],
103-
['required' => ['reportUniqueId', 'apiParameters']],
113+
[
114+
'allOf' => [
115+
['required' => ['reportUniqueId', 'apiParameters']],
116+
[
117+
'properties' => [
118+
'apiParameters' => [
119+
'not' => [
120+
'type' => 'array',
121+
'maxItems' => 0,
122+
],
123+
],
124+
],
125+
],
126+
],
127+
],
104128
[
105129
'required' => ['apiModule'],
106130
'not' => ['required' => ['apiAction']],
@@ -122,11 +146,15 @@ public function get(
122146
?string $date = null,
123147
?array $apiParameters = null
124148
): array {
149+
$apiParameters = $apiParameters === null
150+
? null
151+
: ToolDataNormalizer::requireStringKeyedArrayOrEmptyList($apiParameters, 'apiParameters');
152+
125153
if ($reportUniqueId !== null) {
126154
if (
127155
$apiModule !== null
128156
|| $apiAction !== null
129-
|| $apiParameters !== null
157+
|| ($apiParameters !== null && $apiParameters !== [])
130158
) {
131159
throw new ToolCallException(
132160
'Invalid parameter combination: reportUniqueId cannot be combined '

‎McpTools/ReportProcessed.php‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Piwik\Plugins\McpServer\Contracts\Ports\Reports\ReportProcessedQueryServiceInterface;
1818
use Piwik\Plugins\McpServer\Contracts\Records\Reports\ReportProcessedRecord;
1919
use Piwik\Plugins\McpServer\Schemas\Reports\ReportProcessedToolOutputSchema;
20+
use Piwik\Plugins\McpServer\Support\Normalization\ToolDataNormalizer;
2021
use Piwik\Plugins\McpServer\Support\Reports\GoalMetricsMode;
2122

2223
/**
@@ -85,10 +86,19 @@ public function __construct(private ReportProcessedQueryServiceInterface $queryS
8586
'description' => 'Report API action when reportUniqueId is not used.',
8687
],
8788
'apiParameters' => [
88-
'type' => 'object',
89-
'additionalProperties' => true,
90-
'description' => 'Optional report parameters '
91-
. '(safe generic params and report-specific selector params).',
89+
'oneOf' => [
90+
[
91+
'type' => 'object',
92+
'additionalProperties' => true,
93+
'description' => 'Optional report parameters '
94+
. '(safe generic params and report-specific selector params).',
95+
],
96+
[
97+
'type' => 'array',
98+
'maxItems' => 0,
99+
'description' => 'Empty array is accepted and normalized as no apiParameters.',
100+
],
101+
],
92102
],
93103
'goalMetricsMode' => [
94104
'type' => 'string',
@@ -195,6 +205,10 @@ public function get(
195205
?int $filter_limit = null,
196206
?int $filter_offset = null
197207
): array {
208+
$apiParameters = $apiParameters === null
209+
? null
210+
: ToolDataNormalizer::requireStringKeyedArrayOrEmptyList($apiParameters, 'apiParameters');
211+
198212
return $this->queryService->getProcessedReport(
199213
idSite: $idSite,
200214
period: $period,

‎Support/Normalization/ToolDataNormalizer.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,22 @@ public static function requireStringKeyedArray(mixed $value, string $context): a
3434
return $value;
3535
}
3636

37+
/**
38+
* @return array<string, mixed>
39+
*/
40+
public static function requireStringKeyedArrayOrEmptyList(mixed $value, string $context): array
41+
{
42+
if (!is_array($value)) {
43+
throw new ToolCallException(self::invalidMessage($context));
44+
}
45+
46+
if ($value === []) {
47+
return [];
48+
}
49+
50+
return self::requireStringKeyedArray($value, $context);
51+
}
52+
3753
private static function invalidMessage(string $context): string
3854
{
3955
if (str_ends_with($context, '.')) {

‎tests/Integration/McpTools/ReportMetadataTest.php‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,40 @@ public function testRejectsCombinedUniqueIdAndApiParametersAtSchemaLevel(): void
237237
]);
238238
}
239239

240+
public function testAllowsUniqueIdCombinedWithEmptyListApiParameters(): void
241+
{
242+
$report = $this->findAnyReportMetadata($this->idSite);
243+
self::assertNotNull($report);
244+
$uniqueId = $report['uniqueId'] ?? null;
245+
self::assertIsString($uniqueId);
246+
247+
$server = McpTestHelper::buildServer();
248+
$sessionId = McpTestHelper::initializeSession($server);
249+
$content = McpTestHelper::callToolAndAssertSuccess(
250+
$server,
251+
$sessionId,
252+
ReportMetadata::TOOL_NAME,
253+
[
254+
'idSite' => $this->idSite,
255+
'reportUniqueId' => $uniqueId,
256+
'apiParameters' => [],
257+
],
258+
__METHOD__
259+
);
260+
261+
self::assertSame($uniqueId, $content['uniqueId'] ?? null);
262+
}
263+
264+
public function testRejectsModuleActionWithNonEmptyListApiParametersAtSchemaLevel(): void
265+
{
266+
$this->assertInvalidSchemaArguments([
267+
'idSite' => $this->idSite,
268+
'apiModule' => 'Actions',
269+
'apiAction' => 'getPageUrls',
270+
'apiParameters' => ['flat'],
271+
]);
272+
}
273+
240274
public function testMasksNoAccessAsNotFound(): void
241275
{
242276
$report = $this->findAnyReportMetadata($this->idSite);

‎tests/Integration/McpTools/ReportProcessedTest.php‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,43 @@ public function testRejectsOldGoalParameterNamesAtSchemaLevel(): void
510510
self::assertStringContainsString('goal_columns_mode', $error->message ?? '');
511511
}
512512

513+
public function testAllowsUniqueIdCombinedWithEmptyListApiParameters(): void
514+
{
515+
$reportUniqueId = $this->findReportUniqueId($this->idSite, 'Actions', 'getPageUrls');
516+
self::assertNotNull($reportUniqueId);
517+
518+
$server = McpTestHelper::buildServer();
519+
$sessionId = McpTestHelper::initializeSession($server);
520+
$content = McpTestHelper::callToolAndAssertSuccess(
521+
$server,
522+
$sessionId,
523+
ReportProcessed::TOOL_NAME,
524+
[
525+
'idSite' => $this->idSite,
526+
'period' => 'day',
527+
'date' => '2015-01-03',
528+
'reportUniqueId' => $reportUniqueId,
529+
'apiParameters' => [],
530+
'filter_limit' => 1,
531+
'filter_offset' => 0,
532+
],
533+
__METHOD__
534+
);
535+
536+
$resolvedReport = $content['resolvedReport'] ?? null;
537+
self::assertIsArray($resolvedReport);
538+
self::assertSame($reportUniqueId, $resolvedReport['uniqueId'] ?? null);
539+
}
540+
541+
public function testRejectsModuleActionWithNonEmptyListApiParametersAtSchemaLevel(): void
542+
{
543+
$this->assertInvalidSelectorArgumentsAtSchemaLevel([
544+
'apiModule' => 'Actions',
545+
'apiAction' => 'getPageUrls',
546+
'apiParameters' => ['flat'],
547+
]);
548+
}
549+
513550
public function testReturnsStrictGuidanceForAdHocSegmentInStrictArchivingMode(): void
514551
{
515552
$reportUniqueId = $this->findReportUniqueId($this->idSite, 'Actions', 'getPageUrls');

‎tests/Unit/McpTools/ReportMetadataTest.php‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,49 @@ public function getReportMetadataByModuleAction(
153153
self::assertSame('Actions_getPageUrls', $wrapper->captured['reportUniqueId']);
154154
}
155155

156+
public function testGetAllowsUniqueIdCombinedWithEmptyListApiParameters(): void
157+
{
158+
$wrapper = new class () implements ReportMetadataQueryServiceInterface {
159+
/** @var array<string, mixed> */
160+
public array $captured = [];
161+
162+
public function getReportMetadataByUniqueId(int $idSite, string $reportUniqueId): ReportMetadataRecord
163+
{
164+
$this->captured = ['idSite' => $idSite, 'reportUniqueId' => $reportUniqueId];
165+
166+
return new ReportMetadataRecord(
167+
uniqueId: $reportUniqueId,
168+
module: 'Actions',
169+
action: 'getPageUrls',
170+
name: 'Page URLs',
171+
category: 'Actions',
172+
parameters: [],
173+
metadata: ['uniqueId' => $reportUniqueId]
174+
);
175+
}
176+
177+
public function getReportMetadataByModuleAction(
178+
int $idSite,
179+
string $apiModule,
180+
string $apiAction,
181+
array $apiParameters,
182+
string $period,
183+
string $date
184+
): ReportMetadataRecord {
185+
throw new \RuntimeException('unexpected');
186+
}
187+
};
188+
189+
$actual = (new ReportMetadata($wrapper))->get(
190+
idSite: 1,
191+
reportUniqueId: 'Actions_getPageUrls',
192+
apiParameters: []
193+
);
194+
195+
self::assertSame('Actions_getPageUrls', $actual['uniqueId']);
196+
self::assertSame('Actions_getPageUrls', $wrapper->captured['reportUniqueId']);
197+
}
198+
156199
public function testGetRejectsUniqueIdCombinedWithModuleActionOrApiParameters(): void
157200
{
158201
$wrapper = new class () implements ReportMetadataQueryServiceInterface {
@@ -182,4 +225,35 @@ public function getReportMetadataByModuleAction(
182225
apiModule: 'Actions'
183226
);
184227
}
228+
229+
public function testGetRejectsNonEmptyListApiParameters(): void
230+
{
231+
$wrapper = new class () implements ReportMetadataQueryServiceInterface {
232+
public function getReportMetadataByUniqueId(int $idSite, string $reportUniqueId): ReportMetadataRecord
233+
{
234+
throw new \RuntimeException('unexpected');
235+
}
236+
237+
public function getReportMetadataByModuleAction(
238+
int $idSite,
239+
string $apiModule,
240+
string $apiAction,
241+
array $apiParameters,
242+
string $period,
243+
string $date
244+
): ReportMetadataRecord {
245+
throw new \RuntimeException('unexpected');
246+
}
247+
};
248+
249+
$this->expectException(ToolCallException::class);
250+
$this->expectExceptionMessage('apiParameters is invalid.');
251+
252+
(new ReportMetadata($wrapper))->get(
253+
idSite: 1,
254+
apiModule: 'VisitsSummary',
255+
apiAction: 'get',
256+
apiParameters: ['flat']
257+
);
258+
}
185259
}

0 commit comments

Comments
 (0)