Skip to content
This repository was archived by the owner on Sep 8, 2025. It is now read-only.

Conversation

silentworks
Copy link
Contributor

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

.range doesn't work with .rpc calls as these are POST requests and range only works with GET requests

What is the new behavior?

.range now works with .rpc calls

Additional context

Add any other context or screenshots.

@silentworks silentworks requested review from anand2312 and mansueli and removed request for J0, anand2312 and olirice February 27, 2024 02:22
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Enhancement

PR Summary: This PR introduces an enhancement to the .range method in the PostgREST client, allowing it to work with .rpc calls by using query parameters instead of headers. This change enables the .range method to be compatible with both GET and POST requests, addressing the issue where .range was previously not functional with .rpc calls.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.

General suggestions:

  • Ensure comprehensive testing across different scenarios to validate the new behavior of the .range method, especially in edge cases where foreign_table is involved.
  • Consider documenting this change prominently in the project's change log or migration guide, as it alters the fundamental behavior of the .range method, which could impact existing users.
  • Review the impact of this change on the overall performance of the PostgREST client, particularly in scenarios involving large datasets and complex queries.
postgrest/base_request_builder.py: Refactor to separate concerns and simplify understanding by distinctly handling `foreign_table` logic and default behavior.

While the added functionality to support foreign_table is appreciated, the changes introduce a higher level of complexity and mix concerns by altering both HTTP headers and query parameters based on the presence of an optional argument. This increases the cognitive load required to understand the method's behavior and its impact on the system. Here's a suggested refactor that maintains the simplicity of the original method while incorporating the new functionality:

def range(self: Self, start: int, end: int, foreign_table: Optional[str] = None) -> Self:
    if foreign_table:
        # If foreign_table is specified, modify query parameters.
        offset_key = f"{foreign_table}.offset"
        limit_key = f"{foreign_table}.limit"
        self.params = self.params.add(offset_key, start)
        self.params = self.params.add(limit_key, end - start + 1)
    else:
        # Otherwise, set the headers as before.
        self.headers["Range-Unit"] = "items"
        self.headers["Range"] = f"{start}-{end - 1}"
    return self

This approach clearly separates the handling of foreign_table from the default behavior, making the method easier to understand and maintain. It also adheres more closely to the Single Responsibility Principle by keeping the method focused on a single concern at a time.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@codecov
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.57%. Comparing base (0faa8c3) to head (fff2284).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
+ Coverage   95.31%   95.57%   +0.25%     
==========================================
  Files          28       28              
  Lines        1580     1604      +24     
==========================================
+ Hits         1506     1533      +27     
+ Misses         74       71       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mansueli mansueli left a comment

Choose a reason for hiding this comment

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

LGTM

@silentworks silentworks merged commit eae612c into master Feb 27, 2024
@silentworks silentworks deleted the silentworks/update-range-function branch February 27, 2024 11:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants