Skip to content

Return empty on no collection, fallback to route validation#10719

Merged
abnegate merged 2 commits into1.8.xfrom
fix-error-handler-error
Oct 28, 2025
Merged

Return empty on no collection, fallback to route validation#10719
abnegate merged 2 commits into1.8.xfrom
fix-error-handler-error

Conversation

@abnegate
Copy link
Copy Markdown
Member

@abnegate abnegate commented Oct 28, 2025

What does this PR do?

(Provide a description of what this PR does and why it's needed.)

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)

Related PRs and Issues

  • (Related PR or issue)

Checklist

  • Have you read the Contributing Guidelines on issues?
  • If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Oct 28, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

The getRelatedCollectionKeys method in src/Appwrite/Utopia/Request/Filters/V20.php was changed to broaden exception handling: instead of catching NotFound and rethrowing specific exceptions for missing database or collection cases, the code now catches any Throwable and returns an empty array. The NotFound import was removed. No public signatures were modified.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Change is limited to a single method in one file.
  • Key review focus:
    • Confirm callers expect an empty array instead of exceptions (behavioral contract).
    • Ensure broader Throwable catch does not suppress unexpected errors (consider logging).
    • Verify removal of the NotFound import is safe.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description Check ❓ Inconclusive The PR description provided is essentially a blank contributor template with all section placeholders unfilled. The "What does this PR do?", "Test Plan", and "Related PRs and Issues" sections contain only placeholder text with no actual content describing the changeset. This renders the description completely empty of substantive information, making it impossible to determine whether it meaningfully relates to the changes or provides any clarity about the motivation, testing, or implementation details of the modification.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "Return empty on no collection, fallback to route validation" directly aligns with the main change in the pull request. According to the raw_summary, the primary modification is in the getRelatedCollectionKeys function where exception handling was changed to return an empty array instead of throwing specific exceptions when a database or collection is not found. The title accurately captures this core behavior change and is concise and specific enough for a developer scanning the history to understand the primary intent.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-error-handler-error

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 28, 2025 •
edited
Loading

Security Scan Results for PR

Docker Image Scan Results

Package Version Vulnerability Severity
binutils 2.44-r2 CVE-2025-5244 HIGH
binutils 2.44-r2 CVE-2025-5245 HIGH
libxml2 2.13.8-r0 CVE-2025-49794 CRITICAL
libxml2 2.13.8-r0 CVE-2025-49796 CRITICAL
libxml2 2.13.8-r0 CVE-2025-49795 HIGH
libxml2 2.13.8-r0 CVE-2025-6021 HIGH
pcre2 10.43-r1 CVE-2025-58050 CRITICAL
golang.org/x/crypto v0.31.0 CVE-2025-22869 HIGH
golang.org/x/oauth2 v0.24.0 CVE-2025-22868 HIGH
stdlib 1.22.10 CVE-2025-47907 HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

coderabbitai[bot]
coderabbitai bot reviewed Oct 28, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/Appwrite/Utopia/Request/Filters/V20.php (1)

140-162: Consider the architectural implications of silent failures.

This backward compatibility filter's purpose is to preserve 1.7.x behavior by auto-adding relationship selects. Silently suppressing database/collection lookup failures defeats this purpose—the filter will return empty relationship keys and fail to provide backward compatibility, without any indication that something went wrong.

Consider these alternatives:

  1. Let exceptions propagate and handle them at a higher level where proper error responses can be generated.

  2. Add fallback behavior with proper logging:

    • Log when databases/collections can't be found
    • Include metrics/monitoring for silent failure rates
    • Add request tracing to understand impact

  3. Document the decision: If silent failures are intentional (per the PR title "fallback to route validation"), add comments explaining:

    • Why errors are suppressed here
    • How route validation will catch these cases
    • What the expected user experience is

Can you clarify the intended behavior when database or collection lookups fail? Should clients receive:

  • An empty response with 200 status?
  • A validation error from route handlers?
  • A specific error message?
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a5a603 and 15cabd0.

📒 Files selected for processing (1)
  • src/Appwrite/Utopia/Request/Filters/V20.php (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
  • GitHub Check: E2E Service Test (VCS)
  • GitHub Check: E2E Service Test (Users)
  • GitHub Check: E2E Service Test (Health)
  • GitHub Check: E2E Service Test (Webhooks)
  • GitHub Check: E2E Service Test (Proxy)
  • GitHub Check: E2E Service Test (Storage)
  • GitHub Check: E2E Service Test (Teams)
  • GitHub Check: E2E Service Test (Sites)
  • GitHub Check: E2E Service Test (Account)
  • GitHub Check: E2E Service Test (Functions)
  • GitHub Check: E2E Service Test (Databases/TablesDB)
  • GitHub Check: E2E Service Test (Locale)
  • GitHub Check: E2E Service Test (Realtime)
  • GitHub Check: E2E Service Test (Avatars)
  • GitHub Check: E2E Service Test (Site Screenshots)
  • GitHub Check: E2E Service Test (Dev Keys)
  • GitHub Check: E2E General Test
  • GitHub Check: Unit Test
  • GitHub Check: Benchmark
  • GitHub Check: scan

@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 28, 2025

✨ Benchmark results

  • Requests per second: 1,094
  • Requests with 200 status code: 197,014
  • P99 latency: 0.176722695

⚡ Benchmark Comparison

Metric This PR Latest version
RPS 1,094 1,115
200 197,014 200,831
P99 0.176722695 0.184329692

@abnegate abnegate merged commit b9fb10a into 1.8.x Oct 28, 2025
41 checks passed
@abnegate abnegate deleted the fix-error-handler-error branch October 28, 2025 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@abnegate