-
-
Notifications
You must be signed in to change notification settings - Fork 461
fix(postgrest): handle maybe_single zero-row responses correctly #1384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -104,23 +104,20 @@ class SyncMaybeSingleRequestBuilder: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __init__(self, request: ReqConfig): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.request = request | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+107
to
+120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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:
💡 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": "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 {
"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 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class SyncFilterRequestBuilder( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 1034
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 734
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 1231
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 3594
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 566
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 2326
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 14020
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 4805
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 5613
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 7075
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 606
🏁 Script executed:
Repository: supabase/supabase-py
Length of output: 545
🏁 Script executed:
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 fieldincorrectly matches "The result contains 10 rows" or "20 rows", causing multi-row errors to returnNoneinstead of raising. Use word boundaries withre.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
🤖 Prompt for AI Agents