Skip to content
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

Add execute_request method #881

Merged
merged 5 commits into from
Nov 21, 2024
Merged

Add execute_request method #881

merged 5 commits into from
Nov 21, 2024

Conversation

angrybayblade
Copy link
Collaborator

@angrybayblade angrybayblade commented Nov 21, 2024

Important

Adds execute_request method to ComposioToolSet for executing HTTP requests with specified parameters.

  • New Method:
    • Adds execute_request method to ComposioToolSet in toolset.py.
    • Supports executing HTTP requests with endpoint, method, body, parameters, and either connection_id or app.
    • Raises ComposioSDKError if neither connection_id nor app is provided.
  • Integration:
    • Utilizes request method from collections.py to perform HTTP requests.

This description was created by Ellipsis for 2a89e0c. It will automatically update as commits are pushed.

Copy link

vercel bot commented Nov 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 21, 2024 10:51am

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 972e46d in 54 seconds

More details
  • Looked at 101 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/composio/tools/toolset.py:820
  • Draft comment:
    Use self.client.get_entity(id=self.entity_id) instead of self.get_entity(id=self.entity_id) to access the client method correctly.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests a change that might be related to how the entity is accessed. However, without knowing the implementation details of get_entity, it's difficult to assess the correctness of the comment. The comment does not provide strong evidence or reasoning for the suggested change, making it speculative.
    I might be missing the context of how get_entity is implemented and whether self.client.get_entity is indeed the correct method to use. The comment lacks evidence or explanation, making it speculative.
    Given the lack of evidence and explanation in the comment, and the rules against speculative comments, it seems prudent to delete the comment unless more context is provided.
    Delete the comment as it lacks strong evidence or explanation for the suggested change, making it speculative.

Workflow ID: wflow_KQ0ogVFYWua1Ndxj


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

github-actions bot commented Nov 21, 2024

This comment was generated by github-actions[bot]!

JS SDK Coverage Report

📊 Coverage report for JS SDK can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/coverage-11951648273/coverage/index.html

📁 Test report folder can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/html-report-11951648273/html-report/report.html

json={
"connectedAccountId": connection_id,
"body": body,
"method": method.upper(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding method validation to ensure only valid HTTP methods are used:

valid_methods = {'GET', 'POST', 'PUT', 'DELETE', 'PATCH'}
if method.upper() not in valid_methods:
    raise ValueError(f"Invalid HTTP method: {method}. Must be one of {valid_methods}")

"Please provide connection id or app name to execute a request"
)

return self.client.actions.request(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding logging similar to the execute_action method above:

self.logger.info(f"Executing request to {endpoint} with method={method}, connection_id={connection_id}")
response = self.client.actions.request(...)
self.logger.info(f"Got response from {endpoint}")
return response

@shreysingla11
Copy link
Collaborator

Code Review Summary

The code changes look generally good with well-structured methods and proper type hints. Here's a summary of the review:

Strengths:

✅ Good use of type hints and overloads
✅ Clean parameter handling and transformation
✅ Proper error handling for missing connection_id
✅ Good integration with existing connection management

Areas for Improvement:

  1. Documentation

    • Both new methods need docstrings
    • Parameter validation and error cases should be documented
  2. Validation & Safety

    • Add HTTP method validation
    • Consider adding endpoint URL validation
    • Consider adding request size limits
  3. Observability

    • Add logging similar to other methods for consistency
  4. Minor Enhancements

    • Consider adding request timeout handling
    • Add response validation for non-200 status codes

Overall Rating: 7/10 - Solid implementation that needs some documentation and validation improvements.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 2a89e0c in 18 seconds

More details
  • Looked at 44 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/composio/tools/toolset.py:851
  • Draft comment:
    Use logger's built-in formatting instead of f-string for logging.
        self.logger.info("Got response=%s", response)
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The method execute_request in ComposioToolSet logs the response using f-string formatting, which is not ideal for logging. It should use the logger's built-in formatting capabilities to avoid unnecessary string interpolation when the log level is not enabled.

Workflow ID: wflow_lqvctoLCpew42rrj


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@tushar-composio tushar-composio enabled auto-merge (squash) November 21, 2024 10:47
@tushar-composio tushar-composio merged commit d542050 into master Nov 21, 2024
9 checks passed
@tushar-composio tushar-composio deleted the feat/execute-request branch November 21, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants