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

Add support for testing transaction commands #34

Closed
Fryuni opened this issue Nov 19, 2022 · 8 comments · Fixed by #35
Closed

Add support for testing transaction commands #34

Fryuni opened this issue Nov 19, 2022 · 8 comments · Fixed by #35

Comments

@Fryuni
Copy link
Contributor

Fryuni commented Nov 19, 2022

When implementing utilities to enter and exit transactions using knex, there is a need to test the ordering of transactional commands and how errors are handled.

The use case that got me here was implementing a contextual scope for cross repository transaction.

It is used like this:

new TransactionalScope(
  new SomeHander(
    new FooRepository(),
    new BarRepository(),
  ),
),

SomeHandler doesn't know anything about knex, and the repository interfaces only contain domain types. There is no API difference between an in-memory repository, a prisma repository or a knex repository.

But the entire operation of SomeHandler should be done in a single transaction, if BarRepository fails to write, the changes to the FooRepository must not be persisted.

This is relatively simple to implement, but testing it requires checking if the correct BEGIN, SAVEPOINT xxx, ROLBACK TO SAVEPOINT and ROLLBACK commands are sent and in the correct order.

Would you be willing to accept a PR to allow those commands to be checked on the tracker.on.any matcher, falling back to the current behavior of returning undefined when there is no match?

I'd add them to the history if they ware matched, and keep the current behavior of dropping them otherwise, this way I don't think there would be any breaking change for existing users.

@felixmosh
Copy link
Owner

Hi, can you share a knex queries so I'll be able to understand the issue properly.

@Fryuni
Copy link
Contributor Author

Fryuni commented Nov 19, 2022

The query itself is just the transaction command, which is already handled here:

if (
typeof rawQuery.method === 'undefined' &&
transactionCommands.some((trxCommand) => rawQuery.sql.startsWith(trxCommand))
) {
return resolve(undefined);
}

The problem is that I need to test that those command are being issued when they should, for example, testing that a callback is called strictly after the BEGIN command is sent and that the COMMIT command is sent strictly after the callback returns.

Since currently they are not being recorded in the history and also don't check any of the matchers, there is no way to test this.

@felixmosh
Copy link
Owner

So, basically you would like to check which queries were executed in transaction, and if it was rollbacked or no?

@Fryuni
Copy link
Contributor Author

Fryuni commented Nov 20, 2022

Yes, exactly.

@Fryuni
Copy link
Contributor Author

Fryuni commented Nov 20, 2022

Another idea would be to generate a transaction ID and add it to the RawQuery type, to indicate on which transaction it was executed, this would allow catching bugs such as:

knex.transaction(async tx => {
  await tx.insert(...);
  
  // This query happens in the correct moment, after the transaction starts and before the transaction ends,
  // but it happens outside of the transaction. There is no way to test this as well.
  await knex.select(...);
});

I'll try to think of a way to catch these bugs without requiring a breaking change on RawQuery.

@felixmosh
Copy link
Owner

Another idea would be to generate a transaction ID and add it to the RawQuery type, to indicate on which transaction it was executed, this would allow catching bugs such as:

knex.transaction(async tx => {
  await tx.insert(...);
  
  // This query happens in the correct moment, after the transaction starts and before the transaction ends,
  // but it happens outside of the transaction. There is no way to test this as well.
  await knex.select(...);
});

I'll try to think of a way to catch these bugs without requiring a breaking change on RawQuery.

This is exactly what I've thought 🙂

@Fryuni
Copy link
Contributor Author

Fryuni commented Nov 20, 2022

I'll draft a PR for these as a proposal

@Fryuni
Copy link
Contributor Author

Fryuni commented Nov 21, 2022

I opened #35 with one solution for this that doesn't break any of the existing public APIs.

There are some changes to the internal APIs though.

Let me know what you think about it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants