Skip to content

Automatic transactions #75

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

Closed
mcollina opened this issue Feb 5, 2021 · 6 comments · Fixed by #76
Closed

Automatic transactions #75

mcollina opened this issue Feb 5, 2021 · 6 comments · Fixed by #76

Comments

@mcollina
Copy link
Member

mcollina commented Feb 5, 2021

🚀 Feature Proposal

In most applications I've seen:

fastify.get('/something', function (req, reply) {
  return this.pg.transact(async (client) => {
      const { rows } = await client.query('SELECT id, username, hash, salt FROM users WHERE id=$1', [req.params.id])
      return rows
  })
})

I think we could greatly simplify this by having some dedicated syntax to avoid developer errors

3rd argument

fastify.get('/something', { pg: { transact: true } }, function (req, reply, client) {
  const { rows } = await client.query('SELECT id, username, hash, salt FROM users WHERE id=$1', [req.params.id])
  return rows
})

or even better

fastify.register(require('fastify-postgres'), {
  connectionString: 'postgres://postgres@localhost/postgres',
  wrapRoutesWithTransaction: true
})

fastify.get('/something', function (req, reply, client) {
  const { rows } = await client.query('SELECT id, username, hash, salt FROM users WHERE id=$1', [req.params.id])
  return rows
})

This can be implemented by having an 'onRoute' hook that wraps the handler.

Decorator

fastify.get('/something', { pg: { transact: true } }, function (req, reply) {
  const { rows } = await req.pg.query('SELECT id, username, hash, salt FROM users WHERE id=$1', [req.params.id])
  return rows
})

or even better

fastify.register(require('fastify-postgres'), {
  connectionString: 'postgres://postgres@localhost/postgres',
  wrapRoutesWithTransaction: true
})

fastify.get('/something', function (req, reply) {
  const { rows } = await req.pg.query('SELECT id, username, hash, salt FROM users WHERE id=$1', [req.params.id])
  return rows
})

This can be implemented by having a 'preHandler' hook that starts the transaction and an 'onSend' hook that commits or roll back it.


What do you prefer? The decorator is probably the more idiomatic approach, even it it will require a bit more work to implement.

@StarpTech
Copy link
Member

This will mix "user code" with transaction logic. In case of a retry, the whole handle must be retried.

@mcollina
Copy link
Member Author

mcollina commented Feb 5, 2021

In case of a retry, the whole handle must be retried.

Would you implement retries? This logic is built for the vast majority of cases where develoeprs do not implement retries for the failed transactions. Of course, there should opt-out of this if they need to do retries.

@voxpelli
Copy link
Contributor

voxpelli commented Feb 5, 2021

Automatic transactions: I don’t like that idea

Decorating request with pg and ensuring that transactions created through that doesn’t live past the request in itself: Sounds like a good one. I think the opt-in here is key, I think transactions should always be an active choice.

Decorating request with pg is generally something I would like, even independently of this specific use case.

@mcollina
Copy link
Member Author

mcollina commented Feb 5, 2021

Decorating request with pg and ensuring that transactions created through that doesn’t live past the request in itself: Sounds like a good one. I think the opt-in here is key, I think transactions should always be an active choice.

@voxpelli would you be ok with something like:

fastify.get('/something', { pg: { transact: true } }, function (req, reply) {
  const { rows } = await req.pg.query('SELECT id, username, hash, salt FROM users WHERE id=$1', [req.params.id])
  return rows
})

or possibly

fastify.get('/something', function (req, reply) {
  const { rows } = await req.pg.transaction('SELECT id, username, hash, salt FROM users WHERE id=$1', [req.params.id])
  return rows
})

Where req.pg.transaction is started on first use and automatically committed during 'onSend'?

@voxpelli
Copy link
Contributor

voxpelli commented Feb 5, 2021

@mcollina I would be okay with both of them as solutions to this concepts, the two thoughts on those specifically though are:

  • For the first example: How would { pg: { transact: true } } behave when there are multiple connections? It must be that it refers to the unnamed main connection and if one has a named one which should have a transaction, then one has to use { pg: { nameOfAnotherConnection: { transact: true } } }?
  • For the second example: I would prefer eg. an await req.pg.transact(true); and then use regular await req.pg.query() for the actual queries

Some more thoughts on the general concept:

