Skip to content

Commit

Permalink
fix: funcs.test.js used wrong arguments for .where() (#943)
Browse files Browse the repository at this point in the history
As discussed in our call today... → required for renovated `cds.ql` +
new `cds.xl`
  • Loading branch information
danjoa authored Dec 9, 2024
1 parent 96911ad commit 33a0685
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 39 deletions.
11 changes: 6 additions & 5 deletions db-service/lib/common/DatabaseService.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,14 @@ class DatabaseService extends cds.Service {
* transaction with `BEGIN`
* @returns this
*/
async begin() {
async begin (min) {
// We expect tx.begin() being called for an txed db service
const ctx = this.context

// If .begin is called explicitly it starts a new transaction and executes begin
if (!ctx) return this.tx().begin()
if (!ctx) return this.tx().begin(min)

// REVISIT: can we revisit the below revisit now?
// REVISIT: tenant should be undefined if !this.isMultitenant
let isMultitenant = 'multiTenant' in this.options ? this.options.multiTenant : cds.env.requires.multitenancy
let tenant = isMultitenant && ctx.tenant
Expand All @@ -63,10 +64,10 @@ class DatabaseService extends cds.Service {

// Acquire a pooled connection
this.dbc = await this.acquire()
this.dbc.destroy = this.destroy.bind(this)
this.dbc.destroy = this.destroy.bind(this) // REVISIT: this is bad

// Begin a session...
try {
if (!min) try {
await this.set(new SessionContext(ctx))
await this.send('BEGIN')
} catch (e) {
Expand Down Expand Up @@ -154,7 +155,7 @@ class DatabaseService extends cds.Service {
run(query, data, ...etc) {
// Allow db.run('...',1,2,3,4)
if (data !== undefined && typeof query === 'string' && typeof data !== 'object') data = [data, ...etc]
return super.run(query, data)
return super.run(...arguments) //> important to call like that for tagged template literal args
}

/**
Expand Down
16 changes: 7 additions & 9 deletions db-service/lib/common/generic-pool.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
const { createPool } = require('generic-pool')

class ConnectionPool {
constructor(factory, tenant) {
let bound_factory = { __proto__: factory, create: factory.create.bind(null, tenant) }
return _track_connections4(createPool(bound_factory, factory.options))
}
function ConnectionPool (factory, tenant) {
let bound_factory = { __proto__: factory, create: factory.create.bind(null, tenant) }
return createPool(bound_factory, factory.options)
}

// REVISIT: Is that really neccessary ?!
function _track_connections4(pool) {
function TrackedConnectionPool (factory, tenant) {
const pool = new ConnectionPool (factory, tenant)
const { acquire, release } = pool
return Object.assign(pool, {
async acquire() {
Expand All @@ -23,12 +21,12 @@ function _track_connections4(pool) {
throw err
}
},

release(dbc) {
this._trackedConnections?.delete(dbc._beginStack)
return release.call(this, dbc)
},
})
}

module.exports = ConnectionPool
const DEBUG = /\bpool\b/.test(process.env.DEBUG)
module.exports = DEBUG ? TrackedConnectionPool : ConnectionPool
9 changes: 0 additions & 9 deletions db-service/lib/infer/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const cds = require('@sap/cds')
const JoinTree = require('./join-tree')
const { pseudos } = require('./pseudos')
const { isCalculatedOnRead } = require('../utils')
const { t } = require('@sap/cds/lib/utils/tar')
const cdsTypes = cds.linked({
definitions: {
Timestamp: { type: 'cds.Timestamp' },
Expand Down Expand Up @@ -1061,14 +1060,6 @@ function infer(originalQuery, model) {
return Object.setPrototypeOf(result, base)
}

// REVISIT: functions without return are by nature side-effect functions -> bad
function init$refLinks(arg) {
Object.defineProperty(arg, '$refLinks', {
value: [],
writable: true,
})
}

function getCdsTypeForVal(val) {
// REVISIT: JS null should have a type for proper DB layer conversion logic
// if(val === null) return {type:'cds.String'}
Expand Down
33 changes: 19 additions & 14 deletions sqlite/lib/SQLiteService.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,38 +12,34 @@ const sqliteKeywords = keywords.reduce((prev, curr) => {
return prev
}, {})

// define date and time functions in js to allow for throwing errors
const isTime = /^\d{1,2}:\d{1,2}:\d{1,2}$/
const hasTimezone = /([+-]\d{1,2}:?\d{0,2}|Z)$/
const toDate = (d, allowTime = false) => {
const date = new Date(allowTime && isTime.test(d) ? `1970-01-01T${d}Z` : hasTimezone.test(d) ? d : d + 'Z')
if (Number.isNaN(date.getTime())) throw new Error(`Value does not contain a valid ${allowTime ? 'time' : 'date'} "${d}"`)
return date
}


class SQLiteService extends SQLService {
init() {
return super.init(...arguments)
}

get factory() {
return {
options: { max: 1, ...this.options.pool },
create: tenant => {
const database = this.url4(tenant)
const dbc = new sqlite(database)

const deterministic = { deterministic: true }
dbc.function('session_context', key => dbc[$session][key])
dbc.function('regexp', deterministic, (re, x) => (RegExp(re).test(x) ? 1 : 0))
dbc.function('ISO', deterministic, d => d && new Date(d).toISOString())

// define date and time functions in js to allow for throwing errors
const isTime = /^\d{1,2}:\d{1,2}:\d{1,2}$/
const hasTimezone = /([+-]\d{1,2}:?\d{0,2}|Z)$/
const toDate = (d, allowTime = false) => {
const date = new Date(allowTime && isTime.test(d) ? `1970-01-01T${d}Z` : hasTimezone.test(d) ? d : d + 'Z')
if (Number.isNaN(date.getTime())) throw new Error(`Value does not contain a valid ${allowTime ? 'time' : 'date'} "${d}"`)
return date
}
dbc.function('year', deterministic, d => d === null ? null : toDate(d).getUTCFullYear())
dbc.function('month', deterministic, d => d === null ? null : toDate(d).getUTCMonth() + 1)
dbc.function('day', deterministic, d => d === null ? null : toDate(d).getUTCDate())
dbc.function('hour', deterministic, d => d === null ? null : toDate(d, true).getUTCHours())
dbc.function('minute', deterministic, d => d === null ? null : toDate(d, true).getUTCMinutes())
dbc.function('second', deterministic, d => d === null ? null : toDate(d, true).getUTCSeconds())

if (!dbc.memory) dbc.pragma('journal_mode = WAL')
return dbc
},
Expand Down Expand Up @@ -118,6 +114,15 @@ class SQLiteService extends SQLService {
yield ']'
}

pragma (pragma, options) {
if (!this.dbc) return this.begin('pragma') .then (tx => {
try { return tx.pragma (pragma, options) }
finally { tx.release() }
})
return this.dbc.pragma (pragma, options)
}


exec(sql) {
return this.dbc.exec(sql)
}
Expand Down
4 changes: 2 additions & 2 deletions test/scenarios/bookshop/funcs.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ describe('Bookshop - Functions', () => {

test('avg', async () => {
const { Books } = cds.entities
const res = await cds.run(CQL`SELECT from ${Books} {
const res = await cds.run(CQL`SELECT from ${Books} {
average(stock) as avgStock
}`)
expect(res[0].avgStock).to.not.be.undefined
Expand Down Expand Up @@ -228,7 +228,7 @@ describe('Bookshop - Functions', () => {
const result = data.type ? func.extract(data) : data.value
const cqn = SELECT.one(`${func.func}('${data.value}') as result`)
.from('sap.capire.bookshop.Books')
.where([`${func.func}('${data.value}') = `], result)
.where(`${func.func}('${data.value}') = `, result)

if (data.type & func.type) {
const res = await cqn
Expand Down

0 comments on commit 33a0685

Please sign in to comment.