Merge branch 'master' into features/710-entity-restore-purge

.eslintrc.json

@@ -28,7 +28,7 @@
     "no-else-return": "off",
     "no-multiple-empty-lines": "off",
     "no-nested-ternary": "off",
-    "no-only-tests/no-only-tests": "error",
+    "no-only-tests/no-only-tests": [ "error", { "block": [ "describe", "it", "describeMigration" ] } ],
     "no-restricted-syntax": "off",
     "no-underscore-dangle": "off",
     "nonblock-statement-body-position": "off",

.github/workflows/db-migrations.yml

@@ -0,0 +1,51 @@
+name: Database Migrations
+
+on:
+  pull_request:
+    paths:
+    - .github/workflows/db-migrations.yml
+    - lib/bin/create-docker-databases.js
+    - lib/model/migrations/**
+    - test/db-migrations/**
+    - package.json
+    - package-lock.json
+    - Makefile
+  push:
+    paths:
+    - .github/workflows/db-migrations.yml
+    - lib/bin/create-docker-databases.js
+    - lib/model/migrations/**
+    - test/db-migrations/**
+    - package.json
+    - package-lock.json
+    - Makefile
+
+jobs:
+  db-migration-tests:
+    timeout-minutes: 2
+    # TODO should we use the same container as circle & central?
+    runs-on: ubuntu-latest
+    services:
+      # see: https://docs.github.com/en/enterprise-server@3.5/actions/using-containerized-services/creating-postgresql-service-containers
+      postgres:
+        image: postgres:14.10
+        env:
+          POSTGRES_PASSWORD: odktest
+        ports:
+          - 5432:5432
+        # Set health checks to wait until postgres has started
+        options: >-
+          --health-cmd pg_isready
+          --health-interval 10s
+          --health-timeout 5s
+          --health-retries 5
+    steps:
+    - uses: actions/checkout@v4
+    - name: Set node version
+      uses: actions/setup-node@v4
+      with:
+        node-version: 22.12.0
+        cache: 'npm'
+    - run: npm ci
+    - run: node lib/bin/create-docker-databases.js
+    - run: make test-db-migrations

.github/workflows/oidc-e2e.yml

@@ -40,4 +40,4 @@ jobs:
       uses: actions/upload-artifact@v4
       with:
         name: Playwright Screenshots
-        path: test/e2e/oidc/playwright-results/**/*.png
+        path: test/e2e/oidc/playwright-tests/results/**/*.png

.mocharc.js

@@ -9,6 +9,7 @@
 
 module.exports = {
   ignore: [
+    'test/db-migrations/**',
     'test/e2e/**',
   ],
 };

Makefile

@@ -107,6 +107,12 @@ test: lint
 test-ci: lint
 	BCRYPT=insecure npx mocha --recursive --reporter test/ci-mocha-reporter.js
 
+.PHONY: test-db-migrations
+test-db-migrations:
+	NODE_CONFIG_ENV=db-migration-test npx mocha --bail --sort --timeout=20000 \
+	    --require test/db-migrations/mocha-setup.js \
+	    ./test/db-migrations/**/*.spec.js
+
 .PHONY: test-fast
 test-fast: node_version
 	BCRYPT=insecure npx mocha --recursive --fgrep @slow --invert

config/db-migration-test.json

@@ -0,0 +1,10 @@
+{
+  "default": {
+    "database": {
+      "host": "localhost",
+      "user": "jubilant",
+      "password": "jubilant",
+      "database": "jubilant_test"
+    }
+  }
+}

lib/bin/check-migrations.js

