jpdev01 commented on PR #15347: URL: https://github.com/apache/grails-core/pull/15347#issuecomment-3831965233
> To address the intro/comments: > > I'm not going to say no to this change if it's optional, but in general, I think this is an extremely bad smell in the code to use transactions on setters. Off the top of my head, here are some of the reasons I think this is bad: > > * hydration / databinding will use setters. Starting and stopping transactions can cause data to save and data being saved during binding is likely a very bad idea. > * Also, what if the transaction = new transaction. The whole idea of a transaction is it's atomic, but as part of any databinding, multiple setters/getters will be called. This breaks the concept of a single action since each setter would cause a new transaction. > * setters are often used to set inputs to an object, if you were to do this on a service, it means that a transaction will wrap every bean wiring. That overhead is very likely not something you'd want. > * Groovy 4 focused on the bean spec & standardizing; I think there will be unintended consequences of this change for metaclass handling & places where the bean spec is used (like data binding / domains). > * This behavior will diverge from Spring's `@Transactional` and the differences are probably going to cause confusion. > * If a transaction changes data on a get, getters are no longer read only. I think that's an extremely dangerous precedent to set. > * Although this wouldn't necessarily apply to the domain, if using bean proxies (which spring does by default), I suspect there will be issues with getters/setters having transactions. > > Will take a look at the code once people agree on a path forward. I do think this would have to go into 7.1 and not 7.0 too. @matrei @jamesfredley thoughts? Currently we have some service methods in services named like setters (for example, `setAsDone`), but in practice they represent business commands, not simple property mutation. My intention is not to make bean setters/getters transactional, nor to allow transactions to be started during data binding or object hydration. These methods are only used as explicit service-level operations, invoked from application use cases, and are not part of the bean/property specification. To avoid exactly the issues you pointed out (data binding side effects, multiple transactions during object construction, confusion with the Spring transactional model, and unexpected interactions with the bean specification), I plan to adjust the method signatures and naming so that they clearly express application commands (for example, `markAsDone`, complete, etc.), while keeping setters strictly for simple state mutation. This way, transactional boundaries remain at the service layer, but without giving these methods setter semantics or exposing them to any automatic binding or property access mechanisms. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
