Onyphe: Validate entrypoint argument via Pydantic#2886
Conversation
Reviewer's GuideAdds early validation guards to OnypheDatascanAction.run() to handle missing or empty string/ip arguments without making HTTP requests, updates versioning and changelog, and extends tests to cover the new behavior. Sequence diagram for updated OnypheDatascanAction.run argument guardssequenceDiagram
actor Client
participant OnypheDatascanAction
participant get_arg_ip
participant urljoin
Client->>OnypheDatascanAction: run(arguments)
alt no_ip_and_no_string
OnypheDatascanAction->>OnypheDatascanAction: error
OnypheDatascanAction-->>Client: None
else both_ip_and_string
OnypheDatascanAction-->>Client: TypeError
else ip_only
OnypheDatascanAction->>get_arg_ip: get_arg_ip(arguments)
get_arg_ip-->>OnypheDatascanAction: resource
OnypheDatascanAction->>urljoin: urljoin(url, "datascan/" + resource)
urljoin-->>OnypheDatascanAction: get_url
OnypheDatascanAction-->>Client: result
else string_only
OnypheDatascanAction->>OnypheDatascanAction: resource = arguments.get("string")
alt empty_string
OnypheDatascanAction->>OnypheDatascanAction: error
OnypheDatascanAction-->>Client: None
else non_empty_string
OnypheDatascanAction->>urljoin: urljoin(url, "datascan/" + resource)
urljoin-->>OnypheDatascanAction: get_url
OnypheDatascanAction-->>Client: result
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
runmethod is now returningNonein some branches but still annotated as-> dict; consider updating the type hint (e.g., toOptional[dict]) to reflect the new behavior and keep static analysis accurate. - The argument validation logic in
runnow has multiple separate checks for missing keys and empty string values; consider consolidating these into a single, clearly ordered validation block to reduce duplication and make the control flow easier to follow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `run` method is now returning `None` in some branches but still annotated as `-> dict`; consider updating the type hint (e.g., to `Optional[dict]`) to reflect the new behavior and keep static analysis accurate.
- The argument validation logic in `run` now has multiple separate checks for missing keys and empty string values; consider consolidating these into a single, clearly ordered validation block to reduce duplication and make the control flow easier to follow.
## Individual Comments
### Comment 1
<location path="Onyphe/onyphe/action_onyphe_datascan.py" line_range="26-30" />
<code_context>
def run(self, arguments) -> dict:
url: str = "https://www.onyphe.io/api/v2/simple/"
+ if "ip" not in arguments and "string" not in arguments:
+ self.error("IP or string is required")
+ return None
+
if ("ip" in arguments) == ("string" in arguments):
</code_context>
<issue_to_address>
**issue (bug_risk):** run() can now return None despite the declared dict return type
The new `return None` paths (when neither argument is provided or when `string` is falsy) break the `-> dict` contract and may surprise callers that expect a dict. Please either return a dict-shaped error object, update the annotation to `Optional[dict]`, or raise an exception instead of returning `None` so downstream code can handle failures predictably.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
aa48877 to
9883819
Compare
|
Updated this PR to the native Pydantic v2 pattern used in the related rollout.
|
|
Updated the datascan argument model while intentionally leaving the existing cross-field XOR validator untouched. The optional ip field is now validated as IPvAnyAddress | None, with whitespace trimming and rejection of non-string inputs preserved. The string field now uses a non-empty constrained string, and the action stringifies the validated IP when building the request URL. Added coverage for an invalid-shape IP and the IP-only success path. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ed9cfce to
a869106
Compare
What
Validate Onyphe's
datascanaction argument via a Pydantic v2 model before the action runs, so a missing/blank/malformed IP or search string fails immediately with a clear validation error instead of calling the API with bad data.Changes
ip→IPvAnyAddress | Nonestringkept asNonEmptyStr | Noneip/stringmust be set) is preserved unchangedTesting
pytestandblackpass.