@@ -7,11 +7,11 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
-const { withDatabase, checkMigrations } = require('../model/migrate');
+const { withKnex, checkMigrations } = require('../model/migrate');
 
 (async () => {
   try {
-    await withDatabase(require('config').get('default.database'))(checkMigrations);
+    await withKnex(require('config').get('default.database'))(checkMigrations);
   } catch (err) {
     console.error('Error:', err.message);
     process.exit(1);

lib/bin/check-open-db-queries.js

@@ -7,11 +7,11 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
-const { withDatabase } = require('../model/migrate');
+const { withKnex } = require('../model/migrate');
 
 (async () => {
   try {
-    const { rows } = await withDatabase(require('config').get('default.database'))((db) =>
+    const { rows } = await withKnex(require('config').get('default.database'))((db) =>
       db.raw('SELECT COUNT(*) FROM pg_stat_activity WHERE usename=CURRENT_USER'));
     const queryCount = rows[0].count - 1; // the count query will appear as one of the open queries
 

lib/bin/run-migrations.js

@@ -7,11 +7,11 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
-const { withDatabase, migrate } = require('../model/migrate');
+const { withKnex, migrate } = require('../model/migrate');
 
 (async () => {
   try {
-    await withDatabase(require('config').get('default.database'))(migrate);
+    await withKnex(require('config').get('default.database'))(migrate);
   } catch (err) {
     console.error('Error:', err.message);
     process.exit(1);

lib/external/s3.js

@@ -118,7 +118,9 @@ const init = (config) => {
   // * https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html#API_GetObject_RequestSyntax
   const getRespHeaders = (filename, { contentType }) => ({
     'response-content-disposition': contentDisposition(filename),
-    'response-content-type': contentType,
+    // "null" is a questionable content-type, but matches current central behaviour
+    // See: https://github.com/getodk/central-backend/pull/1352
+    'response-content-type': contentType || 'null',
   });
 
   async function urlForBlob(filename, blob) {

lib/model/knexfile.js

@@ -28,10 +28,10 @@ NODE_CONFIG_DIR=../../config DEBUG=knex:query,knex:bindings npx knex migrate:up
 */
 
 const config = require('config');
-const { connectionObject } = require('../util/db');
+const { knexConnection } = require('../util/db');
 
 module.exports = {
   client: 'pg',
-  connection: connectionObject(config.get('default.database'))
+  connection: knexConnection(config.get('default.database'))
 };
 

lib/model/migrate.js

@@ -11,14 +11,14 @@
 // top-level operations with a database, like migrations.
 
 const knex = require('knex');
-const { connectionObject } = require('../util/db');
+const { knexConnection } = require('../util/db');
 
 // Connects to the postgres database specified in configuration and returns it.
-const connect = (config) => knex({ client: 'pg', connection: connectionObject(config) });
+const knexConnect = (config) => knex({ client: 'pg', connection: knexConnection(config) });
 
 // Connects to a database, passes it to a function for operations, then ensures its closure.
-const withDatabase = (config) => (mutator) => {
-  const db = connect(config);
+const withKnex = (config) => (mutator) => {
+  const db = knexConnect(config);
   return mutator(db).finally(() => db.destroy());
 };
 
@@ -33,5 +33,5 @@ const checkMigrations = (db) => db.migrate.list({ directory: `${__dirname}/migra
       process.exitCode = 1;
   });
 
-module.exports = { checkMigrations, connect, withDatabase, migrate };
+module.exports = { checkMigrations, knexConnect, withKnex, migrate };
 

lib/model/migrations/.eslintrc.json

@@ -0,0 +1,6 @@
+{
+  "extends": "../../../.eslintrc.json",
+  "rules": {
+    "no-restricted-modules": [ "error", { "patterns": [ "../*" ] } ]
+  }
+}

lib/model/migrations/20180727-02-add-md5-to-blobs.js

@@ -8,7 +8,7 @@
 // except according to the terms contained in the LICENSE file.
 //
 
-const { md5sum } = require('../../util/crypto');
+const { md5sum } = require('../../util/crypto'); // eslint-disable-line no-restricted-modules
 
 const up = (knex) =>
   knex.schema.table('blobs', (blobs) => { blobs.string('md5', 32); })

lib/model/migrations/20180727-03-add-form-attachments-table.js

@@ -23,7 +23,7 @@ const up = (knex) =>
 
     fa.index([ 'formId' ]);
   }).then(() => {
-    const { expectedFormAttachments } = require('../../data/schema');
+    const { expectedFormAttachments } = require('../../data/schema'); // eslint-disable-line no-restricted-modules
     const { uniq, pluck } = require('ramda');
 
     // now add all expected attachments on extant forms.

lib/model/migrations/20190520-01-add-form-versioning.js

@@ -7,7 +7,7 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
-const { shasum, sha256sum } = require('../../util/crypto');
+const { shasum, sha256sum } = require('../../util/crypto'); // eslint-disable-line no-restricted-modules
 const assert = require('assert').strict;
 
 const check = (message, query) =>

lib/model/migrations/20191007-01-backfill-client-audits.js

@@ -7,9 +7,9 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
-const { parseClientAudits } = require('../../data/client-audits');
-const { getFormFields } = require('../../data/schema');
-const { traverseXml, findOne, root, node, text } = require('../../util/xml');
+const { parseClientAudits } = require('../../data/client-audits'); // eslint-disable-line no-restricted-modules
+const { getFormFields } = require('../../data/schema'); // eslint-disable-line no-restricted-modules
+const { traverseXml, findOne, root, node, text } = require('../../util/xml'); // eslint-disable-line no-restricted-modules
 
 const up = (db) => new Promise((resolve, reject) => {
   const work = [];

lib/model/migrations/20191231-02-add-schema-storage.js

@@ -7,7 +7,7 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
-const { getFormFields } = require('../../data/schema');
+const { getFormFields } = require('../../data/schema'); // eslint-disable-line no-restricted-modules
 
 const up = async (db) => {
   await db.schema.createTable('form_fields', (fields) => {
@@ -51,7 +51,7 @@ const up = async (db) => {
               // this config hardcoding would be dangerous with tests except that
               // tests will never invoke this path.
               const config = require('config').get('default.database');
-              const db2 = require('../migrate').connect(config);
+              const db2 = require('../migrate').knexConnect(config); // eslint-disable-line no-restricted-modules
               return db2.select('projectId', 'xmlFormId').from('forms').where({ currentDefId: formDefId })
                 .then(([{ projectId, xmlFormId }]) => {
                   process.stderr.write(`\n!!!!\nThe database upgrade to v0.8 has failed because the Form '${xmlFormId}' in Project ${projectId} has an invalid schema. It tries to bind multiple instance nodes at the path ${path}.\n!!!!\n\n`);

lib/model/migrations/20200220-01-repair-submission-parsing.js

@@ -7,7 +7,7 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
-const { Submission } = require('../frames');
+const { Submission } = require('../frames'); // eslint-disable-line no-restricted-modules
 
 const up = async (db) => {
   const work = [];

lib/model/migrations/20210120-01-instance-names.js

@@ -7,7 +7,7 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
-const { Submission } = require('../frames');
+const { Submission } = require('../frames'); // eslint-disable-line no-restricted-modules
 
 const up = async (db) => {
   await db.schema.table('submission_defs', (sds) => {

lib/model/migrations/20211008-01-track-select-many-options.js

@@ -8,10 +8,10 @@
 // except according to the terms contained in the LICENSE file.
 
 const { map } = require('ramda');
-const { getFormFields } = require('../../data/schema');
-const { getSelectMultipleResponses } = require('../../data/submission');
-const { Form } = require('../frames');
-const { construct } = require('../../util/util');
+const { getFormFields } = require('../../data/schema'); // eslint-disable-line no-restricted-modules
+const { getSelectMultipleResponses } = require('../../data/submission'); // eslint-disable-line no-restricted-modules
+const { Form } = require('../frames'); // eslint-disable-line no-restricted-modules
+const { construct } = require('../../util/util'); // eslint-disable-line no-restricted-modules
 
 const up = async (db) => {
   // add select many flag, options field to fields

lib/model/migrations/20230109-01-add-form-schema.js

@@ -7,7 +7,7 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
-const { getFormFields, compare } = require('../../data/schema');
+const { getFormFields, compare } = require('../../data/schema'); // eslint-disable-line no-restricted-modules
 
 /* Steps of this migration
   1. remove check field collision trigger

lib/model/query/forms.js

@@ -242,14 +242,16 @@ const createVersion = (partial, form, publish, duplicating = false) => async ({
       // skip checking for structural change if duplicating because user has already
       // been warning at the time of form definition upload
       if (!duplicating) {
-        await Forms.checkStructuralChange(prevFields, fields)
-          .then(Forms.rejectIfWarnings);
+        await Forms.checkStructuralChange(prevFields, fields);
       }
       // If we haven't been rejected or warned yet, make a new schema id
       schemaId = await Forms._newSchema();
     }
   }
 
+  // Let's check for warnings before pushing to Enketo or to DB
+  await Forms.rejectIfWarnings();
+
   // If not publishing, check whether there is an existing draft that we have access to.
   // If not, generate a draft token and enketoId.
   let { draftToken, enketoId } = form.def;

lib/model/query/submission-attachments.js

@@ -107,12 +107,12 @@ const upsert = (def, files) => ({ Blobs, SubmissionAttachments }) =>
       const present = files.filter((file) => lookup.has(file.fieldname));
       return Promise.all(present
         .map((file) => Blobs.ensure(Blob.fromBuffer(file.buffer, file.mimetype))
-          .then((blobId) => SubmissionAttachments.attach(def, file.fieldname, blobId))));
+          .then((blobId) => SubmissionAttachments.attach(def.id, file.fieldname, blobId))));
     });
 
-const attach = (def, name, blobId) => ({ run }) => run(sql`
+const attach = (defId, name, blobId) => ({ run }) => run(sql`
 update submission_attachments set "blobId"=${blobId}
-where "submissionDefId"=${def.id} and name=${name}`);
+where "submissionDefId"=${defId} and name=${name}`);
 
 // TODO: this is currently audit logged in resource/submissions. probably deal w it here instead.
 

lib/model/query/submissions.js

@@ -18,6 +18,7 @@ const { blankStringToNull, construct } = require('../../util/util');
 const Problem = require('../../util/problem');
 const { streamEncBlobs } = require('../../util/blob');
 const { PURGE_DAY_RANGE } = require('../../util/constants');
+const Option = require('../../util/option');
 
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -269,8 +270,8 @@ where submissions."instanceId"=${instanceId}
   and submission_defs.current=true`)
   .then(map((row) => new Submission(row, { def: new Submission.Def({ id: row.defId }) })));
 
-const getCurrentDefByIds = (projectId, xmlFormId, instanceId, draft) => ({ maybeOne }) => maybeOne(sql`
-select submission_defs.* from submission_defs
+const _buildGetCurrentSql = (cols, projectId, xmlFormId, instanceId, draft) => sql`
+select ${cols} from submission_defs
 inner join
   (select submissions.id, "instanceId" from submissions
     inner join
@@ -280,8 +281,19 @@ inner join
     where submissions."deletedAt" is null and draft=${draft}) as submissions
   on submissions.id=submission_defs."submissionId"
 where submissions."instanceId"=${instanceId} and current=true
-limit 1`)
-  .then(map(construct(Submission.Def)));
+limit 1`;
+
+const getCurrentDefColByIds = (col, projectId, xmlFormId, instanceId, draft) => ({ maybeOneFirst }) =>
+  maybeOneFirst(_buildGetCurrentSql(sql.identifier(['submission_defs', col]), projectId, xmlFormId, instanceId, draft))
+    .then(map(Option.of));
+
+const getCurrentDefColsByIds = (cols, projectId, xmlFormId, instanceId, draft) => ({ maybeOne }) =>
+  maybeOne(_buildGetCurrentSql(sql.join(cols.map(col => sql.identifier(['submission_defs', col])), sql`,`), projectId, xmlFormId, instanceId, draft))
+    .then(map(Option.of));
+
+const getCurrentDefByIds = (projectId, xmlFormId, instanceId, draft) => ({ maybeOne }) =>
+  maybeOne(_buildGetCurrentSql(sql`submission_defs.*`, projectId, xmlFormId, instanceId, draft))
+    .then(map(construct(Submission.Def)));
 
 const getDefById = (submissionDefId) => ({ maybeOne }) => maybeOne(sql`
 select submission_defs.* from submission_defs
@@ -475,7 +487,7 @@ module.exports = {
   setSelectMultipleValues, getSelectMultipleValuesForExport,
   getByIdsWithDef, getSubAndDefById,
   getByIds, getAllForFormByIds, getById, countByFormId, verifyVersion,
-  getDefById, getCurrentDefByIds, getAnyDefByFormAndInstanceId, getDefsByFormAndLogicalId, getDefBySubmissionAndInstanceId, getRootForInstanceId,
+  getDefById, getCurrentDefByIds, getCurrentDefColByIds, getCurrentDefColsByIds, getAnyDefByFormAndInstanceId, getDefsByFormAndLogicalId, getDefBySubmissionAndInstanceId, getRootForInstanceId,
   getDeleted,
   streamForExport, getForExport
 };