Skip to content

Stop logging aggregated GraphQL client input errors as server errors - #41141

Open
lbajsarowicz wants to merge 1 commit into
magento:2.4-developfrom
lbajsarowicz:fix/graphql-aggregated-client-errors-logging
Open

Stop logging aggregated GraphQL client input errors as server errors#41141
lbajsarowicz wants to merge 1 commit into
magento:2.4-developfrom
lbajsarowicz:fix/graphql-aggregated-client-errors-logging

Conversation

@lbajsarowicz

Copy link
Copy Markdown
Contributor

Description

Magento\Framework\GraphQl\Query\ErrorHandler intentionally does not log GraphQL errors that were caused by invalid client input: log() returned early when the error carried extensions['category'] === GraphQlInputException::EXCEPTION_CATEGORY.

That decision was not honoured for aggregate exceptions. GraphQlInputException implements AggregateExceptionInterface, so it can carry child errors added through addError(). When it does, handle() logged every child error with $this->logger->error($aggregatedError) directly, bypassing the suppression that had just been applied to the parent error.

The result is inconsistent by the shape of the exception rather than by its cause: a GraphQlInputException without child errors is suppressed, while the very same exception with child errors is written to the exception log at ERROR level. Mutations that report user errors this way — and bots probing the endpoint with invalid input — produce continuous ERROR noise for requests the server correctly rejected as client errors.

The suppression decision is now computed once per error and applied to both logging calls. Errors that are not client input errors keep being logged exactly as before, including their aggregated child errors.

The formatted response is unchanged. Every error that was formatted before is still formatted, in the same order — only the logging is affected.

Related Issues

Related to #34973 (broader request to keep client-safe GraphQL exceptions out of the exception log). This PR fixes only the narrow inconsistency inside ErrorHandler described above and does not change how any other exception category is logged.

Manual testing scenarios

  1. Install Magento in production mode.
  2. Execute a GraphQL mutation whose resolver throws a GraphQlInputException that carries child errors added via addError().
  3. Before this change: var/log/exception.log (or the configured exception handler) contains one ERROR entry per child error.
  4. After this change: no entry is logged, and the GraphQL response still contains all child error messages.
  5. Verify no over-suppression: trigger an error whose previous exception is a non-client aggregate (for example Magento\Framework\Exception\InputException with child errors) and confirm the parent and each child error are still logged.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit tests (extended Magento\Framework\GraphQl\Test\Unit\Query\ErrorHandlerTest, including a case proving non-client aggregates are still logged)
  • All automated tests passed successfully (all tests are executed on GitHub Actions)

ErrorHandler suppresses logging for errors caused by invalid client input,
but the aggregate branch logged every child error of an AggregateExceptionInterface
directly, so a GraphQlInputException carrying child errors was logged while the
same exception without them was suppressed. The suppression decision is now made
once per error and applied to both logging calls; the formatted response is
unchanged.
@m2-assistant

m2-assistant Bot commented Aug 21, 2026

Copy link
Copy Markdown

Hi @lbajsarowicz. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@lbajsarowicz

Copy link
Copy Markdown
Contributor Author

@magento run all tests

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant