fix(postgrest): handle maybe_single zero-row responses correctly#1384
fix(postgrest): handle maybe_single zero-row responses correctly#1384Dodothereal wants to merge 1 commit intosupabase:mainfrom
Conversation
|
@olirice @silentworks could you please review this bugfix for #1207 when you have time? Thanks. |
📝 WalkthroughWalkthroughThe PR fixes a bug where Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/postgrest/src/postgrest/_async/request_builder.py`:
- Around line 107-120: The substring check in _is_zero_rows_error improperly
matches "10 rows"/"20 rows"; update _is_zero_rows_error to use a regex with word
boundaries (e.g. re.search(r"\b0 rows\b", field)) when scanning error.details,
error.message, and error.hint so only the exact phrase "0 rows" matches; apply
the same change to the equivalent sync implementation (the non-async
RequestBuilder/_is_zero_rows_error) and ensure the execute method still returns
None for true zero-row matches and re-raises other APIError instances
(AsyncSingleRequestBuilder stays used for the async path).
In `@src/postgrest/src/postgrest/_sync/request_builder.py`:
- Around line 107-120: The _is_zero_rows_error helper currently uses a substring
check ("0 rows" in field) which can produce false positives; update PostgREST
error detection in _is_zero_rows_error (used by execute and the
SyncSingleRequestBuilder flow) to use a word-boundary regular expression that
matches the standalone phrase "0 rows" (e.g. r"\b0 rows\b") against APIError
fields (error.details, error.message, error.hint) and return True only on a
regex match; apply the same change to the async counterpart in
request_builder.py for consistency.
| @staticmethod | ||
| def _is_zero_rows_error(error: APIError) -> bool: | ||
| return any( | ||
| field is not None and "0 rows" in field | ||
| for field in (error.details, error.message, error.hint) | ||
| ) | ||
|
|
||
| async def execute(self) -> Optional[SingleAPIResponse]: | ||
| r = None | ||
| try: | ||
| r = await AsyncSingleRequestBuilder(self.request).execute() | ||
| except APIError as e: | ||
| if e.details and "The result contains 0 rows" in e.details: | ||
| return await AsyncSingleRequestBuilder(self.request).execute() | ||
| except APIError as error: | ||
| if self._is_zero_rows_error(error): | ||
| return None | ||
| if not r: | ||
| raise APIError( | ||
| { | ||
| "message": "Missing response", | ||
| "code": "204", | ||
| "hint": "Please check traceback of the code", | ||
| "details": "Postgrest couldn't retrieve response, please check traceback of the code. Please create an issue in `supabase-community/postgrest-py` if needed.", | ||
| } | ||
| ) | ||
| return r | ||
| raise |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for the sync version mentioned in the review
find . -name "*.py" -type f | head -20Repository: supabase/supabase-py
Length of output: 1034
🏁 Script executed:
# Look for the sync version of request_builder
fd -t f "request_builder" --type fRepository: supabase/supabase-py
Length of output: 734
🏁 Script executed:
# Check the current file structure and content
cat -n src/postgrest/src/postgrest/_async/request_builder.py | sed -n '100,130p'Repository: supabase/supabase-py
Length of output: 1231
🏁 Script executed:
# Search for _is_zero_rows_error in sync code
rg "_is_zero_rows_error" -A 5 -B 5Repository: supabase/supabase-py
Length of output: 3594
🏁 Script executed:
# Search for test files that might show actual error messages from PostgREST
fd -t f "test" | grep -E "(request_builder|error)" | head -20Repository: supabase/supabase-py
Length of output: 566
🏁 Script executed:
# Check if there are any existing error handling patterns in the codebase
rg "0 rows" -B 3 -A 3Repository: supabase/supabase-py
Length of output: 2326
🏁 Script executed:
# Look for test cases with multiple rows error messages
rg "multiple.*rows|row.*returned" -B 3 -A 3 -t pyRepository: supabase/supabase-py
Length of output: 14020
🏁 Script executed:
# Check test cases in request builder tests for zero rows handling
cat -n src/postgrest/tests/_sync/test_request_builder.py | head -100Repository: supabase/supabase-py
Length of output: 4805
🏁 Script executed:
# Search for PGRST116 error details to understand the error message format
rg "PGRST116|The result contains" -B 5 -A 5 -t pyRepository: supabase/supabase-py
Length of output: 5613
🏁 Script executed:
# Check if there are any integration tests showing actual API responses
cat -n src/postgrest/tests/_async/test_request_builder.py | head -150Repository: supabase/supabase-py
Length of output: 7075
🏁 Script executed:
# Verify the substring matching issue with actual PostgREST error format
python3 << 'EOF'
import re
# Actual error messages from tests
error_0_rows = "The result contains 0 rows"
error_2_rows = "The result contains 2 rows"
error_10_rows = "The result contains 10 rows"
error_20_rows = "The result contains 20 rows"
# Current implementation (substring match)
print("Current implementation (substring match):")
print(f' "0 rows" in "{error_0_rows}" = {"0 rows" in error_0_rows}')
print(f' "0 rows" in "{error_2_rows}" = {"0 rows" in error_2_rows}')
print(f' "0 rows" in "{error_10_rows}" = {"0 rows" in error_10_rows}')
print(f' "0 rows" in "{error_20_rows}" = {"0 rows" in error_20_rows}')
# Proposed fix (word boundary)
print("\nProposed fix (word boundary regex):")
pattern = r"\b0 rows\b"
print(f' re.search(r"\\b0 rows\\b", "{error_0_rows}") = {bool(re.search(pattern, error_0_rows))}')
print(f' re.search(r"\\b0 rows\\b", "{error_2_rows}") = {bool(re.search(pattern, error_2_rows))}')
print(f' re.search(r"\\b0 rows\\b", "{error_10_rows}") = {bool(re.search(pattern, error_10_rows))}')
print(f' re.search(r"\\b0 rows\\b", "{error_20_rows}") = {bool(re.search(pattern, error_20_rows))}')
EOFRepository: supabase/supabase-py
Length of output: 606
🏁 Script executed:
# Check if 're' module is already imported in the async request_builder file
head -30 src/postgrest/src/postgrest/_async/request_builder.py | grep -E "^import|^from"Repository: supabase/supabase-py
Length of output: 545
🏁 Script executed:
# Also check the sync version for comparison
head -30 src/postgrest/src/postgrest/_sync/request_builder.py | grep -E "^import|^from"Repository: supabase/supabase-py
Length of output: 540
Fix substring matching to avoid false positives on "10 rows", "20 rows", etc.
The substring check "0 rows" in field incorrectly matches "The result contains 10 rows" or "20 rows", causing multi-row errors to return None instead of raising. Use word boundaries with re.search(r"\b0 rows\b", field) to match only the exact "0 rows" error. This issue exists in both the async and sync paths.
🛠️ Proposed fix
+import re
`@staticmethod`
def _is_zero_rows_error(error: APIError) -> bool:
return any(
- field is not None and "0 rows" in field
+ field is not None and re.search(r"\b0 rows\b", field)
for field in (error.details, error.message, error.hint)
)🤖 Prompt for AI Agents
In `@src/postgrest/src/postgrest/_async/request_builder.py` around lines 107 -
120, The substring check in _is_zero_rows_error improperly matches "10 rows"/"20
rows"; update _is_zero_rows_error to use a regex with word boundaries (e.g.
re.search(r"\b0 rows\b", field)) when scanning error.details, error.message, and
error.hint so only the exact phrase "0 rows" matches; apply the same change to
the equivalent sync implementation (the non-async
RequestBuilder/_is_zero_rows_error) and ensure the execute method still returns
None for true zero-row matches and re-raises other APIError instances
(AsyncSingleRequestBuilder stays used for the async path).
| @staticmethod | ||
| def _is_zero_rows_error(error: APIError) -> bool: | ||
| return any( | ||
| field is not None and "0 rows" in field | ||
| for field in (error.details, error.message, error.hint) | ||
| ) | ||
|
|
||
| def execute(self) -> Optional[SingleAPIResponse]: | ||
| r = None | ||
| try: | ||
| r = SyncSingleRequestBuilder(self.request).execute() | ||
| except APIError as e: | ||
| if e.details and "The result contains 0 rows" in e.details: | ||
| return SyncSingleRequestBuilder(self.request).execute() | ||
| except APIError as error: | ||
| if self._is_zero_rows_error(error): | ||
| return None | ||
| if not r: | ||
| raise APIError( | ||
| { | ||
| "message": "Missing response", | ||
| "code": "204", | ||
| "hint": "Please check traceback of the code", | ||
| "details": "Postgrest couldn't retrieve response, please check traceback of the code. Please create an issue in `supabase-community/postgrest-py` if needed.", | ||
| } | ||
| ) | ||
| return r | ||
| raise |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and understand the context
cd src/postgrest && find . -name "*.py" | grep -E "(request_builder|test)" | head -20Repository: supabase/supabase-py
Length of output: 761
🏁 Script executed:
# Look at the actual file mentioned in the review
cat -n src/postgrest/src/postgrest/_sync/request_builder.py | head -130Repository: supabase/supabase-py
Length of output: 4859
🏁 Script executed:
# Search for any tests related to _is_zero_rows_error
rg "_is_zero_rows_error|0 rows" --type py -B 2 -A 5Repository: supabase/supabase-py
Length of output: 4242
🏁 Script executed:
# Look for APIError definition and how errors are constructed
rg "class APIError" --type py -A 10Repository: supabase/supabase-py
Length of output: 1495
🌐 Web query:
PostgREST error message format "0 rows" constraint violation
💡 Result:
In PostgREST, constraint violations (unique/foreign key/not-null/check, etc.) are forwarded from PostgreSQL and returned as JSON with the PostgreSQL SQLSTATE in code, e.g.:
{
"code": "23505",
"details": "...",
"hint": null,
"message": "duplicate key value violates unique constraint ..."
}and PostgREST maps common constraint SQLSTATEs to HTTP statuses (e.g. 23505/23503 → 409 Conflict). [1]
The “0 rows” wording you’re seeing is usually not a constraint violation at all—it’s a PostgREST-generated error that happens when you request a singular JSON object (via Accept: application/vnd.pgrst.object+json) but the query affects/returns 0 rows (or more than 1). In that case PostgREST returns 406 Not Acceptable with details like:
{
"message": "JSON object requested, multiple (or no) rows returned",
"details": "Results contain 0 rows, application/vnd.pgrst.object+json requires 1 row",
"hint": null,
"code": "PGRST505"
}
``` [2]
So:
- If your error `code` is **23xxx** (e.g. 23505/23503/23502/23514), it’s a **real DB constraint violation**. [1]
- If your error mentions **“Results contain 0 rows … requires 1 row”** and has a **PGRST*** code, it’s a **singular-response mismatch / no rows matched** (often due to filters or RLS), not a constraint failure. [2]
**Sources**
[1] PostgREST “Errors” reference (PostgreSQL error JSON + HTTP mapping) https://docs.postgrest.org/en/v14/references/errors.html
[2] PostgREST “Resource Representation” (singular `vnd.pgrst.object`, 0 rows → 406 + PGRST505) https://postgrest.org/en/v11/references/api/resource_representation.html
---
</details>
**Use word-boundary matching to guard against false positives in error message detection.**
While the current PostgREST error format for zero-row cases is "The result contains 0 rows", the substring match `"0 rows" in field` could theoretically match unintended error messages if PostgREST generates messages containing "0 rows" in unexpected contexts. Switching to a word-boundary regex is a safer, more robust approach that explicitly matches only the standalone phrase "0 rows":
<details>
<summary>Proposed fix</summary>
```diff
+import re
@@
`@staticmethod`
def _is_zero_rows_error(error: APIError) -> bool:
return any(
- field is not None and "0 rows" in field
+ field is not None and re.search(r"\b0 rows\b", field)
for field in (error.details, error.message, error.hint)
)Note: The same pattern appears in the async version at src/postgrest/src/postgrest/_async/request_builder.py.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @staticmethod | |
| def _is_zero_rows_error(error: APIError) -> bool: | |
| return any( | |
| field is not None and "0 rows" in field | |
| for field in (error.details, error.message, error.hint) | |
| ) | |
| def execute(self) -> Optional[SingleAPIResponse]: | |
| r = None | |
| try: | |
| r = SyncSingleRequestBuilder(self.request).execute() | |
| except APIError as e: | |
| if e.details and "The result contains 0 rows" in e.details: | |
| return SyncSingleRequestBuilder(self.request).execute() | |
| except APIError as error: | |
| if self._is_zero_rows_error(error): | |
| return None | |
| if not r: | |
| raise APIError( | |
| { | |
| "message": "Missing response", | |
| "code": "204", | |
| "hint": "Please check traceback of the code", | |
| "details": "Postgrest couldn't retrieve response, please check traceback of the code. Please create an issue in `supabase-community/postgrest-py` if needed.", | |
| } | |
| ) | |
| return r | |
| raise | |
| import re | |
| `@staticmethod` | |
| def _is_zero_rows_error(error: APIError) -> bool: | |
| return any( | |
| field is not None and re.search(r"\b0 rows\b", field) | |
| for field in (error.details, error.message, error.hint) | |
| ) | |
| def execute(self) -> Optional[SingleAPIResponse]: | |
| try: | |
| return SyncSingleRequestBuilder(self.request).execute() | |
| except APIError as error: | |
| if self._is_zero_rows_error(error): | |
| return None | |
| raise |
🤖 Prompt for AI Agents
In `@src/postgrest/src/postgrest/_sync/request_builder.py` around lines 107 - 120,
The _is_zero_rows_error helper currently uses a substring check ("0 rows" in
field) which can produce false positives; update PostgREST error detection in
_is_zero_rows_error (used by execute and the SyncSingleRequestBuilder flow) to
use a word-boundary regular expression that matches the standalone phrase "0
rows" (e.g. r"\b0 rows\b") against APIError fields (error.details,
error.message, error.hint) and return True only on a regex match; apply the same
change to the async counterpart in request_builder.py for consistency.
|
Thank you for the PR. As mentioned in #1207 (comment), this is a breaking change, and as such must be instead done in the next major version (#1370). I'll begin working on rewriting |
2 participants
Summary
maybe_single()builders to returnNonewhen the API error clearly indicates zero rowsdetails,message, andhintfieldsAPIErrorfor non-zero-row failures (instead of replacing with a generic 204 error)Why
Issue #1207 reports
maybe_single()raising an error when zero rows are returned. The current implementation only checks one exact message location and can incorrectly produce a synthetic204 Missing responseerror.Testing
uv run --package postgrest pytest tests/_sync/test_client.py tests/_async/test_client.py(run insrc/postgrest)make postgrest.mypyuv run ruff check src/postgrest/src/postgrest/_sync/request_builder.py src/postgrest/src/postgrest/_async/request_builder.py src/postgrest/tests/_sync/test_client.py src/postgrest/tests/_async/test_client.pyCloses #1207
Summary by CodeRabbit
Refactor
Noneinstead of raising an exception, while preserving proper error propagation for other API errors.Tests