Skip to content

fix: log client-safe exceptions at ERROR instead of CRITICAL (#1193) - #1248

Open
ousamabenyounes wants to merge 1 commit into
overblog:masterfrom
ousamabenyounes:fix/issue-1193
Open

fix: log client-safe exceptions at ERROR instead of CRITICAL (#1193)#1248
ousamabenyounes wants to merge 1 commit into
overblog:masterfrom
ousamabenyounes:fix/issue-1193

Conversation

@ousamabenyounes

Copy link
Copy Markdown
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Documented? no
Fixed tickets #1193
License MIT

Problem

ErrorLoggerListener only special-cases UserError and UserWarning; every other
exception falls through to LogLevel::CRITICAL:

if ($exception instanceof UserError) { ... return; }
if ($exception instanceof UserWarning) { ... return; }

$this->log($exception, LogLevel::CRITICAL);

ArgumentsValidationException implements GraphQL\Error\ClientAware and returns true
from isClientSafe(), so it is a client fault (an invalid input), not an internal
server error. It was nonetheless logged as CRITICAL, polluting error logs for what is
routine input validation.

Fix

Add a branch, after the UserError / UserWarning checks and before the CRITICAL
fallthrough, that treats any client-safe ClientAware exception like a UserError:
log its previous cause (if any) at ERROR, and never at CRITICAL.

UserError and UserWarning (also ClientAware) keep their existing, more specific
handling because they are matched earlier. A ClientAware exception with
isClientSafe() === false still falls through to CRITICAL.

Test verification (RED → GREEN)

Two data-provider cases added to ErrorLoggerListenerTest for
ArgumentsValidationException (with and without a previous cause).

RED — on the unmodified branch (fix reverted, tests only), both cases hit the
CRITICAL fallthrough:

FAILURES!
Tests: 10, Assertions: 9, Failures: 2, Risky: 1.
(expected 'error' / no log, actual 'critical')

GREEN — with the fix:

OK (10 tests, 10 assertions)

Full suite is iso-baseline (the 5 pre-existing GraphDumpSchemaCommandTest failures on
master are unrelated and unchanged): Tests: 714, Failures: 5.

ErrorLoggerListener only special-cased UserError and UserWarning; every other
exception fell through to LogLevel::CRITICAL. A client-safe exception such as
ArgumentsValidationException (implements ClientAware with isClientSafe() === true)
is a client fault, not an internal server error, yet it was logged as CRITICAL
and polluted error logs.

Add a branch that treats any ClientAware exception marked client-safe like a
UserError: log its previous cause (if any) at ERROR and never at CRITICAL.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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