feat: store data in individual database tables #497

tamlyn · 2018-08-18T19:10:19Z

Background

Store manuscripts, teams and files in their own tables (keep users in entities)
Use knex for query building
Change some columns to arrays instead of JSONB due to a quirk in pg's query formatting.
Use , instead of . in column names (e.g. meta,title) due to dots being special in knex

Any relevant tickets

Partially addresses #473 (users and identities still to do)

How has this been tested?

Existing tests updated, new unit tests written.

diversemix · 2018-08-28T21:38:56Z

Well after a long day of trying to get the tests to work... there seems to be a few issues that need to be sorted.

The queries do not work if the tables they are joining on are empty
There are "random" errors that a given table does not exist.
There are "random" errors that a given relations do not exist.
deadlocks are detected

Ideas:

In the middle of trying to create a framework for data-access.js
Improve errors lots of them have the stack:

     at Connection.Object.<anonymous>.Connection.parseE (node_modules/pg/lib/connection.js:553:11)
     at Connection.Object.<anonymous>.Connection.parseMessage (node_modules/pg/lib/connection.js:378:19)
     at Socket.<anonymous> (node_modules/pg/lib/connection.js:119:22)

diversemix · 2018-08-28T22:34:18Z

I think I've found the problem (at 11:33pm!) ... needs a fix in create-tables.js like:

      let deleteSQL = ""
      await Promise.all(
        // TODO this is dangerous, change it
        rows.map(row => deleteSQL += `DROP TABLE "${row.tablename}" CASCADE;`),
      )
      await db.query(deleteSQL)

or something a bit more elegant!

…sweet

hem-brahmbhatt · 2018-08-30T16:17:09Z

server/xpub/entities/manuscript/resolvers.test.js

-        files: [{ filename: 'manuscript.pdf' }],
-      })
-    })
+    // it.skip('extracts title from PDF', async () => {


Warnings don't fail the build, so you don't need to comment this out

But it does stop you making a commit ;P

Ah I didn't realise, I may not be running the commit hook locally... I suggest deleting the test and addressing it in #493, rather than commenting it out.

hem-brahmbhatt · 2018-08-30T16:34:09Z

server/xpub/entities/manuscript/resolvers.test.js

@@ -28,10 +32,12 @@ describe('Submission', () => {
    const user = await User.save(userData)
    userId = user.id
    mailer.clearMails()
+    initializing = false


We shouldn't need to have an initializing parameter. Jest will wait for the async function to complete before running the tests.

You can see details here and here

You're right we shouldn't. However I'd like to leave them in - that way if anyone introduces code that makes the tests fail because of async init or tear down then we will easily know.

It may be worth using a promise to make it clearer that we're waiting for the database to be initialised. Using an assertion says to me that we're testing the database initialisation.

i.e.

let initializeDb; const initialize = async () => { replaySetup('success') await Promise.all([ fs.remove(config.get('pubsweet-server.uploads')), createTables(true), ]) const user = await User.save(userData) userId = user.id mailer.clearMails() } beforeEach(async () => { initializeDb = initialize(); }); ... it('Returns empty object when there are no manuscripts in the db', async () => { await initializeDb(); const manuscript = await Query.currentSubmission({}, {}, { user: userId }) expect(manuscript).toBe(null) })

Ooo yeah I like that 👍
.. you want to add it? @damnhipster

This initializing stuff is unnecessary noise. All it is asserting is that the beforeEach function completed before the test started but this is guaranteed by Jest already.

If a bug in the initialisation logic is introduced (e.g. forgetting to await a statement) this won't catch it because the beforeEach still completed and set initializing back to false. The most likely way that assertion could fail is if an exception is thrown in the beforeEach. But in that case it's confusing because the initialisation failed so initializing should really be false.

The promise solution is bypassing Jest's automatic waiting in favour of explicit manual waiting. Surely that's likely to lead to more bugs, not fewer.

I'd like to leave it in there... it helped me find the issue in pubsweet with the asynchronous drop tables. So it would catch issues with pubsweet? Please correct if I am wrong/you disagree.

…nore file

… test file

tamlyn · 2018-09-03T14:51:44Z

server/jest-setup.js

-  pool.testDbName = mockTestDbName
-
-  return pool
-})


