Skip to content

Fix ignored failOnError argument in DeleteEntityDataFetcher#15692

Open
jamesfredley wants to merge 2 commits into
7.2.xfrom
fix/graphql-delete-failonerror
Open

Fix ignored failOnError argument in DeleteEntityDataFetcher#15692
jamesfredley wants to merge 2 commits into
7.2.xfrom
fix/graphql-delete-failonerror

Conversation

@jamesfredley
Copy link
Copy Markdown
Contributor

Summary

DeleteEntityDataFetcher.deleteInstance() called instance.delete(failOnError: true), but GORM's delete(Map) only honors the flush parameter - the failOnError argument is silently ignored. As a result the data fetcher gained no error-handling benefit from it.

This switches the call to instance.delete(flush: true), so the delete executes immediately and any failure (e.g. constraint violation, optimistic locking) is raised at that point and surfaces through the fetcher's existing try/catch to responseHandler.createResponse(env, false, exception). This matches the flush: true delete guidance documented in #15599.

Changes

  • DeleteEntityDataFetcher: delete(failOnError: true) to delete(flush: true).
  • DeleteEntityDataFetcherSpec: added an interaction test asserting deleteInstance calls delete(flush: true) and no longer passes the ignored failOnError argument.

Testing

./gradlew :grails-data-graphql-core:test --tests "org.grails.gorm.graphql.fetcher.impl.DeleteEntityDataFetcherSpec" --tests "org.grails.gorm.graphql.fetcher.impl.SoftDeleteEntityDataFetcherSpec"

All pass, including the existing happy-path / invalid-id cases and the SoftDeleteEntityDataFetcher (which overrides deleteInstance) regression check.

Flagged during review of #15599.

GORM's delete(Map) only honors the flush parameter, so the
failOnError: true argument passed in deleteInstance() was silently
ignored. Switch to delete(flush: true) so the delete executes
immediately and any failure surfaces through the data fetcher's
response handler.

Flagged during review of #15599.

Assisted-by: claude-code:claude-4.8-opus
Copilot AI review requested due to automatic review settings May 29, 2026 03:07
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request fixes DeleteEntityDataFetcher.deleteInstance() to use a GORM delete option that is actually honored, ensuring delete failures surface immediately and are handled by the existing try/catch response creation logic.

Changes:

  • Updated DeleteEntityDataFetcher.deleteInstance() from delete(failOnError: true) to delete(flush: true) so the delete is executed/flushed immediately.
  • Added a Spock interaction test to assert the fetcher calls delete(flush: true) and does not pass the ignored failOnError argument.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
grails-data-graphql/core/src/main/groovy/org/grails/gorm/graphql/fetcher/impl/DeleteEntityDataFetcher.groovy Uses flush: true for deletes so failures surface at the delete point and can be captured by existing error handling.
grails-data-graphql/core/src/test/groovy/org/grails/gorm/graphql/fetcher/impl/DeleteEntityDataFetcherSpec.groovy Adds an interaction test locking in the delete(flush: true) call and preventing regression to the ignored failOnError argument.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants