Skip to content

Commit cb814b2

Browse files
mneudertmneudert
committed
Handle empty report tool apiParameters more gracefully
1 parent 095cf9e commit cb814b2

10 files changed

Lines changed: 432 additions & 13 deletions

File tree

‎Contracts/Records/Reports/ReportProcessedRecord.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
* uniqueId: string,
2424
* apiModule: string,
2525
* apiAction: string,
26-
* apiParameters: array<string, mixed>,
26+
* apiParameters: array<string, mixed>|object,
2727
* }
2828
* @phpstan-type ReportProcessedArray array{
2929
* report: array<string, mixed>,
@@ -69,7 +69,7 @@ public function toArray(): array
6969
'uniqueId' => $this->uniqueId,
7070
'apiModule' => $this->apiModule,
7171
'apiAction' => $this->apiAction,
72-
'apiParameters' => $this->apiParameters,
72+
'apiParameters' => $this->apiParameters === [] ? new \stdClass() : $this->apiParameters,
7373
],
7474
];
7575
}

‎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: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,33 @@ public function testRejectsDangerousApiParametersKey(): void
165165
);
166166
}
167167

168+
public function testSerializesEmptyResolvedApiParametersAsObjectInResponseBody(): void
169+
{
170+
$reportUniqueId = $this->findReportUniqueId($this->idSite, 'Actions', 'getPageUrls');
171+
self::assertNotNull($reportUniqueId);
172+
173+
$server = McpTestHelper::buildServer();
174+
$sessionId = McpTestHelper::initializeSession($server);
175+
$payload = McpTestHelper::makeCallToolRequest(
176+
ReportProcessed::TOOL_NAME,
177+
[
178+
'idSite' => $this->idSite,
179+
'period' => 'day',
180+
'date' => '2015-01-03',
181+
'reportUniqueId' => $reportUniqueId,
182+
'filter_limit' => 1,
183+
'filter_offset' => 0,
184+
],
185+
__METHOD__
186+
);
187+
188+
$response = McpTestHelper::postJson($server, $payload, ['Mcp-Session-Id' => $sessionId]);
189+
$body = McpTestHelper::getResponseBody($response);
190+
191+
self::assertStringContainsString('"resolvedReport"', $body);
192+
self::assertStringContainsString('"apiParameters":{}', $body);
193+
}
194+
168195
public function testRejectsInvalidPeriodDateParameters(): void
169196
{
170197
$reportUniqueId = $this->findReportUniqueId($this->idSite, 'Actions', 'getPageUrls');
@@ -510,6 +537,43 @@ public function testRejectsOldGoalParameterNamesAtSchemaLevel(): void
510537
self::assertStringContainsString('goal_columns_mode', $error->message ?? '');
511538
}
512539

540+
public function testAllowsUniqueIdCombinedWithEmptyListApiParameters(): void
541+
{
542+
$reportUniqueId = $this->findReportUniqueId($this->idSite, 'Actions', 'getPageUrls');
543+
self::assertNotNull($reportUniqueId);
544+
545+
$server = McpTestHelper::buildServer();
546+
$sessionId = McpTestHelper::initializeSession($server);
547+
$content = McpTestHelper::callToolAndAssertSuccess(
548+
$server,
549+
$sessionId,
550+
ReportProcessed::TOOL_NAME,
551+
[
552+
'idSite' => $this->idSite,
553+
'period' => 'day',
554+
'date' => '2015-01-03',
555+
'reportUniqueId' => $reportUniqueId,
556+
'apiParameters' => [],
557+
'filter_limit' => 1,
558+
'filter_offset' => 0,
559+
],
560+
__METHOD__
561+
);
562+
563+
$resolvedReport = $content['resolvedReport'] ?? null;
564+
self::assertIsArray($resolvedReport);
565+
self::assertSame($reportUniqueId, $resolvedReport['uniqueId'] ?? null);
566+
}
567+
568+
public function testRejectsModuleActionWithNonEmptyListApiParametersAtSchemaLevel(): void
569+
{
570+
$this->assertInvalidSelectorArgumentsAtSchemaLevel([
571+
'apiModule' => 'Actions',
572+
'apiAction' => 'getPageUrls',
573+
'apiParameters' => ['flat'],
574+
]);
575+
}
576+
513577
public function testReturnsStrictGuidanceForAdHocSegmentInStrictArchivingMode(): void
514578
{
515579
$reportUniqueId = $this->findReportUniqueId($this->idSite, 'Actions', 'getPageUrls');

‎tests/Unit/McpTools/ReportMetadataTest.php‎

Lines changed: 77 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,38 @@ 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+
$tool = new ReportMetadata($wrapper);
253+
$method = new \ReflectionMethod($tool, 'get');
254+
255+
$method->invokeArgs($tool, [
256+
'idSite' => 1,
257+
'apiModule' => 'VisitsSummary',
258+
'apiAction' => 'get',
259+
'apiParameters' => ['flat'],
260+
]);
261+
}
185262
}

0 commit comments

Comments
 (0)