-
Notifications
You must be signed in to change notification settings - Fork 88
Add validation to check uniqeness of actions name and display name gr111 gr112 #5093
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Dan Tavori <[email protected]>
|
Changelog(s) in markdown:
|
…eck-uniqe-action-name
|
Changelog(s) in markdown:
|
RosenbergYehuda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job!
Approved, but please see my comments.
| class AgentixAction(TestSuiteBase): | ||
| def __init__(self, tmpdir: Path, name: str, repo): | ||
| # Save entities | ||
| self.object_id = name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why three attributes pointing to the same value??
| self.repo_path = repo.path | ||
| self.path = str(tmpdir) | ||
| super().__init__(tmp_path=tmpdir / f"{self.name}.yml", repo_path=str(repo.path)) | ||
| self.path = tmpdir / f"{self.name}.yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this, now its a string, not a path...think about it.
| ] | ||
|
|
||
|
|
||
| def validate_multiple_agentix_actions_with_same_display_name( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a detailed docstring, similar to other functions
| ] | ||
|
|
||
|
|
||
| def validate_multiple_agentix_actions_with_same_name( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here to, please add a dockstring
| @@ -1,3 +1,4 @@ | |||
| #empty | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#empty??
Please add unit tests
| query += """ | ||
| AND elementId(a) <> elementId(b) | ||
| RETURN a.object_id AS a_object_id, collect(b.object_id) AS b_object_ids | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both functions do the same thing, consider using 1 helper function, something like:
| """ | |
| def _validate_duplicate_agentix_action_field( | |
| tx: Transaction, | |
| file_paths: List[str], | |
| field_name: str | |
| ) -> List[Tuple[str, List[str]]]: | |
| """Generic validator for duplicate Agentix Action fields. | |
| Args: | |
| tx: The Neo4j transaction object. | |
| file_paths: List of file paths to filter results. | |
| field_name: The field to check for duplicates ('name' or 'display'). | |
| Returns: | |
| List of tuples (action_id, list_of_duplicate_ids). | |
| """ | |
| query = f"""// Returns Agentix Actions with duplicate {field_name} | |
| MATCH (a:{ContentType.AGENTIX_ACTION}), (b:{ContentType.AGENTIX_ACTION}) | |
| WHERE a.{field_name} = b.{field_name} | |
| """ | |
| if file_paths: | |
| query += f"AND a.path in {file_paths}" | |
| query += """ | |
| AND elementId(a) <> elementId(b) | |
| RETURN a.object_id AS a_object_id, collect(b.object_id) AS b_object_ids | |
| """ | |
| return [ | |
| (item.get("a_object_id"), item.get("b_object_ids")) | |
| for item in run_query(tx, query) | |
| ] | |
| def validate_multiple_agentix_actions_with_same_display_name( | |
| tx: Transaction, file_paths: List[str] | |
| ) -> List[Tuple[str, List[str]]]: | |
| """Query graph to return Agentix Actions with duplicate display names.""" | |
| return _validate_duplicate_agentix_action_field(tx, file_paths, "display") | |
| def validate_multiple_agentix_actions_with_same_name( | |
| tx: Transaction, file_paths: List[str] | |
| ) -> List[Tuple[str, List[str]]]: | |
| """Query graph to return Agentix Actions with duplicate names.""" | |
| return _validate_duplicate_agentix_action_field(tx, file_paths, "name") |
| self.path = str(tmpdir) | ||
| super().__init__(tmp_path=tmpdir / f"{self.name}.yml", repo_path=str(repo.path)) | ||
| self.path = tmpdir / f"{self.name}.yml" | ||
| self.yaml = YAML(tmp_path=self.path, repo_path=str(repo.path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure what you you are doing here, it's for unit tests so i'm not concern, just check if it can be done better.
Related Issues
fixes: https://jira-dc.paloaltonetworks.com/browse/CRTX-201484
Description