Transaction scope behavior seems to differ from the docs

[dbettini]

I am currently implementing a feature that requires a pessimistic for update lock, which requires a running transaction. I can't use find since only findOne seems to have that option in v4, so I went with a query builder approach.
Now, my understanding from the documentation was that I need to use the em instance provided in the callback to actually make queries as part of that transaction, i.e. I can't use my dependency injected repositories that were created from some root em. However, I realized that flushes done in the transacting callback, regardless of which em is used, are rolled back even if an error occurs after the flush.

// provider.ts - repositories will be injected on this
firstRepo = rootEm.getRepository(First);
secondRepo = rootEm.getRepository(Second);

// service.ts
function getData(id: number, trx?: Transaction) {
	return RequestContext.getEntityManager().transactional(async (em: SqlEntityManager) => {
		const qb = em.createQueryBuilder(First).setLockMode(LockMode.PESSIMISTIC_WRITE);
		return qb.select('*').where({ id }).getResult();
	}, trx);
}

function updateSecond(data) {
	// Compute something from data...
	return this.secondRepo.persistAndFlush(new Second(newData));
}

async function update(id) {
	await RequestContext.getEntityManager().transactional(async em => {
		const data = await getData(id, em.getTransactionContext()); // Using the provided transaction em

		const firstRepo = em.getRepository(First); // Using the provided transaction em
		// Compute something from data...
		await firstRepo.persist(new First(newData));

		await updateSecond(data); // Not using the provided transaction em, so this should not be in the same transaction
		
		await firstRepo.flush(); // This seems to be optional, but I do it just in case
		throw new Error(); // Throw to see what is rolled back
	});
}

When I run the above code, no db updates are done. All queries are ran, but are rolled back. Only one transaction has been started and rolled back, according to the logs.

Expected behavior
According to the docs, I would expect that the first repo updates are rolled back, since it's a part of the transaction. Second repo updates, on the other hand, should be applied, since I used the repository that was created from the root em and should therefore use a completely separate transaction.

Versions
v4.5
Postgres 14

Questions

1. From the current state of things, it seems that starting a transaction will result in all queries being ran as a part of it (even if not passing the transaction context or em). Only if I explicitly run updateSecond inside a new transacting callback will it be ran in its own transaction. Does this mean that passing the transaction is not needed in the above getData method in the first place, i.e. every call ran in the callback will use it, regardless of the em instance used? I'm guessing this is the result of Domain API magic that I'm not well versed in.
2. If this is the expected behavior, I feel like nested transactions/savepoints could use an example in the docs, which would show exactly how it should be done, since at the moment it does seem that the docs indicate you should use the same em to use the transaction, which is not the case. I'm guessing that the reason em is passed is because it gives you a clean identity map at the start of the transaction?

[B4nan]

I can't use find since only findOne seems to have that option in v4, so I went with a query builder approach.

I'd say upgrade to v5, no reason to be worried at this stage as its pretty much done, RC is out :]

Now, my understanding from the documentation was that I need to use the em instance provided in the callback to actually make queries as part of that transaction,

That is no longer true, the whole callback is executed in a new transaction scope, so you can use the global EM too and it will pick the right tx context automatically. So you can use the repositories from DI.

Note that there is no such thing as "flushing a repository". You always flush the underlying EM, and you always flush the whole unit, not just some entity types.

Looking at your example, I dont understand why you expect anything to be persisted. You run things inside transactional block, and at the end you throw. This should never save anything. Nested transactions will be rolled back if the parent tx is.

[dbettini]

I'd say upgrade to v5, no reason to be worried at this stage as its pretty much done, RC is out :]

That is definitely the plan in the near future :)

That is no longer true, the whole callback is executed in a new transaction scope, so you can use the global EM too and it will pick the right tx context automatically. So you can use the repositories from DI.

It might be useful to mention this in the documentation, since it currently isn't clear and seems to imply that the provided em should be used for the transaction to be applied.

Looking at your example, I dont understand why you expect anything to be persisted. You run things inside transactional block, and at the end you throw. This should never save anything. Nested transactions will be rolled back if the parent tx is.

Because I thought it is required to explicitly pass some kind of a transaction context (either the provided em or em.getTransactionContext()), like I did in the getData function, for something to be executed as part of that transaction. The reason I thought that is because other ORMs I've used in JS (Sequelize, Objection and Bookshelf) all required explicit transaction context passing from what I remember. However, from your response it seems that all of this is redundant, i.e. I can use my injected repos and I also don't have to pass trx to getData. I'm guessing Domains (and in the new version async hooks) allow for context tracking that allows this to happen. This definitely answers my questions, so thank you!

[dbettini]

@B4nan if I remove explicit trx passing to getData, i.e:

function getData(id: number) {
	const qb = (RequestContext.getEntityManager() as SqlEntityManager)
		.createQueryBuilder(First).setLockMode(LockMode.PESSIMISTIC_WRITE);
	return qb.select('*').where({ id }).getResult();
}

I get An open transaction is required for this operation Error, even though the function is called inside of a transaction block. Would this be a bug? You mentioned above that all things ran inside a transaction block should use that transaction (and they do when using repositories), but it doesn't seem to be the case when trying to create a query builder. I tried using RequestContext.getEntityManager() and injected rootEm, both ends in the same error.

[B4nan]

Right, that might be a bug, you could get around that via explicit usage of em.getContext():

function getData(id: number) {
	const qb = (RequestContext.getEntityManager() as SqlEntityManager)
		.getContext()
		.createQueryBuilder(First).setLockMode(LockMode.PESSIMISTIC_WRITE);
	return qb.select('*').where({ id }).getResult();
}

Will fix this in a minute.

[dbettini]

Yes, that works, thanks!
I just needed to cast the result of getContext, i.e.

(RequestContext.getEntityManager().getContext() as SqlEntityManager)		
		.createQueryBuilder(First).setLockMode(LockMode.PESSIMISTIC_WRITE);

Just to let you know, I initially tried await em.lock(First, LockMode.PESSIMISTIC_WRITE) approach, but that throws Cannot read property '__primaryKeys' of undefined, that's why I went with the query builder in the first place.

[B4nan]

await em.lock(First, LockMode.PESSIMISTIC_WRITE)

it needs to be entity instance, not entity class. you can only lock a single entity this way, not the whole table

[dbettini]

I see, that's a mistake on my part. Thanks for the clarification!