refactor!: give every service exception a shared SupabaseException base - #1644
refactor!: give every service exception a shared SupabaseException base#1644spydon wants to merge 7 commits into
Conversation
|
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 | ✅ 5✅ Passed checks (5 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 |
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
`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.
d68a53b to
aa160d3
Compare
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 extend oneSupabaseExceptioninsupabase_common:Every package re-exports
SupabaseException, so a single catch handles a failure from any service:Breaking changes
AuthException.statusCode(String?)statusCode(int?)AuthException.codeAuthException.errorCodeStorageException.statusCode(String?)statusCode(int?)StorageException.errorStorageException.errorCodeStorageException.fromJson(json, '404')StorageException.fromJson(json, 404)PostgrestException.code(PostgREST code, or the HTTP status when the body was not JSON)errorCode(PostgREST/PostgreSQL code only) andstatusCode(HTTP status)PostgrestException.fromJson(json, code: 409)PostgrestException.fromJson(json, statusCode: 409)PostgrestException.toJson()keyscodestatusCodeanderrorCodeFunctionException.status(int)statusCode(int?)FunctionException.reasonPhrasemessageFunctionsFetchException.status == 0statusCode == nullFunctionResponse.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/PostgreSQL code. A duplicate-key error, for example, is nowstatusCode: 409, errorCode: '23505'.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.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.Auth's
getSessionFromUrlalso had to change how it reads an error callback:error_codeholds either a numeric status (older links) or a code such asotp_expired, so the numeric form becomesstatusCodeand anything else becomeserrorCode, falling back to theerrorparameter. Both shapes are covered by tests.Every auth exception subclass used to repeat the same
toString; the base now prints 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 parsesas JSON but is not an object, for example the
["upstream connect error"]agateway can return, throws a
TypeErroron that cast, which theon FormatExceptiondoes not catch, so it escaped instead of surfacing as aStorageException. Fixed here, with a regression test; #1647 later replaces theinline guard with the shared
tryDecodeJsonObjecthelper.Out of scope
RealtimeSubscribeExceptionand the sealedIcebergExceptionkeep their own shapes for now. MakingFunctionExceptionsealed is tracked separately in #1550.Testing
melos analyze,dcm analyzeandmelos formatclean. Full test suites pass forgotrue,postgrest,storage_client,realtime_client,supabase,supabase_common(against the local stack) andsupabase_flutter, plus theexamplesanalyzer. The capability matrix symbol and drift checks pass unchanged.