jdaugherty commented on PR #15347: URL: https://github.com/apache/grails-core/pull/15347#issuecomment-3803254962
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? -- 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]
