refactor!: split every service exception into a client and an api base - #1644
Open
spydon wants to merge 14 commits into
Open
refactor!: split every service exception into a client and an api base#1644spydon wants to merge 14 commits into
spydon wants to merge 14 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ChangesShared exception contract
Authentication exceptions
Functions exceptions
PostgREST exceptions
Storage exceptions
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/edge_functions/lib/main.dart (1)
363-367: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle transport failures without rendering a null status.
When a request fails before a response,
statusCodeis null. The current fallback displaysFunction failed with status null. Useerror.messagefor that case.Proposed fix
- : 'Function failed with status ${error.statusCode}'; + : error.statusCode == null + ? error.message + : 'Function failed with status ${error.statusCode}';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/edge_functions/lib/main.dart` around lines 363 - 367, Update the FunctionException fallback in the error-handling flow to use error.message when error.statusCode is null, avoiding a rendered “status null” message. Preserve the existing details['error'] handling and continue using the status-based fallback when a status code is available.packages/postgrest/lib/src/types.dart (1)
27-49: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve normalized fields in
PostgrestException.fromJson().
toJson()writesstatusCodeanderrorCode, but the constructor only passes those fields in through separate arguments. When an app serializes an example exception, then deserializes the JSON withPostgrestException.fromJson(...),statusCodeanderrorCodedisappear. Add fallback reads forjson['statusCode']andjson['errorCode'], and cover this in a round-trip test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/postgrest/lib/src/types.dart` around lines 27 - 49, Update PostgrestException.fromJson() to use json['statusCode'] and json['errorCode'] as fallbacks when the statusCode and errorCode arguments are absent, preserving explicitly supplied arguments. Add a round-trip test covering toJson() followed by fromJson() and verifying both normalized fields remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/storage_client/lib/src/fetch.dart`:
- Around line 40-50: Update the error-body parsing flow around
StorageException.fromJson so valid non-object JSON values, including arrays,
strings, and null, are normalized to a StorageException containing the raw
error.body instead of triggering an uncaught TypeError; preserve the existing
object parsing and FormatException behavior, and add a regression test covering
a non-object JSON HTTP error response.
In `@packages/supabase_common/lib/src/supabase_exception.dart`:
- Around line 3-5: Correct the exception-contract documentation so it applies
only to exceptions extending SupabaseException, rather than every service
exception. Update the documentation comment in
packages/supabase_common/lib/src/supabase_exception.dart at lines 3-5 and the
inheritance statement in packages/supabase_common/README.md at lines 14-16;
explicitly scope the README statement to the migrated exception types and leave
RealtimeSubscribeException and IcebergException excluded.
---
Outside diff comments:
In `@examples/edge_functions/lib/main.dart`:
- Around line 363-367: Update the FunctionException fallback in the
error-handling flow to use error.message when error.statusCode is null, avoiding
a rendered “status null” message. Preserve the existing details['error']
handling and continue using the status-based fallback when a status code is
available.
In `@packages/postgrest/lib/src/types.dart`:
- Around line 27-49: Update PostgrestException.fromJson() to use
json['statusCode'] and json['errorCode'] as fallbacks when the statusCode and
errorCode arguments are absent, preserving explicitly supplied arguments. Add a
round-trip test covering toJson() followed by fromJson() and verifying both
normalized fields remain unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1746a526-08a7-4060-8c83-db9eeecb782a
📒 Files selected for processing (39)
examples/edge_functions/integration_test/functions_test.dartexamples/edge_functions/integration_test/invoke_test.dartexamples/edge_functions/lib/main.dartpackages/functions_client/example/functions_dart_example.dartpackages/functions_client/lib/functions_client.dartpackages/functions_client/lib/src/functions_client.dartpackages/functions_client/lib/src/types.dartpackages/functions_client/test/functions_dart_test.dartpackages/gotrue/lib/gotrue.dartpackages/gotrue/lib/src/fetch.dartpackages/gotrue/lib/src/gotrue_client.dartpackages/gotrue/lib/src/types/auth_exception.dartpackages/gotrue/test/client_test.dartpackages/gotrue/test/fetch_test.dartpackages/gotrue/test/otp_mock_test.dartpackages/gotrue/test/passkey_test.dartpackages/gotrue/test/provider_test.dartpackages/gotrue/test/src/gotrue_oauth_api_test.dartpackages/gotrue/test/src/types/auth_exception_test.dartpackages/gotrue/test/web3_auth_test.dartpackages/postgrest/example/main.dartpackages/postgrest/lib/postgrest.dartpackages/postgrest/lib/src/postgrest_builder.dartpackages/postgrest/lib/src/types.dartpackages/postgrest/test/basic_test.dartpackages/postgrest/test/transforms_test.dartpackages/postgrest/test/upsert_test.dartpackages/storage_client/lib/src/fetch.dartpackages/storage_client/lib/src/storage_file_api.dartpackages/storage_client/lib/src/types.dartpackages/storage_client/lib/storage_client.dartpackages/storage_client/test/basic_test.dartpackages/storage_client/test/client_test.dartpackages/storage_client/test/types_test.dartpackages/supabase_common/README.mdpackages/supabase_common/lib/src/supabase_exception.dartpackages/supabase_common/lib/supabase_common.dartpackages/supabase_common/test/supabase_exception_test.dartpackages/supabase_flutter/test/deep_link_test.dart
This was referenced Aug 5, 2026
spydon
changed the title
Tr00d
approved these changes
Aug 5, 2026
spydon
force-pushed
the
breaking/supabase-exception-base
branch
from
August 5, 2026 15:14
d68a53b to
aa160d3
Compare
8 tasks
spydon
force-pushed
the
breaking/supabase-exception-base
branch
2 times, most recently
from
August 10, 2026 14:39
556df34 to
68959ca
Compare
Collaborator
|
We have cases where an exception is thrown with only a message for some bad state in the client e.g. supabase-flutter/packages/gotrue/lib/src/gotrue_client.dart Lines 1069 to 1079 in bbf9b74 So these cases have no status or error code. Currently they are null in these cases. Error codes and status codes are only present for http/supabase api calls. So I'm wondering if it makes sense to separate those api exceptions from these client only exceptions and maybe even have a class with status code being non-nullable. Such that we have a |
Contributor
Author
Sounds like a good idea! |
spydon
force-pushed
the
breaking/supabase-exception-base
branch
from
August 12, 2026 08:21
68959ca to
6667939
Compare
Contributor
Author
|
@Vinzent03 what do you think about this solution? abstract class SupabaseException implements Exception {
final String message;
final String? errorCode;
const SupabaseException(this.message, {this.errorCode});
}
mixin SupabaseApiException on SupabaseException {
int get statusCode;
} |
spydon
changed the title
Collaborator
|
I think I really like the new style. But some small things:
Also may I ask for the reason of excluding the realtimesubscribe exception and iceberg here? Haven't looked into it really, but just wondering. |
Contributor
Author
Sounds like good suggestions, I'll fix those up!
Because they have zero overlap with the other exception structure. EDIT: EDIT2: |
`AuthException`, `StorageException`, `PostgrestException` and `FunctionException` each reimplemented the same message plus status shape under different field names and types. They now extend a single `SupabaseException` in `supabase_common`, so `on SupabaseException` catches a failure from any service. Reconciling the four shapes means: - `statusCode` is an `int?` everywhere. It was a `String?` in auth and storage, and `FunctionException.status` (an `int`) in functions. - The service specific code is `errorCode` everywhere: `AuthException.code`, `StorageException.error` and `PostgrestException.code` are gone. `PostgrestException.errorCode` now only holds the PostgREST/PostgreSQL code, since the HTTP status has its own field. - `PostgrestException.fromJson` takes `statusCode` instead of `code`. - `FunctionException` carries a `message` like the other exceptions. `reasonPhrase` is gone: the response's reason phrase becomes the message, falling back to a per-subtype default when the response has none. - `FunctionsFetchException.statusCode` is `null` instead of `0`, since no response reached the client. - Auth exception subclasses no longer each repeat `toString`; the base prints the concrete runtime type. Part of #1572 (tier 3), under the v3 umbrella #1278.
The exceptions in this package now carry the HTTP status as `statusCode`, so the successful response reporting it as `status` was the odd one out. It also lines up with `http.Response.statusCode`.
…ypes The base class doc and the README claimed every service exception extends `SupabaseException`, but `RealtimeSubscribeException` and `IcebergException` deliberately keep shapes of their own.
`_handleError` cast the decoded error body to `Map<String, dynamic>` inside a `try`/`on FormatException`. A body that parses as JSON but is not an object, for example the `["upstream connect error"]` a gateway can return, throws a `TypeError` on that cast, which the `on FormatException` does not catch, so it escaped instead of surfacing as a `StorageException`. The non-response branch also moved up front, which drops a level of nesting.
`SupabaseException` reported a `statusCode` and an `errorCode` that were
always null for the failures the client raises itself, such as a missing
session or a redirect url without an access token. The layer a failure came
from was not visible from its type.
`SupabaseException` now carries only the message and the error code, and a
new `SupabaseApiException` mixin adds the response's `statusCode` as a
non-nullable `int`. Catching it handles any failure a service answered with,
whichever service that was:
```dart
try {
await supabase.from('countries').select();
} on SupabaseApiException catch (error) {
print('${error.statusCode}: ${error.message}');
}
```
Per package:
- `AuthException` is message plus error code. `AuthApiException` carries the
status, and `AuthWeakPasswordException` now extends it. Both
`AuthRetryableFetchException` and `AuthUnknownException` straddle the two
layers, so they keep a nullable `statusCode` of their own.
- `AuthSessionMissingException` and `AuthInvalidJwtException` no longer
report a fabricated `400`; they report `session_missing` and `invalid_jwt`.
- `getSessionFromUrl` throws an `AuthApiException` when `error_code` is a
numeric status and a plain `AuthException` when it is a code such as
`otp_expired`.
- `StorageApiException` is new and holds what a storage response reported,
including `fromJson`, which now takes a non-nullable status code.
- `PostgrestException` is always a response, so it mixes the api exception in
directly and its `statusCode` is required.
- `FunctionsApiException` is new and sits between `FunctionException` and the
relay/http subtypes. `FunctionsFetchException` has no status at all rather
than a null one.
Part of #1572 (tier 3), under the v3 umbrella #1278.
… unknown exceptions `AuthRetryableFetchException` and `AuthUnknownException` each declared a nullable `statusCode` of their own, so the auth hierarchy had three `statusCode` members at two different nullabilities while `AuthException` itself had none. Both fields were derived rather than load bearing: - `AuthRetryableFetchException` covers a transport failure and a 5xx alike, so it could never promise a status. When the service did answer, the message already carries what it said, falling back to the reason phrase or `HTTP <status>`. - `AuthUnknownException` computed the status from `originalError`, which it exposes, so callers can read it from the response directly. Neither is read anywhere in the client; the retry path only tests the type. `statusCode` now means one thing across every package: a non-nullable `int` that exists exactly when the exception is a `SupabaseApiException`.
…iException Dropping `statusCode` from `AuthRetryableFetchException` cost the status on a retryable failure that the service did answer. Two of the three throw sites put it in the message via `_getStatusMessage`, but the third takes the service's own text from a JSON error body, so the status was unreachable. `AuthRetryableApiException` extends `AuthRetryableFetchException` and mixes in `SupabaseApiException`, so the status is back with no nullable field: - `AuthRetryableFetchException` is the transport case, where the request never reached the service and there is no status to report. - `AuthRetryableApiException` is a 5xx the service answered with. `e is AuthRetryableFetchException` still catches both, so the retry and refresh paths in `GoTrueClient` are unchanged.
Most of the exception test files checked that Dart assigns constructor arguments to fields and that `extends` produces a subtype. Neither can fail. Removed: - Constructor round trips: build an exception, read back the values just passed in. Nine of these across the three files. - Type checks the compiler already guarantees, such as `expect(AuthApiException(...), isA<AuthException>())`, where the static type of the expression makes the matcher a tautology. The hierarchy test keeps its lists typed as `SupabaseException` so its checks stay real narrowings. - The whole `Error handling scenarios` group in gotrue: six tests restating per-class tests with different string literals. - `maintains equality based on parent class and reasons`, which compared field by field and never invoked `==`, so it would have passed with equality broken. - Per-subclass `toString` tests for subclasses that do not override it, which only re-asserted the base and mixin formats already covered in `supabase_common`. What is left covers behaviour that can break: the two `toString` formats, equality including the runtime type guard, the constructors that derive a message or an error code, `StorageApiException.fromJson`, and which exceptions are `SupabaseApiException`. The kept `toString` assertions are now exact matches instead of `contains`, so a format change fails them. gotrue's exception test drops from 40 tests to 17.
…rename PostgrestException Two alignment fixes from review. `FunctionsHttpException` and `FunctionsApiException` described the same thing: a non-2xx response from the Edge Function. That is the definition of an api exception, so `FunctionsApiException` takes over the name and the default message, and `FunctionsHttpException` is gone. `FunctionsRelayException` stays, since a relay error means the function may never have run. `PostgrestException` becomes `PostgrestApiException`. Every Postgrest failure comes from a response, so it was the only api exception not carrying the `Api` part, and the plain name is now free for a client raised base if one is ever needed. Also trims the exception dartdocs to what a caller needs, dropping the notes that explained the naming and the layering rationale.
spydon
force-pushed
the
breaking/supabase-exception-base
branch
from
August 12, 2026 15:30
dd6f916 to
2476ef4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tier 3 of #1572, first of four PRs. Under the v3 umbrella #1278.
What
AuthException,StorageException,PostgrestExceptionandFunctionExceptioneach reimplemented the same message plus status shape under different field names and types. They now share one hierarchy insupabase_common:A plain
SupabaseExceptionis a failure the client raised on its own, before or without a request. ASupabaseApiExceptionis a failure a service reported, sostatusCodeis a non-nullableintthat exists exactly when a service answered, and the layer a failure came from is visible from its type alone. Both are re-exported from every package, so one catch handles a failure from any service:The two level split came out of @Vinzent03's review. A single base handed every client raised failure a
statusCodeand anerrorCodethat were always null, and storage went further than that: a request that never reached the server producedStorageException(statusCode: 'ClientException'), the exception's own type name in the status field.SupabaseApiExceptionis a mixin rather than a second base class because the two axes are orthogonal.AuthApiExceptionhas to be anAuthException, since that is what callers catch, and also the cross service type for "a service answered". Dart gives you one superclass, so one of the two has to be a mixin or an interface, and neither can hold state. That is why each API exception declaresstatusCodeitself.The hierarchy
gotrueAuthException,AuthPKCEGrantCodeExchangeError,AuthSessionMissingException,AuthInvalidJwtException,AuthUnknownException,AuthRetryableFetchExceptionAuthApiException,AuthWeakPasswordException,AuthRetryableApiException(new)postgrestPostgrestApiExceptionstorage_clientStorageExceptionStorageApiException(new)functions_clientFunctionException,FunctionsFetchExceptionFunctionsApiException,FunctionsRelayExceptionPostgrestApiExceptionmixesSupabaseApiExceptionin directly instead of gaining a subclass, since every Postgrest failure comes from a response. It keeps theApipart of the name so it lines up with the other packages, which leaves the plainPostgrestExceptionname free for a client raised base if one is ever needed.Breaking changes
AuthException.statusCode(String?)AuthApiException.statusCode(int, required)AuthException.codeAuthException.errorCodeAuthSessionMissingException.statusCode == '400'errorCodeissession_missingAuthInvalidJwtException.statusCode == '400'errorCodeisinvalid_jwtAuthWeakPasswordException extends AuthExceptionextends AuthApiExceptionAuthRetryableFetchException.statusCodeAuthRetryableApiException, which carries itAuthUnknownException.statusCodeoriginalErrorStorageException.statusCode(String?)StorageApiException.statusCode(int, required)StorageException.errorerrorCodeStorageException.fromJson(json, '404')StorageApiException.fromJson(json, 404)PostgrestExceptionPostgrestApiExceptionPostgrestException.code(PostgREST code, or the HTTP status when the body was not JSON)errorCode(PostgREST or PostgreSQL code only) andstatusCode(HTTP status, non-nullable)PostgrestException.fromJson(json, code: 409)PostgrestApiException.fromJson(json, statusCode: 409)PostgrestException.toJson()keycodestatusCodeanderrorCodeFunctionsHttpExceptionFunctionsApiExceptionFunctionException.status(int)FunctionsApiException.statusCode(int, required)FunctionException.reasonPhrasemessageFunctionsFetchException.status == 0FunctionResponse.statusFunctionResponse.statusCodeNotes on the less mechanical ones:
code. It used to stuff the HTTP status intocodewhen the error body was not JSON, socodewas sometimesPGRST116and sometimes409. The status now has its own field anderrorCodeis only ever a PostgREST or PostgreSQL code. A duplicate key violation reads asstatusCode: 409, errorCode: '23505'.AuthSessionMissingExceptionandAuthInvalidJwtExceptionare raised by the client, never by the service, so the400they reported was invented. They report an error code instead.AuthRetryableFetchExceptionsplit in two. It was thrown both for a transport failure, which has no status, and for a 5xx, which has one. It now covers only the transport case, andAuthRetryableApiExceptionextends it for a 5xx.e is AuthRetryableFetchExceptionstill catches both, so the retry and refresh paths inGoTrueClientare unchanged.FunctionExceptionhad no message, onlystatus,detailsandreasonPhrase. The response's reason phrase becomes the message, and when the response carries none, as over HTTP/2, each subtype falls back to its own default, matching supabase-js:'Failed to send a request to the Edge Function','Relay error invoking the Edge Function','Edge Function returned a non-2xx status code'. The response body is still indetails.FunctionsHttpExceptionis gone. It andFunctionsApiExceptionboth meant "the Edge Function answered with a non-2xx status", which is the definition of an api exception, soFunctionsApiExceptiontook over the name and the default message.FunctionsRelayExceptionstays, because a relay error means the function may never have run.FunctionResponse.statusis renamed too. With the exceptions in that package reportingstatusCode, the successful response was the only thing left calling itstatus. It also lines up withhttp.Response.statusCode.AuthExceptioncompares the runtime type.statusCodemoved to the subclasses, so equality had to move with it. The base compares the concrete type, message and error code, and the API subclasses add the status, which keeps equality symmetric across the hierarchy.getSessionFromUrlalso had to change how it reads an error callback:error_codeholds either a numeric status (older links) or a code such asotp_expired. The numeric form now throws anAuthApiExceptionand anything else a plainAuthExceptionwhoseerrorCodefalls back to theerrorparameter. Both shapes are covered by tests.Every auth exception subclass used to repeat the same
toString; the shared classes now print the concrete runtime type, so only the subtypes with extra fields (originalError,reasons,details,hint) override it.A bug this surfaces
Fetch._handleErrorin storage cast the decoded error body toMap<String, dynamic>inside atry/on FormatException. A body that parses as JSON but is not an object, for example the["upstream connect error"]a gateway can return, throws aTypeErroron that cast, which theon FormatExceptiondoes not catch, so it escaped instead of surfacing as aStorageException. Fixed here, with a regression test; #1647 later replaces the inline guard with the sharedtryDecodeJsonObjecthelper.Testing
dart analyze,dcm analyzeanddart formatare clean. Full test suites pass forgotrue,postgrest,storage_client,functions_client,realtime_client,supabase,supabase_common(against the local stack) andsupabase_flutter, plus the examples analyzer. The capability matrix symbol, drift and schema checks pass with the new symbols registered.auth_exception_test.dartloses more lines than it gains, which is deliberate. It had grown a large number of tests that could not fail: constructor round trips reading back the values just passed in,isAchecks the compiler already guarantees, a wholeError handling scenariosgroup restating other tests with different string literals, and one test named after equality that compared field by field and never invoked==. What is left covers behaviour that can break, and itstoStringassertions are exact matches rather thancontains. The real coverage for this change is infetch_test.dart, which drives HTTP responses through the client and asserts the exception that comes out.Migration
MIGRATION.mdgains a section for the new hierarchy: the shape, the full rename table, and the one case that needs a code change rather than a rename, which is reading a status off a per-service base.Out of scope
RealtimeSubscribeExceptionhas nomessageat all, only aRealtimeSubscribeStatusanddetails, and it reports a channel subscription outcome rather than a request failure. It does not fit the base without inventing a message, so it keeps its own shape.The sealed
IcebergExceptionis a closer fit and is worth migrating, in its own PR: it already hasmessage, a non-nullablestatusCode, atypethat maps ontoerrorCode, anddetails. It also carries the samestatusCode == 0sentinel for a network failure that this PR removed fromFunctionsFetchException, so it wants the same client and api split rather than a straight rename.Making
FunctionExceptionsealed is tracked separately in #1550.