fix(sdk-python): avoid empty JSON body on GET#1606
Open
mahendrarathore1742 wants to merge 2 commits intolangfuse:mainfrom
Open
fix(sdk-python): avoid empty JSON body on GET#1606mahendrarathore1742 wants to merge 2 commits intolangfuse:mainfrom
mahendrarathore1742 wants to merge 2 commits intolangfuse:mainfrom
Conversation
Return None when no additional body parameters are provided so GET requests don't include {} payloads. Adds unit tests for request body handling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes an issue where the SDK would send an empty JSON body ({}) with GET requests, which violates HTTP specs and is rejected by some load balancers (e.g., Google Cloud). Now, if no additional body parameters are provided, the request body is set to None for GET requests.
Fix: Avoid sending {} as the body for GET requests.
Adds unit tests to ensure correct request body behavior.
Disclaimer: Experimental PR review
Greptile Summary
This PR fixes a bug where GET requests could send an empty JSON body (
{}) that is rejected by some load balancers (e.g., Google Cloud). The fix refactorsmaybe_filter_request_bodyto returnNonedirectly when noadditional_body_parametersare present, rather than returning{}and relying on a downstreamjson_body != {}guard inget_request_bodyto convert it toNone.Key changes:
maybe_filter_request_bodynow short-circuits toNonewhenrequest_optionsisNoneor whenadditional_body_parametersis absent/empty, making the intent explicit.!= {}safety check inget_request_body(lines 204–206) is retained and still useful for the non-Nonedatamapping path, which is unchanged.request_options=Nonewould complete coverage of the new early-return path.Confidence Score: 5/5
request_options=None). The core logic change is sound:maybe_filter_request_bodypreviously returned{}for the no-body case which was already neutralised by the!= {}check downstream, and the new code simply makes thatNonereturn explicit. No regressions are introduced.tests/test_http_client.py.Important Files Changed
maybe_filter_request_bodyto returnNonedirectly whenadditional_body_parametersis absent/empty, replacing the indirect{} → Noneconversion that relied on a downstream!= {}check inget_request_body. Logic is correct and cleaner.request_options=Nonetest for completeness, but all existing assertions are correct.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[get_request_body called] --> B{data is not None?} B -- Yes --> C[maybe_filter_request_body\nwith data] B -- No --> D[maybe_filter_request_body\nwith json] D --> E{request_options\nis None?} E -- Yes --> F[return None\nNEW short-circuit] E -- No --> G{additional_body_parameters\nfalsy or missing?} G -- Yes --> H[return None\nNEW short-circuit] G -- No --> I[return jsonable_encoder\nof params] C --> J[data_content merged dict] I --> K[json_body set] F --> K H --> K J --> L[data_body set] K --> M{json_body is empty dict?} M -- Yes --> N[return None\nsafety net] M -- No --> O[return json_body] L --> P{data_body is empty dict?} P -- Yes --> Q[return None\nsafety net] P -- No --> R[return data_body] style F fill:#90EE90 style H fill:#90EE90Reviews (1): Last reviewed commit: "fix(sdk-python): avoid empty JSON body o..." | Re-trigger Greptile
(2/5) Greptile learns from your feedback when you react with thumbs up/down!