Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Object delete() in a controller no longer works without a Transaction as of 3.2.4 #10444

Closed
gsuhm opened this issue Feb 1, 2017 · 20 comments
Closed

Comments

@gsuhm
Copy link

gsuhm commented Feb 1, 2017

Deleting an object in a controller does not actually cause a delete. Might be other db-issues but the sample project demonstrates the broken delete():

def delete() {

    def a = Test.findById(params.id)
    a.delete()
    redirect controller: 'SaveTest'

}

Steps to Reproduce

check out project: https://github.com/gsuhm/saveIssue
Run app: http://localhost:8091/saveTest/index
Click "add" to add new sample objects.
Click "delete" (nothing happens)

Expected Behaviour

downgrade to 3.2.3 in gradle.properties - re-run above steps and objects will successfully delete.

Environment Information

  • Operating System: OSX, Linux (ubuntu 16)
  • Grails Version: 3.2.4
  • JDK Version: 1.8.0_111

Example Application

https://github.com/gsuhm/saveIssue

@jeffscottbrown
Copy link
Member

I suspect it is being deleted and the transaction isn't being committed. If you annotate the delete action (or the controller class) with @grails.transaction.Transactional, does the behavior change?

@gsuhm
Copy link
Author

gsuhm commented Feb 1, 2017

Yes, it works if you add @transactional to the controller. Wasn't needed in 3.2.3.

@jeffscottbrown
Copy link
Member

Yes, it works if you add @transactional to the controller.

Right. Thanks for the feedback.

@jeffscottbrown
Copy link
Member

It is a breaking change which we generally don't want to introduce in a minor point release so we will look at it and see if it is problematic to revert the behavior, but the use case is a dubious one. It isn't a great idea to be doing a delete outside of a transaction. For now you can make it work by using a transaction or by calling a.delete(flush: true).

Thanks for the feedback.

@jeffscottbrown
Copy link
Member

Another point, I think Hibernate 5.2 doesn't allow this behavior. I think it would throw an exception if you try to write outside of a transaction.

@AlexKovynev
Copy link

This means that all controller methods need to be annotated? Can i set Transactional to controller globally? static transactional = true is my choice for each controller? What about quartz plugin jobs?

@graemerocher
Copy link
Member

graemerocher commented Feb 3, 2017

You should never, ever write logic that modifies data that is not annotated with @Transactional. The connection will be in autoCommit mode which will lead to surprising behaviour and in addition read operations will be more expensive

If you look at the logic generated by generate-controller it already uses @Transactional

@gsuhm
Copy link
Author

gsuhm commented Feb 3, 2017

This is an issue bigger than just not having @transactional on your controller. If you do not have the annotation, the save() works in 3.2.4, but the delete() does not. Turning sql trace on shows the delete sql in 3.2.3 and it is missing in 3.2.4.

Hibernate: insert into test (id, version, a) values (null, ?, ?)
Hibernate: delete from test where id=? and version=?

@jeffscottbrown
Copy link
Member

This is an issue bigger than just not having @transactional on your controller.

Right. But the point is really that you really shouldn't be modifying the database outside of a transaction. You don't have to make your controllers transactional, but wherever your code is that is modifying the database, that should be transactional.

As I suggested above, it isn't great that this release included a breaking change, but it really only breaks code that isn't following general best practice.

@jeffscottbrown
Copy link
Member

This means that all controller methods need to be annotated?

No. It definitely doesn't mean that. Many controllers don't do anything with a database. If you did want all of the actions in a controller to be transactional, you could just annotate the controller class.

Don't think of it like "all controller methods need to be annotated". Think of it like "all of my code that is modifying the database needs to be transactional, and some of my code that is just reading from the database might need to be transactional".

@gsuhm
Copy link
Author

gsuhm commented Feb 3, 2017

@jeffbrown I don't disagree, but either controllers should be not transactional by default (and clearly documented that way), or the delete() should work like 3.2.3. Also, command objects that reference domain instances are also doing database updates behind the scenes, which implies controllers are by default transactional, no?

@jeffscottbrown
Copy link
Member

Also, command objects that reference domain instances are also doing database updates behind the scenes, which implies controllers are by default transactional, no?.

No. Controllers are not transactional by default.

Also, not all command objects reference domain instances and even those that do, not all of those are doing database updates. It depends on other factors in your app. Some do, some don't.

@gsuhm
Copy link
Author

gsuhm commented Feb 3, 2017

So that is good to know - each data operation is then in it's own transaction within a controller (if @transactional is not specified).

I know that not all command objects contain domain objects, but when they do, is that update done in the context of its own transaction then? (assuming @transactional was not specified)

@AlexKovynev
Copy link

AlexKovynev commented Feb 4, 2017

Funny, grails generate-controller does not work, and generate-all too, how can i do right things i don't know:(.
| Error Command not found generate-controller.

I think maybe some documentation in migration guide would be useful.
Till i annotate all my controllers, services and jobs as transactional just in case.

@benorama
Copy link

benorama commented Feb 5, 2017

As detailed in GORM #865 issue, it's not only during delete(), but any updates to a domain objects.
It looks like session flush / dirty checking is not performed at the end of the request?

Concerning this:

"all of my code that is modifying the database needs to be transactional, and some of my code that is just reading from the database might need to be transactional"

I understand the best practice to wrap strategic updates around a transaction (preferably in dedicated services), but I suppose there is an additional cost in terms of performance.
In some use cases, you might prefer performances over update consistency and not use transactional commits?

@graemerocher
Copy link
Member

On the contrary the performance will be worse without a transaction. With SQL databases EVERY interaction with the database happens within a transaction, the only difference is that when you don't specifically demarcate your transactional boundaries you are in "auto commit" mode, meaning each SQL executed is wrapped in a on demand transaction. See https://en.wikipedia.org/wiki/Autocommit

Autocommit mode, in theory, incurs per-statement transaction overhead, having often undesirable performance or resource utilization impact.

In addition if you mark a transaction as readonly there is less overhead because Hibernate does not need to perform dirty checking, which will again improve performance.

@graemerocher graemerocher added this to the grails-3.2.6 milestone Feb 6, 2017
@graemerocher graemerocher self-assigned this Feb 6, 2017
@graemerocher
Copy link
Member

This change is a default configuration change as flush mode now defaults to MANUAL. Adding the following to application.yml:

hibernate:
    flush:
        mode: AUTO

Restores the previous behaviour. This change was supposed to go into only master, but went into 6.0.x.

Flush mode AUTO is not recommended and the behavioural difference can mostly be resolved through correct use of transactions.

Now that the behaviour is out there and is going to be the default behaviour in the future, plus can be changed via configuration I am unconvinced any additional changes should be applied other than an update to the documentation.

graemerocher added a commit to grails/gorm-hibernate5 that referenced this issue Feb 6, 2017
@graemerocher graemerocher changed the title Object delete() in a controller no longer works as of 3.2.4 Object delete() in a controller no longer works without a Transaction as of 3.2.4 Feb 6, 2017
@graemerocher
Copy link
Member

Updated documentation on this issue

@mimkorn
Copy link

mimkorn commented Dec 21, 2022

Hi, for a few days I'm fighting with a similar problem — delete and update operations do not work as I would expect in unit tests. I'm at Grails 5.2.5, using Grails-testing-support and I have tests you can run like so, which behave very weirdly.

I am having big difficulties finding out what to do, since annotating with @transactional does not seem to be either possible or making any difference in the context of unit tests. Any idea what I can do here? I had opened up a ticket about this in grails-testing-support grails/grails-testing-support#259

@puneetbehl
Copy link
Contributor

@mimkorn From my perspective, It is not a good idea to commend on an already closed issue. Also, I think the problem which you are facing might not be related to this issue. In general, I would advise to write integration tests for testing any GORM operation.

I believe https://grails.slack.com is the best place to ask any questions as it may receive attention from the Grails Community there.

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

No branches or pull requests

8 participants