Add filters when selecting $skiptoken submission (#1129)

Closes getodk/central#608.

lib/model/query/submissions.js

@@ -199,21 +199,36 @@ const getById = (submissionId) => ({ maybeOne }) =>
   maybeOne(sql`select * from submissions where id=${submissionId} and "deletedAt" is null`)
     .then(map(construct(Submission)));
 
-const countByFormId = (formId, draft, options = QueryOptions.none) => ({ one }) => one(sql`
+const joinOnSkiptoken = (skiptoken, formId, draft) => {
+  if (skiptoken == null) return sql``;
+  // In the case of a subtable, we fetch all submissions without pagination: we
+  // don't use $skiptoken in the query, only in application code.
+  if (skiptoken.repeatId != null) return sql``;
+  // If the submission associated with the $skiptoken has been deleted but not
+  // purged, we should still be able to use the $skiptoken. Because of that, we
+  // don't filter on "deletedAt".
+  return sql`INNER JOIN (
+  SELECT id, "createdAt"
+  FROM submissions
+  WHERE ${equals({ formId, draft })} AND "instanceId" = ${skiptoken.instanceId}
+) AS cursor ON
+  submissions."createdAt" < cursor."createdAt" OR
+  (submissions."createdAt" = cursor."createdAt" AND submissions.id < cursor.id)`;
+};
+
+const countByFormId = (formId, draft, options = QueryOptions.none) => ({ one }) => {
+  const filter = sql`${equals({ formId, draft })} AND
+"deletedAt" IS NULL AND
+${odataFilter(options.filter, odataToColumnMap)}`;
+  return one(sql`
 SELECT * FROM
-( SELECT COUNT(*) count FROM submissions
-  WHERE ${equals({ formId, draft })} AND "deletedAt" IS NULL AND ${odataFilter(options.filter, odataToColumnMap)}) AS "all"
+( SELECT COUNT(*) count FROM submissions WHERE ${filter}) AS "all"
 CROSS JOIN
 ( SELECT COUNT(*) remaining FROM submissions
-  ${options.skiptoken ? sql`
-  INNER JOIN
-  ( SELECT id, "createdAt" FROM submissions WHERE "instanceId" = ${options.skiptoken.instanceId}) AS cursor
-  ON submissions."createdAt" <= cursor."createdAt" AND submissions.id < cursor.id
-  `: sql``} 
-  WHERE ${equals({ formId, draft })} 
-  AND "deletedAt" IS NULL 
-  AND ${odataFilter(options.filter, odataToColumnMap)}) AS skiptoken
+  ${joinOnSkiptoken(options.skiptoken, formId, draft)}
+  WHERE ${filter}) AS skiptoken
 `);
+};
 
 const verifyVersion = (formId, rootId, instanceId, draft) => ({ maybeOne }) => maybeOne(sql`
 select true from submissions
@@ -349,11 +364,7 @@ inner join
   (select "submissionId", (count(id) - 1) as count from submission_defs
     group by "submissionId") as edits
   on edits."submissionId"=submission_defs."submissionId"
-${options.skiptoken && !options.skiptoken.repeatId ? sql` -- in case of subtable we are fetching all Submissions without pagination
-  inner join 
-  (select id, "createdAt" from submissions where "instanceId" = ${options.skiptoken.instanceId}) as cursor
-  on submissions."createdAt" <= cursor."createdAt" and submissions.id < cursor.id
-`: sql``}  
+${joinOnSkiptoken(options.skiptoken, formId, draft)}
 where
   ${encrypted ? sql`(form_defs."encKeyId" is null or form_defs."encKeyId" in (${sql.join(keyIds, sql`,`)})) and` : sql``}
   ${odataFilter(options.filter, options.isSubmissionsTable ? odataToColumnMap : odataSubTableToColumnMap)} and

lib/util/db.js

@@ -350,7 +350,7 @@ class QueryOptions {
   }
 
   hasPaging() {
-    return (this.offset != null) || (this.limit != null);
+    return (this.offset != null) || (this.limit != null) || (this.skiptoken != null);
   }
 
   allowArgs(...allowed) {

test/data/xml.js

@@ -529,9 +529,9 @@ module.exports = {
       three: fullInstance('withrepeat', '1.0', 'rthree', '<name>Chelsea</name><age>38</age><children><child><name>Candace</name><age>2</age></child></children>'),
     },
     simple2: {
-      one: instance('simple2', 's2one', '<name>Alice</name><age>30</age>'),
-      two: instance('simple2', 's2two', '<name>Bob</name><age>34</age>'),
-      three: instance('simple2', 's2three', '<name>Chelsea</name><age>38</age>')
+      one: fullInstance('simple2', '2.1', 's2one', '<name>Alice</name><age>30</age>'),
+      two: fullInstance('simple2', '2.1', 's2two', '<name>Bob</name><age>34</age>'),
+      three: fullInstance('simple2', '2.1', 's2three', '<name>Chelsea</name><age>38</age>')
     },
     doubleRepeat: {
       double: `<data id="doubleRepeat" version="1.0">

test/integration/api/odata.js

@@ -4,6 +4,8 @@ const testData = require('../../data/xml');
 const { dissocPath, identity } = require('ramda');
 const { QueryOptions } = require('../../../lib/util/db');
 const should = require('should');
+const { URL } = require('url');
+const { url } = require('../../../lib/util/http');
 
 // NOTE: for the data output tests, we do not attempt to extensively determine if every
 // internal case is covered; there are already two layers of tests below these, at
@@ -801,6 +803,113 @@ describe('api: /forms/:id.svc', () => {
         });
     }));
 
+    it('should support $skiptoken even if associated submission is deleted', testService(async (service, { run }) => {
+      const asAlice = await service.login('alice');
+      await asAlice.post('/v1/projects/1/forms/simple/submissions')
+        .send(testData.instances.simple.one)
+        .set('Content-Type', 'text/xml')
+        .expect(200);
+      await asAlice.post('/v1/projects/1/forms/simple/submissions')
+        .send(testData.instances.simple.two)
+        .set('Content-Type', 'text/xml')
+        .expect(200);
+      const skiptoken = await asAlice.get('/v1/projects/1/forms/simple.svc/Submissions?%24top=1')
+        .expect(200)
+        .then(({ body }) => new URL(body['@odata.nextLink']).searchParams.get('$skiptoken'));
+      QueryOptions.parseSkiptoken(skiptoken).instanceId.should.equal('two');
+      // We don't have a submission delete endpoint yet, but we should soon.
+      await run(sql`UPDATE submissions SET "deletedAt" = clock_timestamp()
+WHERE "instanceId" = 'two'`);
+      const { body: odata } = await asAlice.get(url`/v1/projects/1/forms/simple.svc/Submissions?%24skiptoken=${skiptoken}`)
+        .expect(200);
+      odata.value.length.should.equal(1);
+      odata.value[0].__id.should.equal('one');
+    }));
+
+    it('should return no submissions if $skiptoken instanceId does not exist', testService(async (service) => {
+      const asAlice = await service.login('alice');
+      await asAlice.post('/v1/projects/1/forms/simple/submissions')
+        .send(testData.instances.simple.one)
+        .set('Content-Type', 'text/xml')
+        .expect(200);
+      const skiptoken = QueryOptions.getSkiptoken({ instanceId: 'foo' });
+      const { body: odata } = await asAlice.get(url`/v1/projects/1/forms/simple.svc/Submissions?%24skiptoken=${skiptoken}`)
+        .expect(200);
+      odata.value.length.should.equal(0);
+    }));
+
+    it('should not return duplicate submissions if $skiptoken instanceId is reused', testService(async (service) => {
+      const asAlice = await service.login('alice');
+
+      // Create two submissions with instance IDs of 'one' and 'two'. Creating
+      // them in reverse order so that OData returns 'one' first.
+      await asAlice.post('/v1/projects/1/forms/simple/submissions')
+        .send(testData.instances.simple.two)
+        .set('Content-Type', 'text/xml')
+        .expect(200);
+      await asAlice.post('/v1/projects/1/forms/simple/submissions')
+        .send(testData.instances.simple.one)
+        .set('Content-Type', 'text/xml')
+        .expect(200);
+
+      // Create a draft submission to the same form using an instance ID of 'one'.
+      await asAlice.post('/v1/projects/1/forms/simple/draft')
+        .expect(200);
+      await asAlice.post('/v1/projects/1/forms/simple/draft/submissions')
+        .send(testData.instances.simple.one)
+        .set('Content-Type', 'text/xml')
+        .expect(200);
+
+      // Create a submission to a different form using an instance ID of 'one'.
+      await asAlice.post('/v1/projects/1/forms?publish=true')
+        .send(testData.forms.simple2)
+        .set('Content-Type', 'text/xml')
+        .expect(200);
+      await asAlice.post('/v1/projects/1/forms/simple2/submissions')
+        .send(testData.instances.simple2.one.replace('id="s2one"', 'id="one"'))
+        .set('Content-Type', 'text/xml')
+        .expect(200);
+
+      const { body: firstChunk } = await asAlice.get('/v1/projects/1/forms/simple.svc/Submissions?%24top=1&%24count=true')
+        .expect(200);
+      firstChunk.value.length.should.equal(1);
+      firstChunk.value[0].__id.should.equal('one');
+      firstChunk['@odata.count'].should.equal(2);
+      const skiptoken = new URL(firstChunk['@odata.nextLink']).searchParams
+        .get('$skiptoken');
+      QueryOptions.parseSkiptoken(skiptoken).instanceId.should.equal('one');
+
+      const { body: secondChunk } = await asAlice.get(url`/v1/projects/1/forms/simple.svc/Submissions?%24skiptoken=${skiptoken}&%24count=true`)
+        .expect(200);
+      secondChunk.value.length.should.equal(1);
+      secondChunk.value[0].__id.should.equal('two');
+      secondChunk['@odata.count'].should.equal(2);
+      should.not.exist(secondChunk['@odata.nextLink']);
+    }));
+
+    it('should return a matching submission whose id is after that of $skiptoken submission', testService(async (service, { run }) => {
+      const asAlice = await service.login('alice');
+      await asAlice.post('/v1/projects/1/forms/simple/submissions')
+        .send(testData.instances.simple.two)
+        .set('Content-Type', 'text/xml')
+        .expect(200);
+      await asAlice.post('/v1/projects/1/forms/simple/submissions')
+        .send(testData.instances.simple.one)
+        .set('Content-Type', 'text/xml')
+        .expect(200);
+      // Pretend like 'one' was created before 'two', but its id is greater than
+      // the id of 'two'.
+      await run(sql`UPDATE submissions SET "createdAt" = '2000-01-01' WHERE "instanceId" = 'one'`);
+      const skiptoken = await asAlice.get('/v1/projects/1/forms/simple.svc/Submissions?%24top=1')
+        .expect(200)
+        .then(({ body }) => new URL(body['@odata.nextLink']).searchParams.get('$skiptoken'));
+      QueryOptions.parseSkiptoken(skiptoken).instanceId.should.equal('two');
+      const { body: odata } = await asAlice.get(url`/v1/projects/1/forms/simple.svc/Submissions?%24skiptoken=${skiptoken}`)
+        .expect(200);
+      odata.value.length.should.equal(1);
+      odata.value[0].__id.should.equal('one');
+    }));
+
     it('should limit and filter Submissions', testService(async (service) => {
       const asAlice = await withSubmissions(service, identity);
 

test/integration/other/migrations.js

@@ -699,8 +699,7 @@ describe('database migrations from 20230512: adding entity_def_sources table', f
       .expect(200);
     await asAlice.post('/v1/projects/1/forms/simple2/submissions')
       .set('Content-Type', 'application/xml')
-      .send(testData.instances.simple2.one
-        .replace('id="simple2"', 'id="simple2" version="2.1"'))
+      .send(testData.instances.simple2.one)
       .expect(200);
     await asAlice.patch('/v1/projects/1/forms/simple2/submissions/s2one')
       .send({ reviewState: 'approved' })
@@ -725,8 +724,7 @@ describe('database migrations from 20230512: adding entity_def_sources table', f
     // Create a fifth submission that will be deleted and wont have an approval event
     await asAlice.post('/v1/projects/1/forms/simple2/submissions')
       .set('Content-Type', 'application/xml')
-      .send(testData.instances.simple2.two
-        .replace('id="simple2"', 'id="simple2" version="2.1"'))
+      .send(testData.instances.simple2.two)
       .expect(200);
 
     // ----- Manually create entities to link to these submissions as sources ----

test/unit/util/db.js

@@ -406,6 +406,7 @@ returning *`);
       (new QueryOptions()).hasPaging().should.equal(false);
       (new QueryOptions({ offset: 0 })).hasPaging().should.equal(true);
       (new QueryOptions({ limit: 0 })).hasPaging().should.equal(true);
+      (new QueryOptions({ skiptoken: 'foo' })).hasPaging().should.equal(true);
     });
 
     it('should transfer allowed args from quarantine on allowArgs', () => {