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

feat: automated test isolation #952

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 181 additions & 4 deletions db-service/lib/common/DatabaseService.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,192 @@
const infer = require('../infer')
const cds = require('@sap/cds')

const databaseModel = cds.linked({
definitions: {
schemas: new cds.entity({
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a more distinct name, like cds_test_schemas to avoid clashes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The model is inside the database isolation. It actually clashes with SYS.SCHEMAS in HANA, but as this is isolated it doesn't matter.

  • SYS
    • Database 1 <- database model
      • Tenant 1 <- application model
      • ...
    • ...

kind: 'entity',
elements: {
tenant: { type: 'String', key: true },
source: { type: 'String' },
available: { type: 'Boolean' },
started: { type: 'Timestamp' },
},
}),
}
})
databaseModel.meta = {}

/** @typedef {unknown} DatabaseDriver */

class DatabaseService extends cds.Service {

init() {
async init() {
cds.on('shutdown', () => this.disconnect())
if (Object.getOwnPropertyDescriptor(cds, 'test') || this.options.isolate) {

Choose a reason for hiding this comment

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

Isn't cds.test always available? So we would also run thus code for productive server start,,

Choose a reason for hiding this comment

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

should such test DB setup not be a module outside of the productive code?

await this._isolate()
}
return super.init()
}

async _isolate() {
const { options = {} } = cds
const fts = cds.requires.toggles && cds.resolve(cds.env.features.folders)
const src = [options.from || '*', ...(fts || [])]
const fullSrc = [cds.root, ...src.flat()]

const isolate = { src }
if (typeof this.database === 'function' && typeof this.tenant === 'function') {
const hash = str => {
const { createHash } = require('crypto')
const hash = createHash('sha1')
hash.update(str)
return hash.digest('hex')
}

const isolation = process.env.TRAVIS_JOB_ID || process.env.GITHUB_RUN_ID || require('os').userInfo().username || 'test_db'
const srchash = hash(fullSrc.join('/'))
// Create one database for each overall test execution
isolate.database = 'D' + hash(isolation)
// Create one tenant for each model source definition
isolate.tenant = 'T' + srchash
// Track source definition hash
isolate.source = srchash

// Create new database isolation
await this.database(isolate)

let isnew = false
try {
await this.tx(tx => tx.run(CREATE(databaseModel.definitions.schemas))).catch(() => { })
await this.tx(async tx => {
tx.model = databaseModel
// await tx.run(DELETE.from('schemas').where`true=true`)
// If insert works the schema does not yet exist and this client has won the race and can deploy the contents
await tx.run(INSERT({ tenant: isolate.tenant, source: isolate.source, available: false, started: new Date() }).into('schemas'))
isnew = true
})
} catch (err) {

Check warning on line 70 in db-service/lib/common/DatabaseService.js

View workflow job for this annotation

GitHub Actions / Tests (22)

'err' is defined but never used
// If the schema already exists wait for the row to be updated with available=true
let available = []
while (available.length === 0) {
await this.tx(async tx => {
tx.model = databaseModel
available = await tx.run(SELECT.from('schemas').where`tenant=${isolate.tenant} and available=${true}`)
})
}
}

// Create/Activate tenant isolation in database
await this.tenant(isolate)

if (isnew) {
await this._isolate_deploy(isolate)
await this.database(isolate)
await this.tx(async tx => {
tx.model = databaseModel
await tx.run(UPDATE('schemas').where`tenant=${isolate.tenant}`.with({ available: true, started: new Date() }))
})
await this.tenant(isolate)
}
} else {
await this._isolate_deploy(isolate)
}

if (typeof this.database === 'function' && typeof this.tenant === 'function') {
this._modified = {}
this.before(['*'], async (req) => {
if (
!req.query ||
req.query?.SELECT ||
(typeof req.query === 'string' && /^(BEGIN|COMMIT|ROLLBACK|SELECT)/i.test(req.query))
) return // Ignore reading requests
if (req.target) this._modified[req.target.name] = true
if (req.tx._isolating) return req.tx._isolating
if (this._isolating) return

// Add modification tracking for deep-queries internal calls
for (const fn of ['onSIMPLE', 'onUPDATE', 'onINSERT']) {
const org = this[fn]
this[fn] = function (req) {
if (req.query?.target) this._modified[req.query.target.name] = true
return org.apply(this, arguments)
}
}

this._isolating = true
return (req.tx._isolating = req.tx.commit()
.then(() => this._isolate_write(isolate))
.then(() => {
return req.tx.begin()
}))
})
}
}

async _isolate_write(isolate) {
await this.database(isolate)

let isnew = false
await this.tx(async tx => {
tx.model = databaseModel
const schemas = await tx.run(SELECT.from('schemas').where`tenant!=${isolate.tenant} and source=${isolate.source} and available=${true}`.forUpdate().limit(1))
if (schemas.length) {
const tenant = isolate.tenant = schemas[0].tenant
await tx.run(UPDATE('schemas').where`tenant=${tenant}`.with({ available: false, started: new Date() }))
} else {
isolate.tenant = 'T' + cds.utils.uuid()
await tx.run(INSERT({ tenant: isolate.tenant, source: isolate.source, available: false, started: new Date() }).into('schemas'))
isnew = true
}
delete this._modified.schemas // REVISIT: make sure to not track schemas modifications
})

await this.tenant(isolate)

if (isnew) await this._isolate_deploy(isolate)

// Release schema for follow up test runs
cds.on('shutdown', async () => {
try {
// Clean tenant entities
await this.tx(async tx => {
await tx.begin()
for (const entity in this._modified) {
const query = DELETE(entity).where`true=true`
if (!query.target._unresolved) await tx.onSIMPLE({ query }) // Skip deep delete
}
// UPSERT all data sources again
await cds.deploy.data(tx, tx.model, { schema_evolution: 'auto' })
})

await this.database(isolate) // switch back to database level
await this.tx(tx => {
tx.model = databaseModel
return UPDATE('schemas').where`tenant=${isolate.tenant}`.with({ available: true })
})
await this.disconnect()
} catch (err) {

Check warning on line 170 in db-service/lib/common/DatabaseService.js

View workflow job for this annotation

GitHub Actions / Tests (22)

'err' is defined but never used
// if an shutdown handler throws an error it goes into an infinite loop
debugger
}
})
}

async _isolate_deploy(isolate) {
await this.tx(async () => {
try {
const src = isolate.src
const { options = {} } = cds
const m = await cds.load(src, options).then(cds.minify)
// options.schema_evolution = 'auto'
await cds.deploy(m).to(this, options)
} catch (err) {
if (err.code === 'MODEL_NOT_FOUND' || err.code === 288) return
throw err
}
})
}

/**
* Dictionary of connection pools per tenant
*/
Expand Down Expand Up @@ -47,7 +224,7 @@
* transaction with `BEGIN`
* @returns this
*/
async begin (min) {
async begin(min) {
// We expect tx.begin() being called for an txed db service
const ctx = this.context

Expand Down Expand Up @@ -129,9 +306,9 @@
}

// REVISIT: should happen automatically after a configurable time
async disconnect (tenant) {
async disconnect(tenant) {
const tenants = tenant ? [tenant] : Object.keys(this.pools)
await Promise.all (tenants.map (async t => {
await Promise.all(tenants.map(async t => {
const pool = this.pools[t]; if (!pool) return
delete this.pools[t]
await pool.drain()
Expand Down
5 changes: 3 additions & 2 deletions hana/lib/HANAService.js
Original file line number Diff line number Diff line change
Expand Up @@ -1255,7 +1255,7 @@ SELECT ${mixing} FROM JSON_TABLE(SRC.JSON, '$' COLUMNS(${extraction})) AS NEW LE

// Creates a new database using HDI container groups
async database({ database }, clean = false) {
if (clean) {
if (this.options.credentials.__system__) {
// Reset back to system credentials
this.options.credentials = this.options.credentials.__system__
}
Expand Down Expand Up @@ -1297,11 +1297,12 @@ SELECT ${mixing} FROM JSON_TABLE(SRC.JSON, '$' COLUMNS(${extraction})) AS NEW LE
// This removes SCHEMA name conflicts when testing in the same system
// Additionally this allows for deploying using the HDI procedures
async tenant({ database, tenant }, clean = false) {
if (clean) {
if (this.options.credentials.__database__) {
// Reset back to database credentials
this.options.credentials = this.options.credentials.__database__
}

tenant = tenant.replaceAll('-','')
const creds = {
containerGroup: database.toUpperCase(),
usergroup: `${database}_USERS`.toUpperCase(),
Expand Down
24 changes: 14 additions & 10 deletions hana/lib/scripts/container-tenant.sql
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,24 @@ BEGIN SEQUENTIAL EXECUTION

NO_PARAMS = SELECT * FROM _SYS_DI.T_NO_PARAMETERS;

IGNOREPARAMS =
SELECT 'IGNORE_DEPLOYED' AS KEY, 'TRUE' AS VALUE FROM DUMMY
UNION ALL
SELECT 'IGNORE_WORK' AS KEY, 'TRUE' AS VALUE FROM DUMMY;
CALL _SYS_DI#{{{GROUP}}}.DROP_CONTAINER(:SCHEMANAME, :IGNOREPARAMS, :RETURN_CODE, :REQUEST_ID, :MESSAGES);
ALL_MESSAGES = SELECT * FROM :MESSAGES;

SELECT COUNT(*) INTO USER_EXISTS FROM SYS.USERS WHERE USER_NAME = :USERNAME;
IF :USER_EXISTS > 0 THEN
EXEC 'DROP USER ' || :USERNAME || ' CASCADE';
IF :CREATECONTAINER = FALSE THEN
IGNOREPARAMS =
SELECT 'IGNORE_DEPLOYED' AS KEY, 'TRUE' AS VALUE FROM DUMMY
UNION ALL
SELECT 'IGNORE_WORK' AS KEY, 'TRUE' AS VALUE FROM DUMMY;
CALL _SYS_DI#{{{GROUP}}}.DROP_CONTAINER(:SCHEMANAME, :IGNOREPARAMS, :RETURN_CODE, :REQUEST_ID, :MESSAGES);
ALL_MESSAGES = SELECT * FROM :MESSAGES;

IF :USER_EXISTS > 0 THEN
EXEC 'DROP USER ' || :USERNAME || ' CASCADE';
END IF;
END IF;

IF :CREATECONTAINER = TRUE THEN
EXEC 'CREATE USER ' || :USERNAME || ' PASSWORD ' || :USERPASS || ' NO FORCE_FIRST_PASSWORD_CHANGE SET USERGROUP "{{{GROUP}}}_USERS"';
IF :USER_EXISTS = 0 THEN
EXEC 'CREATE USER ' || :USERNAME || ' PASSWORD ' || :USERPASS || ' NO FORCE_FIRST_PASSWORD_CHANGE SET USERGROUP "{{{GROUP}}}_USERS"';
END IF;
EXEC 'GRANT EXECUTE ON SYS.GET_INSUFFICIENT_PRIVILEGE_ERROR_DETAILS TO ' || :USERNAME;

CALL _SYS_DI#{{{GROUP}}}.CREATE_CONTAINER(:SCHEMANAME, :NO_PARAMS, :RETURN_CODE, :REQUEST_ID, :MESSAGES);
Expand Down
1 change: 0 additions & 1 deletion hana/test/spatial.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ const cds = require('../../test/cds.js')

describe('Spatial Types', () => {
const { data, expect } = cds.test(__dirname + '/../../test/compliance/resources')
data.autoIsolation(true)
data.autoReset()

test('point', async () => {
Expand Down
19 changes: 11 additions & 8 deletions postgres/lib/PostgresService.js
Original file line number Diff line number Diff line change
Expand Up @@ -636,15 +636,16 @@ GROUP BY k

try {
if (!clean) {
await cds
await this.tx(tx => tx
.run(`CREATE USER "${creds.user}" IN GROUP "${creds.usergroup}" PASSWORD '${creds.password}'`)
.catch(e => {
if (e.code === '42710') return
if (e.code === '42710' || e.code === '23505') return
throw e
})
}))

// Retry granting priviledges as this is being done by multiple instances
// Postgres just rejects when other connections are granting the same user
const grant = (i = 0) => cds.run(`GRANT CREATE, CONNECT ON DATABASE "${creds.database}" TO "${creds.user}";`)
const grant = (i = 0) => this.tx(tx => tx.run(`GRANT CREATE, CONNECT ON DATABASE "${creds.database}" TO "${creds.user}";`))
.catch((err) => {
if (i > 100) throw err
return grant(i + 1)
Expand All @@ -657,10 +658,12 @@ GROUP BY k
this.options.credentials = Object.assign({}, this.options.credentials, creds)

// Create new schema using schema owner
await this.tx(async tx => {
await tx.run(`DROP SCHEMA IF EXISTS "${creds.schema}" CASCADE`)
if (!clean) await tx.run(`CREATE SCHEMA "${creds.schema}" AUTHORIZATION "${creds.user}"`)
})
if (clean) await this.tx(tx => tx.run(`DROP SCHEMA IF EXISTS "${creds.schema}" CASCADE`))
else await this.tx(tx => tx.run(`CREATE SCHEMA "${creds.schema}" AUTHORIZATION "${creds.user}"`)
.catch(err => {
if (err.code == '42P06') return
throw err
}))
} finally {
await this.disconnect()
}
Expand Down
1 change: 0 additions & 1 deletion postgres/test/odata-string-functions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ process.env.DEBUG && jest.setTimeout(100000)
describe('String + Collection functions', () => {
const { GET, expect, data } = cds.test('serve', '--project', project).verbose()

data.autoIsolation(true)
data.autoReset(true)

test('concat', async () => {
Expand Down
1 change: 0 additions & 1 deletion postgres/test/ql.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ process.env.DEBUG && jest.setTimeout(100000)
describe('QL to PostgreSQL', () => {
const { expect, data } = cds.test('serve', '--project', project).verbose()

data.autoIsolation(true)
data.autoReset(true)

describe('SELECT', () => {
Expand Down
1 change: 0 additions & 1 deletion postgres/test/service-admin.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ process.env.DEBUG && jest.setTimeout(100000)
describe('OData to Postgres dialect', () => {
const { GET, POST, DELETE, expect, data } = cds.test('serve', '--project', project).verbose()

data.autoIsolation(true)
data.autoReset(true)

test('OData: List of entities exposed by the admin service', async () => {
Expand Down
1 change: 0 additions & 1 deletion postgres/test/service-types.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ process.env.DEBUG && jest.setTimeout(100000)
describe('OData to Postgres dialect', () => {
const { GET, POST, expect, data } = cds.test('serve', '--project', project)

data.autoIsolation(true)
data.autoReset(true)

describe('OData types: CREATE', () => {
Expand Down
1 change: 0 additions & 1 deletion postgres/test/service.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ const guidRegEx = /\b[0-9a-f]{8}\b-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-\b[0-9a-f
describe('OData to Postgres dialect', () => {
const { GET, POST, PUT, PATCH, DELETE, expect, data } = cds.test('serve', '--project', project)

data.autoIsolation(true)
data.autoReset(true)

// making sure we're running the beershop
Expand Down
1 change: 0 additions & 1 deletion postgres/test/timezone.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ process.env.DEBUG && jest.setTimeout(100000)
describe('CAP PostgreSQL Adapter', () => {
const { GET, PUT, expect, data } = cds.test('serve', '--project', project).verbose()

data.autoIsolation(true)
data.autoReset(true)

describe('Timezone Handling', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/bookshop/db/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ module.exports = async tx => {
if (already_filled) return

await tx.run(
INSERT.into('sap.common.Currencies')
UPSERT.into('sap.common.Currencies')
.columns(['code', 'symbol', 'name'])
.rows(
['EUR', '€', 'Euro'],
Expand Down
Loading
Loading