Skip to content

Commit 2b14865

Browse files
KocKoc
authored
perf: count form submissions in bulk for the forms list (#3734)
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
1 parent 4821664 commit 2b14865

5 files changed

Lines changed: 243 additions & 39 deletions

File tree

‎lib/Controller/ApiController.php‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -126,20 +126,16 @@ public function preflightedCors() {
126126
#[NoAdminRequired()]
127127
#[ApiRoute(verb: 'GET', url: '/api/v3/forms')]
128128
public function getForms(string $type = 'owned'): DataResponse {
129-
$result = [];
130-
131129
if ($type === 'owned') {
132130
$forms = $this->formMapper->findAllByOwnerId($this->currentUser->getUID());
133-
foreach ($forms as $form) {
134-
$result[] = $this->formsService->getPartialFormArray($form);
135-
}
136131
} elseif ($type === 'shared') {
137132
$forms = $this->formsService->getSharedForms($this->currentUser);
138-
$result = array_values(array_map($this->formsService->getPartialFormArray(...), $forms));
139133
} else {
140134
throw new OCSBadRequestException('wrong form type supplied');
141135
}
142136

137+
$result = $this->formsService->getPartialFormArrays($forms);
138+
143139
return new DataResponse($result, Http::STATUS_OK);
144140
}
145141

‎lib/Db/SubmissionMapper.php‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
* @extends QBMapper<Submission>
2020
*/
2121
class SubmissionMapper extends QBMapper {
22+
private const CHUNK_SIZE = 1000;
23+
2224
/**
2325
* SubmissionMapper constructor.
2426
* @param IDBConnection $db
@@ -134,6 +136,39 @@ public function countSubmissions(int $formId, ?string $userId = null, ?string $s
134136
return $this->countSubmissionsWithFilters($formId, $userId, -1, $searchString);
135137
}
136138

139+
/**
140+
* Counts submissions for multiple forms in a single query.
141+
*
142+
* @param list<int> $formIds The IDs of the forms to count submissions for.
143+
* @param string|null $userId Optionally limit the counted submissions to the given user.
144+
* @return array<int, int> Map of form ID to submission count. Forms without submissions are not included.
145+
* @throws \Exception If an error occurs during the count operation.
146+
*/
147+
public function countSubmissionsByForms(array $formIds, ?string $userId = null): array {
148+
$counts = [];
149+
150+
foreach (array_chunk(array_unique($formIds), self::CHUNK_SIZE) as $formIdsChunk) {
151+
$qb = $this->db->getQueryBuilder();
152+
153+
$qb->select('submissions.form_id', $qb->func()->count('submissions.id', 'submission_count'))
154+
->from($this->getTableName(), 'submissions')
155+
->where($qb->expr()->in('submissions.form_id', $qb->createNamedParameter($formIdsChunk, IQueryBuilder::PARAM_INT_ARRAY)))
156+
->groupBy('submissions.form_id');
157+
158+
if ($userId !== null) {
159+
$qb->andWhere($qb->expr()->eq('submissions.user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)));
160+
}
161+
162+
$result = $qb->executeQuery();
163+
while ($row = $result->fetch()) {
164+
$counts[(int)$row['form_id']] = (int)$row['submission_count'];
165+
}
166+
$result->closeCursor();
167+
}
168+
169+
return $counts;
170+
}
171+
137172
/**
138173
* Count submissions by form with optional filters.
139174
*

‎lib/Service/FormsService.php‎

Lines changed: 45 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -247,39 +247,59 @@ public function getForm(Form $form): array {
247247
}
248248

249249
/**
250-
* Create partial form, as returned by Forms-Lists.
250+
* Create partial forms, as returned by Forms-Lists.
251+
* Submission counts are counted with at most two queries for all given forms.
251252
*
252-
* @param Form $form
253-
* @return array
253+
* @param Form[] $forms
254+
* @return list<array>
254255
* @throws IMapperException
255256
*/
256-
public function getPartialFormArray(Form $form): array {
257-
$result = [
258-
'id' => $form->getId(),
259-
'hash' => $form->getHash(),
260-
'title' => $form->getTitle(),
261-
'expires' => $form->getExpires(),
262-
'lastUpdated' => $form->getLastUpdated(),
263-
'permissions' => $this->getPermissions($form),
264-
'partial' => true,
265-
'state' => $form->getState(),
266-
'lockedBy' => $form->getLockedBy(),
267-
'lockedUntil' => $form->getLockedUntil(),
268-
];
257+
public function getPartialFormArrays(array $forms): array {
258+
$results = [];
259+
$formIdsWithResultsPermission = [];
260+
$formIdsWithoutResultsPermission = [];
261+
262+
foreach ($forms as $key => $form) {
263+
$permissions = $this->getPermissions($form);
264+
$results[$key] = [
265+
'id' => $form->getId(),
266+
'hash' => $form->getHash(),
267+
'title' => $form->getTitle(),
268+
'expires' => $form->getExpires(),
269+
'lastUpdated' => $form->getLastUpdated(),
270+
'permissions' => $permissions,
271+
'partial' => true,
272+
'state' => $form->getState(),
273+
'lockedBy' => $form->getLockedBy(),
274+
'lockedUntil' => $form->getLockedUntil(),
275+
];
276+
277+
if (in_array(Constants::PERMISSION_RESULTS, $permissions)) {
278+
$formIdsWithResultsPermission[$key] = $form->getId();
279+
} else {
280+
$formIdsWithoutResultsPermission[$key] = $form->getId();
281+
}
282+
}
269283

270284
// Append submissionCount if currentUser has permissions to see results
271-
if (in_array(Constants::PERMISSION_RESULTS, $result['permissions'])) {
272-
$result['submissionCount'] = $this->submissionMapper->countSubmissions($form->getId());
273-
} else {
274-
$userSubmissionCount = $this->submissionMapper->countSubmissions($form->getId(), $this->currentUser->getUID());
275-
if ($userSubmissionCount > 0) {
276-
$result['submissionCount'] = $userSubmissionCount;
277-
// Append `results` permission if user has submitted to the form
278-
$result['permissions'][] = Constants::PERMISSION_RESULTS;
285+
$submissionCounts = $this->submissionMapper->countSubmissionsByForms(array_values($formIdsWithResultsPermission));
286+
foreach ($formIdsWithResultsPermission as $key => $formId) {
287+
$results[$key]['submissionCount'] = $submissionCounts[$formId] ?? 0;
288+
}
289+
290+
if ($formIdsWithoutResultsPermission !== [] && $this->currentUser !== null) {
291+
$userSubmissionCounts = $this->submissionMapper->countSubmissionsByForms(array_values($formIdsWithoutResultsPermission), $this->currentUser->getUID());
292+
foreach ($formIdsWithoutResultsPermission as $key => $formId) {
293+
$userSubmissionCount = $userSubmissionCounts[$formId] ?? 0;
294+
if ($userSubmissionCount > 0) {
295+
$results[$key]['submissionCount'] = $userSubmissionCount;
296+
// Append `results` permission if user has submitted to the form
297+
$results[$key]['permissions'][] = Constants::PERMISSION_RESULTS;
298+
}
279299
}
280300
}
281301

282-
return $result;
302+
return array_values($results);
283303
}
284304

285305
/**

‎tests/Integration/DB/SubmissionMapperTest.php‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,28 @@ public function testCountSubmissionsEmptyForm(): void {
186186
$this->assertEquals(0, $count);
187187
}
188188

189+
public function testCountSubmissionsByForms(): void {
190+
$counts = $this->submissionMapper->countSubmissionsByForms([
191+
$this->testForms[0]['id'],
192+
$this->testForms[1]['id'],
193+
]);
194+
195+
$this->assertEquals([$this->testForms[0]['id'] => 3], $counts);
196+
}
197+
198+
public function testCountSubmissionsByFormsWithUser(): void {
199+
$counts = $this->submissionMapper->countSubmissionsByForms([
200+
$this->testForms[0]['id'],
201+
$this->testForms[1]['id'],
202+
], 'user1');
203+
204+
$this->assertEquals([$this->testForms[0]['id'] => 2], $counts);
205+
}
206+
207+
public function testCountSubmissionsByFormsEmptyList(): void {
208+
$this->assertEquals([], $this->submissionMapper->countSubmissionsByForms([]));
209+
}
210+
189211
public function testDeleteById(): void {
190212
// Get the first submission from the form
191213
$submissions = $this->submissionMapper->findByForm($this->testForms[0]['id']);

‎tests/Unit/Service/FormsServiceTest.php‎

Lines changed: 139 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -401,12 +401,12 @@ public function testGetPartialForm(array $expected) {
401401
$form->setLastUpdated(123456789);
402402

403403
$this->submissionMapper->expects($this->once())
404-
->method('countSubmissions')
405-
->with(42)
406-
->willReturn(123);
404+
->method('countSubmissionsByForms')
405+
->with([42])
406+
->willReturn([42 => 123]);
407407

408408
// Run the test
409-
$this->assertEquals($expected, $this->formsService->getPartialFormArray($form));
409+
$this->assertEquals($expected, $this->formsService->getPartialFormArrays([$form])[0]);
410410
}
411411

412412
public static function dataGetPartialFormShared() {
@@ -454,12 +454,143 @@ public function testGetPartialFormShared(array $expected) {
454454
->willReturn([$share]);
455455

456456
$this->submissionMapper->expects($this->once())
457-
->method('countSubmissions')
458-
->with(42)
459-
->willReturn(123);
457+
->method('countSubmissionsByForms')
458+
->with([42])
459+
->willReturn([42 => 123]);
460460

461461
// Run the test
462-
$this->assertEquals($expected, $this->formsService->getPartialFormArray($form));
462+
$this->assertEquals($expected, $this->formsService->getPartialFormArrays([$form])[0]);
463+
}
464+
465+
/**
466+
* Shared users without results permission get the count of their own
467+
* submissions and the `results` permission appended once they submitted.
468+
*/
469+
public function testGetPartialFormSharedUserSubmissions() {
470+
$expected = [
471+
'id' => 42,
472+
'hash' => 'abcdefg',
473+
'title' => 'Form 1',
474+
'expires' => 0,
475+
'lastUpdated' => 123456789,
476+
'permissions' => ['submit', 'results'],
477+
'submissionCount' => 2,
478+
'state' => 0,
479+
'partial' => true,
480+
'lockedBy' => null,
481+
'lockedUntil' => null,
482+
];
483+
484+
$form = new Form();
485+
$form->setId(42);
486+
$form->setState(0);
487+
$form->setHash('abcdefg');
488+
$form->setTitle('Form 1');
489+
$form->setOwnerId('otherUser');
490+
$form->setExpires(0);
491+
$form->setLastUpdated(123456789);
492+
493+
$share = new Share();
494+
$share->setFormId(42);
495+
$share->setPermissions([Constants::PERMISSION_SUBMIT]);
496+
$share->setShareType(IShare::TYPE_USER);
497+
$share->setShareWith('currentUser');
498+
499+
$this->shareMapper->expects($this->any())
500+
->method('findByForm')
501+
->with(42)
502+
->willReturn([$share]);
503+
504+
$this->submissionMapper->expects($this->exactly(2))
505+
->method('countSubmissionsByForms')
506+
->willReturnCallback(function (array $formIds, ?string $userId = null): array {
507+
if ($userId === null) {
508+
$this->assertSame([], $formIds);
509+
return [];
510+
}
511+
$this->assertSame([42], $formIds);
512+
$this->assertSame('currentUser', $userId);
513+
return [42 => 2];
514+
});
515+
516+
$this->assertEquals($expected, $this->formsService->getPartialFormArrays([$form])[0]);
517+
}
518+
519+
/**
520+
* Submission counts for multiple forms are fetched with a single query per
521+
* permission bucket instead of one query per form.
522+
*/
523+
public function testGetPartialFormArraysCountsInBulk() {
524+
$ownedForm = new Form();
525+
$ownedForm->setId(42);
526+
$ownedForm->setState(0);
527+
$ownedForm->setHash('abcdefg');
528+
$ownedForm->setTitle('Form 1');
529+
$ownedForm->setOwnerId('currentUser');
530+
$ownedForm->setExpires(0);
531+
$ownedForm->setLastUpdated(123456789);
532+
533+
$sharedForm = new Form();
534+
$sharedForm->setId(43);
535+
$sharedForm->setState(0);
536+
$sharedForm->setHash('hijklmn');
537+
$sharedForm->setTitle('Form 2');
538+
$sharedForm->setOwnerId('otherUser');
539+
$sharedForm->setExpires(0);
540+
$sharedForm->setLastUpdated(123456789);
541+
542+
$share = new Share();
543+
$share->setFormId(43);
544+
$share->setPermissions([Constants::PERMISSION_SUBMIT]);
545+
$share->setShareType(IShare::TYPE_USER);
546+
$share->setShareWith('currentUser');
547+
548+
$this->shareMapper->expects($this->any())
549+
->method('findByForm')
550+
->with(43)
551+
->willReturn([$share]);
552+
553+
$this->submissionMapper->expects($this->exactly(2))
554+
->method('countSubmissionsByForms')
555+
->willReturnCallback(function (array $formIds, ?string $userId = null): array {
556+
if ($userId === null) {
557+
$this->assertSame([42], $formIds);
558+
return [42 => 7];
559+
}
560+
$this->assertSame('currentUser', $userId);
561+
$this->assertSame([43], $formIds);
562+
return [];
563+
});
564+
565+
$expected = [
566+
[
567+
'id' => 42,
568+
'hash' => 'abcdefg',
569+
'title' => 'Form 1',
570+
'expires' => 0,
571+
'lastUpdated' => 123456789,
572+
'permissions' => Constants::PERMISSION_ALL,
573+
'partial' => true,
574+
'state' => 0,
575+
'lockedBy' => null,
576+
'lockedUntil' => null,
577+
'submissionCount' => 7,
578+
],
579+
[
580+
'id' => 43,
581+
'hash' => 'hijklmn',
582+
'title' => 'Form 2',
583+
'expires' => 0,
584+
'lastUpdated' => 123456789,
585+
'permissions' => ['submit'],
586+
'partial' => true,
587+
'state' => 0,
588+
'lockedBy' => null,
589+
'lockedUntil' => null,
590+
],
591+
];
592+
593+
$this->assertEquals($expected, $this->formsService->getPartialFormArrays([$ownedForm, $sharedForm]));
463594
}
464595

465596
public static function dataGetPublicForm() {

0 commit comments

Comments
 (0)