fix: make ReplaceAllProperties atomic

[kazuhidelee]

Summary

ReplaceAllProperties runs a delete followed by a series of upserts. Before the change, if no transaction was provided with opts, getStoreOrTransaction returned the plain store and Postgres ran each statement in autocommit mode. so a failed upsert in middle would leave the entity with no properties at all.
The fix wraps the delete/insert sequence in a transaction when the caller hasn't already provided one, and reuses the existing querier when they have.

To keep the code organized and avoid duplication, I used helper functions for this implementation:

• replaceAllPropertiesWithQuerier() for the delete-then-reinsert sequence
• saveAllPropertiesWithQuerier() for the shared property upsert loop

Fixes #4423

Testing

New tests cover:

• transaction is opened when called with plain store
• existing transaction querier is reused
• transaction wrapper errors surface correctly
• original properties are preserved when a mid-reinsertion failure triggers rollback.

[coveralls]

coverage: 59.412% (+0.02%) from 59.39% — kazuhidelee:issue-4423-replace-all-properties-transaction into mindersec:main

[evankanderson]

Do you want to leave SaveAllProperties exposed without a TX guard?

[kazuhidelee]

Thanks for the feedback. I updated the SaveAllProperties to use the same transaction guard pattern when it runs against the plain store. And also added unit tests that tests SaveAllProperties as well.

[evankanderson]

Sorry for the extra feedback; re-reading this, I noticed the duplicated calls to the helper method, and it seemed simpler to not need a helper at all and just update the querier and options.

[evankanderson]

Since this is calling the same function (replaceAllPropertiesWithQuerier), it feels like it might be more natural to update qtx and continue, rather than early-returning:

Suggested change

	return store.WithTransactionErr(func(qtx db.ExtendQuerier) error {
	return ps.replaceAllPropertiesWithQuerier(ctx, entityID, props, qtx)
	if store, ok := qtx.(db.Store); ok {
	tx, err := store.BeginTransaction()
	if err != nil {
	return err
	}
	qtx = store.GetQuerierWithTransation(tx)
	opts = append(opts, CallBuilder().WithStoreOrTransaction(tx))
	}
	err := qtx.DeleteAllPropertiesForEntity(ctx, entityID)
	if err != nil {
	fmt.Errorf("failed to delete properties: %w", err)
	}
	return ps.SaveAllProperties(ctx, entityID, props, opts)

(and then not needing to add the additional functions)

[kazuhidelee]

Thanks, I agree. I updated both ReplaceAllProperties and SaveAllProperties to upgrade qtx to a transaction-backed querier when starting from the plain store then continue through a single code path.

[evankanderson]

Really sorry for missing the extra transaction management handled by WithTransactionErr. Seeing what I requested, I sort of facepalmed at myself.

Let's merge d674132 from before yesterday's comment.