Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove slonik, insert postgres.js #564

Closed
wants to merge 10 commits into from
7 changes: 3 additions & 4 deletions lib/bin/run-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,14 @@
const { merge } = require('ramda');
const config = require('config');
const exit = require('express-graceful-exit');

global.tap = (x) => { console.log(x); return x; }; // eslint-disable-line no-console
require('../util/tap');

////////////////////////////////////////////////////////////////////////////////
// CONTAINER SETUP

// initialize our slonik connection pool.
const { slonikPool } = require('../external/slonik');
const db = slonikPool(config.get('default.database'));
const { postgres } = require('../external/postgres');
const db = postgres(config.get('default.database'));

// set up our mailer.
const env = config.get('default.env');
Expand Down
4 changes: 1 addition & 3 deletions lib/data/attachments.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,7 @@ const streamAttachments = (inStream, decryptor) => {
const writable = new Writable({
objectMode: true,
highWaterMark: 5, // the default is 16, we'll be a little more conservative.
write(x, _, done) {
const att = x.row;

write(att, _, done) {
// this sanitization means that two filenames could end up identical.
// luckily, this is not actually illegal in the zip spec; two files can live at precisely
// the same location, and the conflict is dealt with interactively by the unzipping client.
Expand Down
5 changes: 3 additions & 2 deletions lib/data/briefcase.js
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,8 @@ const streamBriefcaseCsvs = (inStream, inFields, xmlFormId, selectValues, decryp
// to provide the data to do so.
const archive = zipPart();
const name = `${sanitize(xmlFormId)}.csv`;
archive.append(PartialPipe.of(inStream, rootStream, csv()), { name });
archive.append(PartialPipe.of(inStream, rootStream, csv()), { name }, () => { archive.finalize(); });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//


{
// two passes; this first pass counts field names (so we know later whether
// to append a ~1 number).
Expand All @@ -364,7 +365,7 @@ const streamBriefcaseCsvs = (inStream, inFields, xmlFormId, selectValues, decryp
}
}
}
archive.finalize();

return archive;
};

Expand Down
3 changes: 1 addition & 2 deletions lib/data/client-audits.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,10 @@ const streamClientAudits = (inStream, form, decryptor) => {
let first = true;
const csvifier = new Transform({
objectMode: true,
transform(x, _, done) {
transform(data, _, done) {
// data here contains ClientAudit attchement info as well as associated
// submission instanceId fetched through query in
// model/query/client-audits.js
const data = x.row;

// TODO: we do not currently try/catch this block because it feels low risk.
// this may not actually be the case..
Expand Down
15 changes: 7 additions & 8 deletions lib/data/odata-filter.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
// including this file, may be copied, modified, propagated, or distributed
// except according to the terms contained in the LICENSE file.

const { sql } = require('slonik');
const { raw } = require('slonik-sql-tag-raw');
const { sql } = require('../external/postgres');
const odataParser = require('odata-v4-parser');
const Problem = require('../util/problem');

Expand All @@ -20,24 +19,24 @@ const methodCall = (fn, params) => {
// n.b. odata-v4-parser appears to already validate function name and arity.
const lowerName = fn.toLowerCase();
if (extractFunctions.includes(lowerName))
return sql`extract(${raw(lowerName)} from ${op(params[0])})`; // eslint-disable-line no-use-before-define
return sql`extract(${sql.unsafe(lowerName)} from ${op(params[0])})`; // eslint-disable-line no-use-before-define
else if (fn === 'now')
return sql`now()`;
};
const binaryOp = (left, right, operator) =>
// always use parens to ensure the original AST op precedence.
sql`(${op(left)} ${raw(operator)} ${op(right)})`; // eslint-disable-line no-use-before-define
sql`(${op(left)} ${sql.unsafe(operator)} ${op(right)})`; // eslint-disable-line no-use-before-define

const op = (node) => {
if (node.type === 'FirstMemberExpression') {
if (node.raw === '__system/submissionDate') {
return sql.identifier([ 'submissions', 'createdAt' ]); // TODO: HACK HACK
return sql('submissions.createdAt'); // TODO: HACK HACK
} else if (node.raw === '__system/updatedAt') {
return sql.identifier([ 'submissions', 'updatedAt' ]); // TODO: HACK HACK
return sql('submissions.updatedAt'); // TODO: HACK HACK
} else if (node.raw === '__system/submitterId') {
return sql.identifier([ 'submissions', 'submitterId' ]); // TODO: HACK HACK
return sql('submissions.submitterId'); // TODO: HACK HACK
} else if (node.raw === '__system/reviewState') {
return sql.identifier([ 'submissions', 'reviewState' ]); // TODO: HACK HACK
return sql('submissions.reviewState'); // TODO: HACK HACK
} else {
throw Problem.internal.unsupportedODataField({ at: node.position, text: node.raw });
}
Expand Down
42 changes: 42 additions & 0 deletions lib/external/postgres.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright 2022 ODK Central Developers
// See the NOTICE file at the top-level directory of this distribution and at
// https://github.com/getodk/central-backend/blob/master/NOTICE.
// This file is part of ODK Central. It is subject to the license terms in
// the LICENSE file found in the top-level directory of this distribution and at
// https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central,
// including this file, may be copied, modified, propagated, or distributed
// except according to the terms contained in the LICENSE file.

// CRCRCR uhhhh confusing naming maybe idk
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm

const _postgres = require('postgres');
const { connectionString } = require('../util/db');

const options = {
// when saving to the database we transform all undefined to null rather than
// throw. this ought to be safe at time of writing because it's exactly what
// we did with slonik. possibly someday with better hygiene this can go away.
transform: { undefined: null },
types: {
// the functions here are how postgres implements them for numerics.
// the issue is just that for range safety reasons they do not include
// bigint in their default impl, which is a problem we are unlikely to have.
// the practical problem is that count() in postgres yields a bigint.
bigint: { to: 0, from: [ 20 ], serialize: (x => '' + x), parse: (x => +x) },

// we don't want to automatically assume all non-true values can be safely
// equal to 'f', since we can take user input here.
boolean: {
to: 16, from: 16,
serialize: (x => (x === true ? 't' : x === false ? 'f' : x)),
parse: (x => x === 't')
}
}
};
const postgres = (config) => _postgres(connectionString(config), options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but I think we'll need to do something more here to support SSL connections, which we currently support. From a quick glance, it looks like Slonik and postgres.js accept SSL options differently. For Slonik, we pass ?ssl=true as part of the connection string. However, the only mention of SSL that I see in the postgres.js readme has to do with an ssl option that's specified apart from the connection string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#433 is about Slonik, but I think has some relevant discussion.


// turns out you can get the templater just by omitting connection info completely.
// and p/postgres is happy to mix template literals across "connections".
const sql = _postgres(undefined, options);

module.exports = { postgres, sql, options };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm options


30 changes: 0 additions & 30 deletions lib/external/slonik.js

This file was deleted.

8 changes: 4 additions & 4 deletions lib/model/container.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

const { merge, head } = require('ramda');
const { postgresErrorToProblem, queryFuncs } = require('../util/db');
const { resolve, ignoringResult } = require('../util/promise');
const { ignoringResult } = require('../util/promise');


////////////////////////////////////////////////////////////////////////////////
Expand All @@ -28,7 +28,7 @@ const queryModuleBuilder = (definition, container) => {
module[methodName] = (...args) => {
const fn = definition[methodName];
const result = fn(...args)(container);
const wrapped = (result.pipe != null) ? resolve(result) :
const wrapped = (result.pipe != null) ? result :
(result.catch != null) ? result.catch(postgresErrorToProblem) :
result; // eslint-disable-line indent

Expand Down Expand Up @@ -59,8 +59,8 @@ const queryModuleBuilder = (definition, container) => {
class Container {
constructor(...resources) { Object.assign(this, ...resources); }
transacting(proc) {
if (this.isTransacting === true) return proc(this);
return this.db.transaction((trxn) => proc(this.with({ db: trxn, isTransacting: true })));
if (this.isTransacting === true) return proc(this); // needed due to testing.
return this.db.begin((trxn) => proc(this.with({ db: trxn, isTransacting: true })));
}
}

Expand Down
5 changes: 2 additions & 3 deletions lib/model/frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 { raw } = require('slonik-sql-tag-raw');
const { pick, without } = require('ramda');
const uuid = require('uuid/v4');
const { sql } = require('../external/postgres');
const { pickAll } = require('../util/util');
const Option = require('../util/option');

Expand Down Expand Up @@ -62,9 +62,8 @@ class Frame {
{ /* eslint-disable no-shadow */
// TODO: precomputing is good but this is sort of dirty :/
const Frame = class extends this { static get def() { return def; } };
Frame.fieldlist = raw(def.fields.map((s) => `"${s}"`).join(','));
Frame.fieldlist = sql.unsafe(def.fields.map((s) => `"${s}"`).join(','));
Frame.insertfields = without([ 'id' ], def.fields);
Frame.insertlist = raw(Frame.insertfields.map((s) => `"${s}"`).join(','));
Frame.hasCreatedAt = def.fields.includes('createdAt');
Frame.hasUpdatedAt = def.fields.includes('updatedAt');
return Frame;
Expand Down
2 changes: 1 addition & 1 deletion lib/model/frames.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 { sql } = require('slonik');
const { sql } = require('../external/postgres');
const { Frame, table, readable, writable, aux, species, embedded } = require('./frame');
const { isBlank } = require('../util/util');
const Option = require('../util/option');
Expand Down
2 changes: 1 addition & 1 deletion lib/model/query/actees.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// except according to the terms contained in the LICENSE file.

const uuid = require('uuid/v4');
const { sql } = require('slonik');
const { sql } = require('../../external/postgres');
const { Actee } = require('../frames');
const { construct } = require('../../util/util');

Expand Down
2 changes: 1 addition & 1 deletion lib/model/query/actors.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 { sql } = require('slonik');
const { sql } = require('../../external/postgres');
const { map } = require('ramda');
const { insert, markDeleted } = require('../../util/db');
const { resolve } = require('../../util/promise');
Expand Down
2 changes: 1 addition & 1 deletion lib/model/query/analytics.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// except according to the terms contained in the LICENSE file.

const config = require('config');
const { sql } = require('slonik');
const { sql } = require('../../external/postgres');
const { clone } = require('ramda');
const { metricsTemplate } = require('../../data/analytics');

Expand Down
8 changes: 4 additions & 4 deletions lib/model/query/assignments.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 { sql } = require('slonik');
const { sql } = require('../../external/postgres');
const { Actor, Assignment } = require('../frames');
const { extender, equals, QueryOptions } = require('../../util/db');
const { getOrReject } = require('../../util/promise');
Expand Down Expand Up @@ -36,9 +36,9 @@ const grantSystem = (actor, systemName, actee) => ({ Assignments, Roles }) =>
.then(getOrReject(Problem.internal.missingSystemRow('role')))
.then((role) => Assignments.grant(actor, role, actee));

const _revoke = (actor, roleId, acteeId) => ({ db }) =>
db.query(sql`delete from assignments where ${equals({ actorId: actor.id, roleId, acteeId })}`)
.then(({ rowCount }) => Number(rowCount) > 0);
const _revoke = (actor, roleId, acteeId) => ({ q }) =>
q(sql`delete from assignments where ${equals({ actorId: actor.id, roleId, acteeId })}`)
.then(({ count }) => count > 0);

_revoke.audit = (actor, roleId, acteeId) => (log) => {
if (actor.type === 'singleUse') return null;
Expand Down
8 changes: 4 additions & 4 deletions lib/model/query/audits.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@
// including this file, may be copied, modified, propagated, or distributed
// except according to the terms contained in the LICENSE file.

const { sql } = require('slonik');
const { sql } = require('../../external/postgres');
const { map } = require('ramda');
const { Actee, Actor, Audit, Form, Project } = require('../frames');
const { extender, equals, page, QueryOptions } = require('../../util/db');
const { extender, equals, joinSqlStr, page, QueryOptions } = require('../../util/db');
const Option = require('../../util/option');
const { construct } = require('../../util/util');

Expand All @@ -25,7 +25,7 @@ const log = (actor, action, actee, details) => ({ run, context }) => {

return run(sql`
insert into audits ("actorId", action, "acteeId", details, notes, "loggedAt", processed, failures)
values (${actorId}, ${action}, ${acteeId}, ${(details == null) ? null : JSON.stringify(details)}, ${notes}, clock_timestamp(), ${processed}, 0)`);
values (${actorId}, ${action}, ${acteeId}, ${details}, ${notes}, clock_timestamp(), ${processed}, 0)`);
};


Expand Down Expand Up @@ -63,7 +63,7 @@ const auditFilterer = (options) => {
options.ifArg('start', (start) => result.push(sql`"loggedAt" >= ${start}`));
options.ifArg('end', (end) => result.push(sql`"loggedAt" <= ${end}`));
options.ifArg('action', (action) => result.push(actionCondition(action)));
return (result.length === 0) ? sql`true` : sql.join(result, sql` and `);
return (result.length === 0) ? sql`true` : joinSqlStr(result, sql` and `);
};

const _get = extender(Audit)(Option.of(Actor), Option.of(Actor.alias('actee_actor', 'acteeActor')), Option.of(Form), Option.of(Form.Def), Option.of(Project), Option.of(Actee))((fields, extend, options) => sql`
Expand Down
2 changes: 1 addition & 1 deletion lib/model/query/auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 { sql } = require('slonik');
const { sql } = require('../../external/postgres');
const { compose, uniq, flatten, map } = require('ramda');
const { Actor, Session } = require('../frames');
const { resolve, reject } = require('../../util/promise');
Expand Down
4 changes: 2 additions & 2 deletions lib/model/query/blobs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 { sql } = require('slonik');
const { sql } = require('../../external/postgres');
const { map } = require('ramda');
const { Blob } = require('../frames');
const { construct } = require('../../util/util');
Expand All @@ -22,7 +22,7 @@ const { construct } = require('../../util/util');
const ensure = (blob) => ({ oneFirst }) => oneFirst(sql`
with ensured as
(insert into blobs (sha, md5, content, "contentType") values
(${blob.sha}, ${blob.md5}, ${sql.binary(blob.content)}, ${blob.contentType || null})
(${blob.sha}, ${blob.md5}, ${blob.content}, ${blob.contentType})
on conflict (sha) do update set sha = ${blob.sha}
returning id)
select id from ensured`);
Expand Down
6 changes: 3 additions & 3 deletions lib/model/query/client-audits.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
// including this file, may be copied, modified, propagated, or distributed
// except according to the terms contained in the LICENSE file.

const { sql } = require('slonik');
const { insertMany, QueryOptions } = require('../../util/db');
const { sql } = require('../../external/postgres');
const { insertMany, QueryOptions, joinSqlStr } = require('../../util/db');
const { odataFilter } = require('../../data/odata-filter');


Expand All @@ -20,7 +20,7 @@ const existsForBlob = (blobId) => ({ maybeOne }) =>

// TODO: copy-pasted from query/submission-attachments.js
const keyIdCondition = (keyIds) =>
sql.join((((keyIds == null) || (keyIds.length === 0)) ? [ -1 ] : keyIds), sql`,`);
joinSqlStr((((keyIds == null) || (keyIds.length === 0)) ? [ -1 ] : keyIds), sql`,`);

const streamForExport = (formId, draft, keyIds, options = QueryOptions.none) => ({ stream }) => stream(sql`
select client_audits.*, blobs.content, submissions."instanceId", "localKey", "keyId", index, submissions."instanceId" from submission_defs
Expand Down
2 changes: 1 addition & 1 deletion lib/model/query/comments.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 { sql } = require('slonik');
const { sql } = require('../../external/postgres');
const { map } = require('ramda');
const { Actor, Comment } = require('../frames');
const { extender, insert, QueryOptions } = require('../../util/db');
Expand Down
Loading