A design like this probably only work when no error handling is made within the router, right? The async route handler basically needs to get rejected for it to work? An error sent to reply.send() perhaps would also make the transaction be rejected?

So a failing UPDATE articles like here won't reject the transaction and thus leave the UPDATE users updated:

fastify.post('/something', { pg: { transact: true } }, async (req, reply) => {
  try {
    await req.pg.query('UPDATE users SET username=$1 WHERE id=$2', [req.params.username, req.params.id]);
    await req.pg.query('UPDATE articles SET username=$1 WHERE userid=$2', [req.params.username, req.params.id])
  } catch () {
    return reply.send(
      // A nicely rendered error page with a user facing error message and the user submitted data losslessly preserved
    );
  }

  return reply.redirect('/admin/users');
});

It needs to be written like:

fastify.post('/something', async (req, reply) => {
  try {
    await req.pg.transact(async (client) => {
      await req.pg.query('UPDATE users SET username=$1 WHERE id=$2', [req.params.username, req.params.id]);
      await req.pg.query('UPDATE articles SET username=$1 WHERE userid=$2', [req.params.username, req.params.id])
    });
  } catch () {
    return reply.send(
      // A nicely rendered error page with a user facing error message and the user submitted data losslessly preserved
    );
  }

  return reply.redirect('/admin/users');
});

Only if one does no in-route error handling does it work, and that's only really feasible in API:s, and even then it would be possible for a refactoring to later add in some error handling without realizing that they are breaking the transactionality of it 🤔

fastify.post('/something', { pg: { transact: true } }, async (req, reply) => {
  await req.pg.query('UPDATE users SET username=$1 WHERE id=$2', [req.params.username, req.params.id]);
  await req.pg.query('UPDATE articles SET username=$1 WHERE userid=$2', [req.params.username, req.params.id])

  return reply.redirect('/admin/users');
});

@mcollina
Copy link
Member Author

mcollina commented Feb 5, 2021

  • For the first example: How would { pg: { transact: true } } behave when there are multiple connections? It must be that it refers to the unnamed main connection and if one has a named one which should have a transaction, then one has to use { pg: { nameOfAnotherConnection: { transact: true } } }?

Yes exactly. I think this is preferred approach.

toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 9, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 9, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 9, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 10, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 11, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 11, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 16, 2021
* Uses hooks to handle transactions outside the route handler code
* preHandler does BEGIN
* onSend does COMMIT
* onError does ROLLBACK
* useTransaction routeOption gives the developer opt-in for this feature - they have to explicitly ask
* Tests cover the four possibilities, a passing and failing set of queries, called in both true and false states for useTransaction

resolves fastify#75
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 16, 2021
* Uses hooks to handle transactions outside the route handler code
* preHandler does BEGIN
* onSend does COMMIT
* onError does ROLLBACK
* useTransaction routeOption gives the developer opt-in for this feature - they have to explicitly ask
* Tests cover the four possibilities, a passing and failing set of queries, called in both true and false states for useTransaction

For DX:

* Adds `--fix` to the linting to help automate indenting etc.
* Adds a `testonly` script which doesn't drop out for linting (helpful when iterating quickly over tests).

resolves fastify#75
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 17, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 17, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 17, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 18, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 18, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 18, 2021
As with the base fastify instance, the pg decoration will use namespaces. For each namespaced called to the main function, the namespace will be added to the req.pg object too, with similar checks
The option to use a chosen namespace with the routeOptions is provided and we make sure that this namespace is used to extract the correct client from req.pg
A test also covers this for queries wrapped in a route transaction

Resolves fastify#75
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 18, 2021
As with the base fastify instance, the pg decoration will use namespaces. For each namespaced called to the main function, the namespace will be added to the req.pg object too, with similar checks
The option to use a chosen namespace with the routeOptions is provided and we make sure that this namespace is used to extract the correct client from req.pg
A test also covers this for queries wrapped in a route transaction

Resolves fastify#75
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 19, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 19, 2021
We needed to avoid the unnecessary addition of handlers in the case where a name and transact value mismatched (different names, or true and a name, etc.)

Resolves fastify#75
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 19, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 19, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 22, 2021
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 22, 2021
Adds in a README section covering the transact option

Resolves fastify#75
toni-sharpe pushed a commit to toni-sharpe/fastify-postgres that referenced this issue Feb 23, 2021
Resolves fastify#75

Co-authored-by: James Sumners <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants