pharo-rdbms / glorp

Generic Lightweight Object Relational Persistence
MIT License
23 stars 24 forks source link

Nesting inUnitOfWork: [] produces no errors #148

Closed lopezca closed 7 months ago

lopezca commented 1 year ago

The code for inUnitOfWorkDo: [ ] contains a guard (myUnitOfWork) to avoid creating a new UnitOfWork object to track object changes.

image

With this code, nested calls to inUnitOfWork: will work just fine as they will use the outer most UOW object created.

The Glorp booklet also mentions this briefly:

image

BUT

the code for beginUnitOfWork states that nesting is not possible yet.

image

Questions:

1) Will there be errors if creating nested units of work due to something else? If so, the code should emit an error if a UOW already exist.

2) It's ok to have nesting UOW's as in reality, only the most outer one will control comit/rollback. if so, then simply updating the error to: ' Cannot create a new UOW when one is already created' would be sufficient to clarify the situation.

Please comment.

eMaringolo commented 1 year ago

I think the valid option is 2. beginUnitOfWork should be a private method, because idiomatically speaking, what you end up using is something like inUnitOfWorkDo: which if it's called within an existing UOW will use the outermost (as in, the existing) one.

JoachimTuchel commented 12 months ago

@eMaringolo I don't think the assumption about inUnitOfWorkDo: is valid. It would require to build an extra level of mementos in an application for GUI applications. We do use beginUnitOfWork and don't use inUnitOfWorkDo: because that would mean you have to "buffer" all modifications to objects until you do an inUnitOfWorkDo:.

The way I understand @lopezca the change suggested in option 2 is to just change the error text, right? If so, I don't think the suggested text would be an improvement. I think the term "nested transactions" is common enough to make this error message clear. Maybe the suggested text can be added to the existing one, but I wouldn't like to see the word "nested" or "nesting" disappear .

JoachimTuchel commented 12 months ago

@eMaringolo I'd like to understand why you think reusing the outer context/uow is a valid thing. It's been a while since I learned about databases, but in my memories, a nested transaction/uow can be rolled back without affecting its outer transaction/uow. In case of Glorp, this is simply not implemented (by design / unfinishedness), so acting "as if" is probably a misleading approach. You shouldn't be able to "create" an inner uow without being informed that this is probably not what you think it is. If Glorp creates a nested uow without protesting, you may end up rolling back the outer uow when you think you just roll back the inner uow. So I guess an exception is the only correct thing you can get. In the end you must design your application in a way that doesn't require nested transactions/uow's and in order to make sure you do, Glorp should make it very clear if you try to use a non-existing feature.

So the only way out is to either throw an exception (status quo) or implement nesting transactions (who dares?)

JoachimTuchel commented 7 months ago

Carlos, could you add a comment on what the conclusion of this discussion is? In how far is this issue completed?