Conversation
Signed-off-by: gnishant <106644073+gnishant-gupta@users.noreply.github.com>
Signed-off-by: gnishant <106644073+gnishant-gupta@users.noreply.github.com>
…arketplace into tipcommon_gitsync_1p
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive updates to the TIPCommon package to improve its GitSync capabilities and overall platform interaction. The changes include the addition of several new data models to represent various platform entities, a significant expansion of the API client functionality to support a wider range of SOAR operations, and various robustness improvements such as better error handling and URL normalization. These updates ensure that the CLI tool is better equipped to handle complex synchronization tasks and platform configurations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the TIPCommon package to version 2.3.6, significantly expanding the data models and REST API wrappers to support both legacy and One Platform (1P) SOAR environments. Key changes include the addition of numerous dataclasses for data normalization and a wide array of new API methods for managing integrations, cases, and system settings. Feedback identifies several typos in function names, a logic error in the bulk tag addition method, and opportunities to improve robustness by catching specific exceptions and using safe dictionary access. Additionally, suggestions were provided to enhance type safety and adhere to library logging best practices.
| """Add tags to case in bulk""" | ||
| endpoint = "/cases:executeBulkAddTag" | ||
| payload = { | ||
| "propertiesStatus": { | ||
| "additionalProp1": 0, | ||
| "additionalProp2": 0, | ||
| "additionalProp3": 0, | ||
| }, | ||
| "displayName": self.params.name, | ||
| "parameterType": self.params.type, | ||
| "defaultValue": self.params.default_value, | ||
| "optionalValuesJson": str(self.params.optional_json), | ||
| } | ||
| return self._make_request(HttpMethod.PATCH, endpoint, json_payload=payload) |
There was a problem hiding this comment.
This function appears to have a copy-paste error.
- The function name
add_tags_to_case_in_bulkshas a typo and should likely beadd_tags_to_case_in_bulk. - The payload being constructed uses parameters (
self.params.name,self.params.type, etc.) that seem to belong to adding a dynamic environment parameter, not adding tags to cases. This will cause the API call to fail or have unintended consequences.
Please correct the payload to match the requirements for the /cases:executeBulkAddTag endpoint.
| environments: list[str] | ||
|
|
||
| @classmethod | ||
| def from_legacy_or_1p(cls, data: SingleJson) -> "Entity": |
There was a problem hiding this comment.
The return type hint for this class method is "Entity", but the method is part of the BlockRecord class and returns an instance of it. This is misleading and can cause issues with static analysis tools. The return type should be "BlockRecord".
| def from_legacy_or_1p(cls, data: SingleJson) -> "Entity": | |
| def from_legacy_or_1p(cls, data: SingleJson) -> "BlockRecord": |
| except Exception: | ||
| envs = [] |
There was a problem hiding this comment.
Using a broad except Exception: can hide bugs. It's better to catch specific exceptions you anticipate, such as json.JSONDecodeError or TypeError when parsing JSON.
| except Exception: | |
| envs = [] | |
| except (json.JSONDecodeError, TypeError): | |
| envs = [] |
| return response.json() | ||
|
|
||
|
|
||
| def attache_workflow_to_case( |
There was a problem hiding this comment.
There's a typo in the function name. It should be attach_workflow_to_case instead of attache_workflow_to_case.
| def attache_workflow_to_case( | |
| def attach_workflow_to_case( |
| except Exception: | ||
| scopes = [] |
There was a problem hiding this comment.
The use of a broad except Exception: clause can mask unexpected errors and make debugging more difficult. It's better to catch more specific exceptions that you expect to handle.
| except Exception: | |
| scopes = [] | |
| except (json.JSONDecodeError, TypeError): | |
| scopes = [] |
| endpoint = ( | ||
| "/system/settings/environments" | ||
| ) | ||
| return self._make_request(HttpMethod.GET, endpoint).json()["environments"] |
There was a problem hiding this comment.
Directly accessing a key with ...json()["environments"] is unsafe and will raise a KeyError if the key is not present in the response. It's safer to use the .get() method with a default value.
| return self._make_request(HttpMethod.GET, endpoint).json()["environments"] | |
| return self._make_request(HttpMethod.GET, endpoint).json().get("environments", []) |
| except Exception: | ||
| return [] |
There was a problem hiding this comment.
Using a broad except Exception: can hide bugs. It's better to catch specific exceptions you anticipate, such as json.JSONDecodeError if the response body is not valid JSON.
| except Exception: | |
| return [] | |
| except json.JSONDecodeError: | |
| return [] |
| data["params"] = data["parameters"] | ||
| detailed_data_list.append(data) | ||
| else: | ||
| print(f"Failed to fetch details for {name}") |
There was a problem hiding this comment.
Using print() for logging in a library is not recommended as it writes directly to stdout and can't be controlled by the application's logging configuration. Please use the provided logger instance instead.
| print(f"Failed to fetch details for {name}") | |
| self.chronicle_soar.LOGGER.warning(f"Failed to fetch details for {name}") |
| payload = self.params.logo_data | ||
| return self._make_request(HttpMethod.PATCH, endpoint, json_payload=payload) | ||
|
|
||
| def attache_workflow_to_case(self) -> requests.Response: |
There was a problem hiding this comment.
There's a typo in the function name. It should be attach_workflow_to_case instead of attache_workflow_to_case.
| def attache_workflow_to_case(self) -> requests.Response: | |
| def attach_workflow_to_case(self) -> requests.Response: |
| def get_installed_integrations(self) -> requests.Response: | ||
| """Get installed integrations.""" | ||
| endpoint: str = "/integrations" | ||
| return self._make_request(HttpMethod.GET, endpoint).json()["integrations"] |
There was a problem hiding this comment.
Directly accessing a key with ...json()["integrations"] is unsafe and will raise a KeyError if the key is not present in the response. It's safer to use the .get() method with a default value.
| return self._make_request(HttpMethod.GET, endpoint).json()["integrations"] | |
| return self._make_request(HttpMethod.GET, endpoint).json().get("integrations", []) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8fc5a6e. Configure here.
| response = api_client.update_blocklist() | ||
| try: | ||
| response = validate_response(response, validate_json=False) | ||
| return response |
There was a problem hiding this comment.
validate_response returns None, overwriting response object
High Severity
In update_blocklist, the result of validate_response() is assigned back to response, but validate_response returns None. This means response is overwritten with None and then returned, so update_blocklist always returns None on success instead of the actual response data.
Reviewed by Cursor Bugbot for commit 8fc5a6e. Configure here.
| ) -> SingleJson: | ||
| """Add or update company logo.""" | ||
| api_client = get_soar_client(chronicle_soar) | ||
| api_client.params.company_logo = company_logo |
There was a problem hiding this comment.
Attribute name mismatch: company_logo vs logo_data
High Severity
add_or_update_company_logo sets api_client.params.company_logo, but both LegacySoarApi and OnePlatformSoarApi implementations read self.params.logo_data. This mismatch means the payload will be None (or trigger an AttributeError depending on the Container implementation), so the logo data is never actually sent to the API.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 8fc5a6e. Configure here.
| def get_all_model_block_records(self) -> requests.Response: | ||
| """Get all model block records.""" | ||
| endpoint: str = "settings/GetAllModelBlockRecords" | ||
| return self.get_page_results(endpoint) |
There was a problem hiding this comment.
Missing leading slash in legacy API endpoints
High Severity
The endpoints "settings/GetAllModelBlockRecords" and "settings/GetCompanyLogo" are missing a leading /. Since _make_request constructs URLs via string concatenation (f"{base_uri}{endpoint}"), these will produce malformed URLs like https://host/external/v1settings/... instead of https://host/external/v1/settings/..., causing all requests to these endpoints to fail.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8fc5a6e. Configure here.
| except (ValueError, InternalJSONDecoderError): | ||
| return [] | ||
|
|
||
| return [] |
There was a problem hiding this comment.
get_sla_records dead code after .json() call
Low Severity
In get_sla_records, response is already the parsed JSON result (from .json() at line 2429), so the hasattr(response, "json") check at line 2441 is dead code — a parsed dict/list will never have a .json() method. More importantly, if .json() raises a JSONDecodeError, the outer try/except catches ValueError but not JSONDecodeError directly from the requests library, which may not be an InternalJSONDecoderError.
Reviewed by Cursor Bugbot for commit 8fc5a6e. Configure here.
2 participants
QA Fixes for GitSync in TIPCommon
Examples:
Fix: Resolve issue with API endpoint returning 500 error[Buganizer ID: 987654321] Feature: Add support for custom data typesDocs: Update README with installation instructionsDescription
QA Fixes for GitSync in TIPCommon
What problem does this PR solve?
(e.g., "Fixes a bug where X was happening," "Implements feature Y to allow Z," "Improves performance of function A.")
How does this PR solve the problem?
(e.g., "Modified algorithm in
src/foo.js," "Added new componentBar.vue," "Updated dependencybazto version 1.2.3.")Any other relevant information (e.g., design choices, tradeoffs, known issues):
(e.g., "Chose approach A over B due to performance considerations," "This change might affect X in certain edge cases," "Requires manual migration steps for existing users.")
Checklist:
Please ensure you have completed the following items before submitting your PR.
This helps us review your contribution faster and more efficiently.
General Checks:
Open-Source Specific Checks:
For Google Team Members and Reviewers Only:
Screenshots (If Applicable)
If your changes involve UI or visual elements, please include screenshots or GIFs here.
Ensure any sensitive data is redacted or generalized.
Further Comments / Questions
Any additional comments, questions, or areas where you'd like specific feedback.
Note
Medium Risk
Adds a large set of new Chronicle SOAR API wrappers and corresponding data models, plus request URL rewriting for 1P download endpoints; breadth of new endpoints and response-shape normalization increases integration/compat risk despite mostly additive changes.
Overview
Bumps
TIPCommonversion to2.3.6and significantly expands the SOAR REST surface with many new helper functions inrest/soar_api.pyfor managing templates, environments, integrations/packages, domains/networks/custom lists, blocklists/SLA, playbooks, IDE items, mapping rules, and simulated cases.Adds multiple new/updated data models in
data_models.py(e.g.,Environment,IntegrationSetting,VisualFamily,OntologyRecord,CaseTag,Domain,Network,SlaDefinition,SimulatedCases) and makes existing parsers more defensive (e.g.,InstalledIntegrationInstance,DynamicParameter).Extends both legacy and 1P SOAR platform clients with the corresponding endpoints, includes several QA fix parameter/endpoint adjustments, and updates
BaseSoarApi._make_requestto rewrite certain/v1alpha/.../download/integrations/...URLs to the expected download path.Reviewed by Cursor Bugbot for commit 8fc5a6e. Bugbot is set up for automated code reviews on this repo. Configure here.