Conversation
Signed-off-by: yuluo-yx <yuluo08290126@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements concrete isInvalid() validation logic for multiple protocol entities in hertzbeat-common-core, and adds JUnit tests to verify the new validation behavior.
Changes:
- Implemented
isInvalid()checks for several protocol configs (SNMP/SMTP/IMAP/FTP/NTP/NGQL/Script/Eureka SD/Zookeeper SD). - Added unit tests covering valid/invalid protocol parameter combinations for each updated protocol type.
- Standardized parts of validation using shared utilities (e.g.,
IpDomainUtil,StringUtils).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/ZookeeperSdProtocol.java | Implements Zookeeper SD protocol validation (blank/whitespace/path prefix format). |
| hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/SnmpProtocol.java | Adds SNMP validation for host/port/version, v1/v2c/v3 requirements, operation + OIDs checks. |
| hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/SmtpProtocol.java | Adds SMTP validation for host/port/email and optional numeric timeout. |
| hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/ScriptProtocol.java | Adds script validation for charset/parseType/tool and presence of command/path. |
| hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/NtpProtocol.java | Adds NTP validation for host, optional port, optional numeric timeout. |
| hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/NgqlProtocol.java | Adds NGQL validation for connection fields, parseType allowlist, commands list. |
| hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/ImapProtocol.java | Adds IMAP validation for host/port/timeout/email/authorize/folder + ssl value. |
| hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/FtpProtocol.java | Adds FTP/SFTP validation for host/port/direction/timeout + ssl value + creds when ssl=true. |
| hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/EurekaSdProtocol.java | Adds Eureka SD URL validation using URI parsing + scheme restriction. |
| hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/ZookeeperSdProtocolTest.java | Adds tests for Zookeeper SD validity rules. |
| hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/SnmpProtocolTest.java | Adds tests for SNMP version/credential/operation/OID validation. |
| hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/SmtpProtocolTest.java | Adds tests for SMTP host/port/email/timeout validation. |
| hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/ScriptProtocolTest.java | Adds tests for script charset/parseType/tool and command/path requirements. |
| hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/NtpProtocolTest.java | Adds tests for NTP host/port/timeout validation. |
| hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/NgqlProtocolTest.java | Adds tests for NGQL parseType/commands/timeout validation. |
| hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/ImapProtocolTest.java | Adds tests for IMAP ssl/folder/timeout/host validation. |
| hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/FtpProtocolTest.java | Adds tests for FTP/SFTP ssl, direction, timeout, and credential requirements. |
| hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/entity/job/protocol/EurekaSdProtocolTest.java | Adds tests for Eureka SD URL validation (scheme/blank/malformed). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!validateIpDomain(host) || !validPort(port) || StringUtils.isBlank(direction) || StringUtils.isBlank(timeout)) { | ||
| return true; | ||
| } | ||
| if (!CommonUtil.isNumeric(timeout)) { |
There was a problem hiding this comment.
CommonUtil.isNumeric(timeout) accepts decimal/signed values (e.g., "3000.5"), but the FTP collector later uses Integer.parseInt(timeout) (e.g., in FtpCollectImpl) which would throw at runtime. Consider validating timeout as an integer (e.g., parse with Integer.parseInt in a try/catch, or use a stricter integer-only check) to keep isInvalid() consistent with downstream parsing.
Suggested change
| if (!CommonUtil.isNumeric(timeout)) { | |
| try { | |
| Integer.parseInt(timeout); | |
| } catch (NumberFormatException ex) { |
| if (StringUtils.isAnyBlank(timeout, email, authorize, folderName)) { | ||
| return true; | ||
| } | ||
| if (!CommonUtil.isNumeric(timeout)) { |
There was a problem hiding this comment.
CommonUtil.isNumeric(timeout) treats decimals as numeric, but ImapCollectImpl later does Integer.parseInt(imapProtocol.getTimeout()), which will fail for values like "6000.5". Tighten the timeout validation here to ensure it matches the collector’s integer parsing expectations (integer-only parse/check).
Suggested change
| if (!CommonUtil.isNumeric(timeout)) { | |
| if (!StringUtils.isNumeric(timeout)) { |
...-common-core/src/main/java/org/apache/hertzbeat/common/entity/job/protocol/NgqlProtocol.java
Show resolved
Hide resolved
tomsun28
approved these changes
Apr 1, 2026
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.
No description provided.