driver-adapters: remove dispose method for JS transaction interface #4489
Conversation
aqrln
requested review from
miguelff and
Druue
and removed request for
a team
aqrln
force-pushed
the
driver-adapters-remove-dispose-method-for-js-transaction-interface
branch
from
222f720 to
47179e6
Compare
CodSpeed Performance ReportMerging #4489 will not alter performanceComparing Summary
|
...engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs
Show resolved
Hide resolved
| @@ -0,0 +1,24 @@ | |||
| use query_engine_tests::*; | |||
|
|
|||
| #[test_suite(schema(generic), only(JS, Sqlite))] | |||
There was a problem hiding this comment.
JS-only because after adding the test turned out that it's actually broken with native SQLite connector, this scenario has never been tested before (except indirectly via smoke tests for driver adapters which uncovered the issue back then):
[2023-11-22T14:21:52Z] called `Result::unwrap()` on an `Err` value: QueryCoreError(ConnectorError(ConnectorError { user_facing_error: None, kind: QueryError(SqliteError { extended_code: 1, message: Some("cannot start a transaction within a transaction") }), transient: false }))
I'll open an issue with TypeScript reproduction, and we can fix it separately.
There was a problem hiding this comment.
The fix might be to move the logic added in this PR somewhere to the level above, let me know if you'd prefer it to be done in this PR rather than separately.
There was a problem hiding this comment.
I think that should maybe be blanket Drop for Transaction from quaint? If that's easy enough to do, then I'd prefer to do it here and fix the issue for all driver, otherwise follow-up should be fine too.
There was a problem hiding this comment.
I think that should maybe be blanket Drop for Transaction from quaint?
Unfortunately this violates orphan rules, and if it didn't, the fact that we also need some state and not just the Drop impl could make this a little awkward. I think it would be better to wrap the quaint transaction into some struct that will take care of this. Let's do that as a follow-up so we can land this PR already.
This method will not be used anymore after prisma/prisma-engines#4489 as this logic is de-duplicated and moved to the Rust side.
Remove the boilerplate `Transaction.dispose` method from the public driver adapter interface and move the corresponding functionality directly to the destructor of `TransactionProxy`. Refs: #4286 Closes: prisma/team-orm#391
aqrln
force-pushed
the
driver-adapters-remove-dispose-method-for-js-transaction-interface
branch
from
39eaaa8 to
5ab9f4f
Compare
...engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Miguel Fernández <[email protected]>
There's no need in release-store in commit/rollback and acquire-load in destructor anymore since we don't care for commit to have finished in the destructor anymore.
aqrln
deleted the
driver-adapters-remove-dispose-method-for-js-transaction-interface
branch
This method will not be used anymore after prisma/prisma-engines#4489 as this logic is de-duplicated and moved to the Rust side.
…22069) This method will not be used anymore after prisma/prisma-engines#4489 as this logic is de-duplicated and moved to the Rust side.
Remove the boilerplate
Transaction.disposemethod from the publicdriver adapter interface and move the corresponding functionality
directly to the destructor of
TransactionProxy.Refs: #4286
Closes: https://github.com/prisma/team-orm/issues/391