Skip to content
This repository has been archived by the owner on Oct 20, 2022. It is now read-only.

Add a new EntityActions that remove all "transactionally" code #78

Closed
djx314 opened this issue Mar 17, 2016 · 4 comments
Closed

Add a new EntityActions that remove all "transactionally" code #78

djx314 opened this issue Mar 17, 2016 · 4 comments

Comments

@djx314
Copy link
Contributor

djx314 commented Mar 17, 2016

as the issue here slick/slick#1274
use transactionally can bring deadlocks, any way, transactionally code is slowly than no transactionally code.
So I suggest that can active-slick provide a new EntityActions trait that remove all the transactionally code to give us more chice.

@djx314 djx314 changed the title Add Add a new EntityActions that remove all "transactionally" code Mar 17, 2016
@octonato
Copy link
Contributor

You have a good point.

Currently all functionally in EntityActions that need .transactionally will be broken if we remove it.

Functionally that will be totally broken are:

  • beforeInsert and beforeUpdate
    We'll need to remove the beforeInsert and make sure user get an error if they override it (it will have to be a runtime error)
  • fetchAll have to be removed
    We can't make a StreamDBIO without it

There is already a fix for slick/slick#1274, PR is here: slick/slick#1461

So I wonder if it makes sense to change Active Slick if it'll be fixed in 3.2.0.

@djx314
Copy link
Contributor Author

djx314 commented Mar 24, 2016

@rcavalcanti Sorry for leaving for a long time.
The reason above is not the single reason.
Take a look at the code:

(for {
  rowA <- modelA.save
  rowB <- modelB.save
} yield {
  true
}).transactionally

This code used by active-slick will take 3 transactionally. It will bring different meanings. I can't 100% believe that the code will work as what I expected. So if sometime I have to save some many to many models(about 3 tables), I need to controller the transactionally by myself. This is the main reason that I suggest to add a new EntityActions trait( with another name ) that all without transactionally.

@octonato
Copy link
Contributor

As far I understand, there is no nested transactions, therefore we will not have 3 transactions, but only one. Indeed, the transactionally method will be called 3 times, but only one transaction will be started. The outer one.

I personally think that we should adopt the principle of least surprise.

In your example, the code runs in a single transaction and that's not a surprise because you explicitly added a call to transactionally.

The same for the current EntityActions. When we call modelA.save the machinery behind (beforeInsert and beforeUpdate) will run on the same transaction of the insert or update. Again that's the least surprising.

EntityActionsNoTx will be very error prone. If a user calls modelA.save without calling transactionally, he may get wrong results and this will be very hard to debug.

That said, the only reason for an EntityActionsNoTx to exists is solely to avoid calling transactionally behind the scenes more than once while from the functionally point of view, it doesn't make any different if we call transactionally more than once.

@djx314
Copy link
Contributor Author

djx314 commented Mar 25, 2016

Oh, I see. Thanks a lot!

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

No branches or pull requests

2 participants