Removing this stuff means we can no longer parallelise test runs (need to run jest with --runInBand to avoid errors on multi-core machines). But we have so few tests that for the time being it's only a couple of seconds faster when running in parallel. The gain in simplicity is probably worth the loss in performance.

It wasn't the simplicity ... it was the fact that jest.mock doesn't work. There are three places db.js is used in the project and the jest mock would only work for one of them. I did try to explicitly mock out the paths but to no avail. Do you know how to ensure that jest mocks everything under node_modules ? @tamlyn

No, I just had a look too and gave up. It's a hacky solution which is good when it works but hard to debug when it doesn't. That's what I means about simplicity. At least it's easier to reason about what's going on when you have a single database.

Agreed, just a shame it doesn't work. I just saw your commit (e615fc1) to put in the "runInBand" flag, looks good. I was wondering where is the best place to put in the tests for selectById (and status) to test the queries.... At the data-access level it gets tested to make sure it returns the correct object id ... but it doesn't check exactly what is returned. I'm thinking this should go in the resolve.test ...? ... what's the thinking @damnhipster @tamlyn ?

With Hem's help I realised that the root of the problem here was that pubsweet-server was present multiple times in the dependency tree. This is a problem because each one gets its own database pool and it introduces subtle problems as seen here. Ensuring that all pubsweet dependencies are always upgraded together avoids this.

I've re-enabled parallel test execution for now. It will become more valuable as the number of tests increases.

Oh wow! That is fantastic! So how was the dependency tree fixed ... we'll talk tomorrow - great work again 👍

Use DISTINCT to limit files and teams returned in queries. Recursively delete linked files and teams when deleting a manuscript. Force Jest to run tests in serial.

Reduce verbosity of data access by dynamically building SQL queries. Use comma as separator for nested columns because dot is special in Knex and colon is special in Objection.

hem-brahmbhatt · 2018-09-04T11:35:09Z

server/xpub/util.js

+    return { ...entity, [columnName]: value }
+  }, {})
+
+module.exports = { rowToEntity, entityToRow }


These functions look like they're better off living under server/xpub/entities/util.js, as they're specifically for entities.

Update teams instead of replacing them. Add some tests.

Reinstate parallel test running.

hem-brahmbhatt · 2018-09-04T16:25:13Z

server/xpub/entities/file/data-access.js

