Skip to content

Commit

Permalink
chore: handle transactions already started at the controller layer (#…
Browse files Browse the repository at this point in the history
…4953)

## About the changes
This PR adds a method to safeguard us from opening a new transaction
while inside another transaction, resulting in two isolated transactions
that will not be atomic (if one fails, the other might still complete
successfully).


https://github.com/knex/knex/blob/bbbe4d4637b3838e4a297a457460cd2c76a700d5/lib/knex-builder/make-knex.js#L143C5-L144C88

We're currently opening transactions at the controller layer 

https://github.com/Unleash/unleash/blob/2746bd151766f8afbbaa2f640e8ebee6f4f98086/src/lib/features/export-import-toggles/export-import-controller.ts#L206-L208

but in some other places, we do it at the store level:

https://github.com/Unleash/unleash/blob/2746bd151766f8afbbaa2f640e8ebee6f4f98086/src/lib/db/access-store.ts#L577


## Alternative
We can remove store-level transactions and move them to the controller
following this approach:


https://github.com/Unleash/unleash/blob/cb034976b93abc799df774858d716a49f645d669/src/lib/services/index.ts#L282-L284


https://github.com/Unleash/unleash/blob/cb034976b93abc799df774858d716a49f645d669/src/lib/features/export-import-toggles/export-import-controller.ts#L206-L208

This option is more expensive because we have to:
1. Write the factory methods that propagate the transaction to the
stores (therefore creating the store factory methods as well)
2. Identify the methods for creating the transactions at the store level
and backtrack the calls until the controller layer
  • Loading branch information
gastonfournier authored Oct 6, 2023
1 parent 2746bd1 commit 52fa872
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 6 deletions.
9 changes: 5 additions & 4 deletions src/lib/db/access-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
NamePermissionRef,
PermissionRef,
} from 'lib/services/access-service';
import { inTransaction } from './transaction';

const T = {
ROLE_USER: 'role_user',
Expand Down Expand Up @@ -574,7 +575,7 @@ export class AccessStore implements IAccessStore {
};
});

await this.db.transaction(async (tx) => {
await inTransaction(this.db, async (tx) => {
if (userRows.length > 0) {
await tx(T.ROLE_USER)
.insert(userRows)
Expand Down Expand Up @@ -620,7 +621,7 @@ export class AccessStore implements IAccessStore {
})),
);

await this.db.transaction(async (tx) => {
await inTransaction(this.db, async (tx) => {
if (groupRows.length > 0) {
await tx(T.GROUP_ROLE)
.insert(groupRows)
Expand Down Expand Up @@ -656,7 +657,7 @@ export class AccessStore implements IAccessStore {
role_id: role,
}));

await this.db.transaction(async (tx) => {
await inTransaction(this.db, async (tx) => {
await tx(T.ROLE_USER)
.where('project', projectId)
.andWhere('user_id', userId)
Expand Down Expand Up @@ -707,7 +708,7 @@ export class AccessStore implements IAccessStore {
created_by: createdBy,
}));

await this.db.transaction(async (tx) => {
await inTransaction(this.db, async (tx) => {
await tx(T.GROUP_ROLE)
.where('project', projectId)
.andWhere('group_id', groupId)
Expand Down
3 changes: 2 additions & 1 deletion src/lib/db/api-token-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
} from '../types/models/api-token';
import { ALL_PROJECTS } from '../util/constants';
import { Db } from './db';
import { inTransaction } from './transaction';

const TABLE = 'api_tokens';
const API_LINK_TABLE = 'api_token_project';
Expand Down Expand Up @@ -139,7 +140,7 @@ export class ApiTokenStore implements IApiTokenStore {
}

async insert(newToken: IApiTokenCreate): Promise<IApiToken> {
const response = await this.db.transaction(async (tx) => {
const response = await inTransaction(this.db, async (tx) => {
const [row] = await tx<ITokenInsert>(TABLE).insert(
toRow(newToken),
['created_at'],
Expand Down
26 changes: 26 additions & 0 deletions src/lib/db/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,39 @@ export type WithTransactional<S> = S & {
transactional: <R>(fn: (service: S) => R) => Promise<R>;
};

/**
* @deprecated this is a temporal solution to deal with transactions at the store level.
* Ideally, we should handle transactions at the service level (each service method should be transactional).
* The controller should define the transactional scope as follows:
* https://github.com/Unleash/unleash/blob/cb034976b93abc799df774858d716a49f645d669/src/lib/features/export-import-toggles/export-import-controller.ts#L206-L208
*
* To be able to use .transactional method, services should be instantiated like this:
* https://github.com/Unleash/unleash/blob/cb034976b93abc799df774858d716a49f645d669/src/lib/services/index.ts#L282-L284
*
* This function makes sure that `fn` is executed in a transaction.
* If the db is already in a transaction, it will execute `fn` in that transactional scope.
*
* https://github.com/knex/knex/blob/bbbe4d4637b3838e4a297a457460cd2c76a700d5/lib/knex-builder/make-knex.js#L143C5-L144C88
*/
export async function inTransaction<R>(
db: Knex,
fn: (db: Knex) => R,
): Promise<R> {
if (db.isTransaction) {
return fn(db);
}
return db.transaction(async (tx) => fn(tx));
}

export function withTransactional<S>(
serviceFactory: (db: Knex) => S,
db: Knex,
): WithTransactional<S> {
const service = serviceFactory(db) as WithTransactional<S>;

service.transactional = async <R>(fn: (service: S) => R) =>
// Maybe: inTransaction(db, async (trx: Knex.Transaction) => fn(serviceFactory(trx)));
// this assumes that the caller didn't start a transaction already and opens a new one.
db.transaction(async (trx: Knex.Transaction) => {
const transactionalService = serviceFactory(trx);
return fn(transactionalService);
Expand Down
2 changes: 1 addition & 1 deletion src/lib/types/option.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export interface IUnleashOptions {
versionCheck?: Partial<IVersionOption>;
telemetry?: boolean;
authentication?: Partial<IAuthOption>;
ui?: object;
ui?: IUIConfig;
frontendApi?: IFrontendApi;
import?: Partial<IImportOption>;
experimental?: Partial<IExperimentalOptions>;
Expand Down

0 comments on commit 52fa872

Please sign in to comment.