+]
+
+const dataAccess = {
+  async selectById(id) {


@diversemix In response to your comment about testing selectById, given that it's a function on the exported dataAccess object, it should be tested in dataAccess.test.js

... the issue with that is we need to create files and teams within that script. But I guess this is a reflection on the hierarchy of objects. Maybe we directly use the corresponding entity's data-access functions to do that?

diversemix · 2018-09-06T08:13:48Z

I'm trying to run the tests on this branch ... and I started with a fresh database and node_modules and I keep seeing #539

Is anyone else seeing this?

diversemix · 2018-09-06T12:41:40Z

server/xpub/entities/file/index.js

+const lodash = require('lodash')
+const dataAccess = require('./data-access')
+
+const empty = { teamMembers: [] }


This is a bug - fixing as part of the testing changes going it.

diversemix · 2018-09-06T13:00:02Z

@tamlyn I've updated with the tests and made that minor fix to the definition of empty for File

diversemix

lgtm

tamlyn · 2018-09-06T13:07:42Z

server/xpub/entities/file/data-access.test.js

+
+describe('FileAccessLayer', () => {
+  beforeEach(async () => {
+    testId = await initializeDatabase()


You're already assigning testId to the outer variable in initializeDatabase.

tamlyn and others added 3 commits August 18, 2018 19:54

feat: store data in individual database tables

eccf35c

Merge package updates from develop

02cd07b

Merge branch 'develop' into 423-sql-schema

b609053

Add the beginnings of a test framework

de72772

diversemix added 5 commits August 30, 2018 11:07

Update pubsweet version to one that includes the fix

b52e815

Remove db mocking, and fix open handles

ff05f61

Fix tests, use checkPermission, reinstate createdBy, remove email tests

a514529

Update yarn.lock as forgot to do with updating package version of pub…

5046609

…sweet

Merge branch 'develop' into 423-sql-schema

3de0a85

hem-brahmbhatt reviewed Aug 30, 2018

View reviewed changes

diversemix added 2 commits August 30, 2018 18:53

Complete the data-access tests

73942e5

Restructure tests to allow for more complex testing, and vscode to ig…

7a3807d

…nore file

diversemix mentioned this pull request Aug 31, 2018

Fix resolvers 'extracts title from PDF' test #493

Closed

diversemix added 2 commits August 31, 2018 10:32

Remove the commented-out upload test (address this later). Remove old…

0e3fab2

… test file

Fix async issue on clearDatabase

ba4609a

tamlyn commented Sep 3, 2018

View reviewed changes

tamlyn added 3 commits September 3, 2018 17:46

fix: duplicate linked entities selected

e615fc1

Use DISTINCT to limit files and teams returned in queries. Recursively delete linked files and teams when deleting a manuscript. Force Jest to run tests in serial.

refactor: use knex query builder

9f358e1

Reduce verbosity of data access by dynamically building SQL queries. Use comma as separator for nested columns because dot is special in Knex and colon is special in Objection.

refactor: use knex query builder in team data access

597fb36

hem-brahmbhatt reviewed Sep 4, 2018

View reviewed changes

fix: updating teams from ManuscriptInput

b20a1a7

Update teams instead of replacing them. Add some tests.

tamlyn changed the title ~~WIP feat: store data in individual database tables~~ feat: store data in individual database tables Sep 4, 2018

tamlyn added 3 commits September 4, 2018 16:08

refactor: move utils

ef62c8b

refactor: use knex query builder in file data access

fc02a24

fix: remove duplicate pubsweet-server dependency

b5a9b9b

Reinstate parallel test running.

hem-brahmbhatt reviewed Sep 4, 2018

View reviewed changes

diversemix reviewed Sep 6, 2018

View reviewed changes

Fix defn of empty file. Add tests for team and file entities.

aea942c

diversemix approved these changes Sep 6, 2018

View reviewed changes

tamlyn commented Sep 6, 2018

View reviewed changes

diversemix and others added 2 commits September 6, 2018 14:13

Fix double assignment of testId

6b1e994

Merge remote-tracking branch 'origin/develop' into 423-sql-schema

61cf85b

diversemix merged commit 6f73ff1 into develop Sep 6, 2018

diversemix deleted the 423-sql-schema branch September 6, 2018 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: store data in individual database tables #497

feat: store data in individual database tables #497

tamlyn commented Aug 18, 2018 •

edited

diversemix commented Aug 28, 2018 •

edited

diversemix commented Aug 28, 2018

hem-brahmbhatt Aug 30, 2018

diversemix Aug 30, 2018

hem-brahmbhatt Aug 31, 2018 •

edited

diversemix Aug 31, 2018

hem-brahmbhatt Aug 30, 2018

hem-brahmbhatt Aug 30, 2018

diversemix Aug 30, 2018

hem-brahmbhatt Aug 31, 2018

diversemix Aug 31, 2018

tamlyn Sep 3, 2018

diversemix Sep 3, 2018

tamlyn Sep 3, 2018

diversemix Sep 3, 2018

tamlyn Sep 3, 2018

diversemix Sep 4, 2018

tamlyn Sep 4, 2018

diversemix Sep 4, 2018

hem-brahmbhatt Sep 4, 2018

hem-brahmbhatt Sep 4, 2018

diversemix Sep 4, 2018

diversemix commented Sep 6, 2018 •

edited

diversemix Sep 6, 2018

diversemix commented Sep 6, 2018

diversemix left a comment

tamlyn Sep 6, 2018

feat: store data in individual database tables #497

feat: store data in individual database tables #497

Conversation

tamlyn commented Aug 18, 2018 • edited

Background

Any relevant tickets

How has this been tested?

diversemix commented Aug 28, 2018 • edited

diversemix commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hem-brahmbhatt Aug 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diversemix commented Sep 6, 2018 • edited

Choose a reason for hiding this comment

diversemix commented Sep 6, 2018

diversemix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamlyn commented Aug 18, 2018 •

edited

diversemix commented Aug 28, 2018 •

edited

hem-brahmbhatt Aug 31, 2018 •

edited

diversemix commented Sep 6, 2018 •

edited