Duansg opened a new pull request, #4366:
URL: https://github.com/apache/hertzbeat/pull/4366
## What's changed?
`RegistryProtocol.isInvalid()` must return `true` when params are unusable —
`RegistryImpl.preCheck()` throws on `true`. But the host and port guards were
written positively and joined with `&&`:
```java
return validateIpDomain(host) && validPort(port)
&& StringUtils.isAnyBlank(host, String.valueOf(port),
discoveryClientTypeName);
```
`&&` short-circuits, so an invalid host makes `validateIpDomain()` return
`false` and the expression collapse to `false` — reported **valid**. Same for
an invalid port. The trailing `isAnyBlank()` can never fire on host or port
either, since anything passing `validateIpDomain()` / `validPort()` is
non-blank by construction.
| `host` | `port` | before | expected |
|---|---|---|---|
| null / blank / malformed | any | **accepted** ❌ | rejected |
| valid | null / blank / non-numeric / >65535 | **accepted** ❌ | rejected |
| valid | valid, `discoveryClientTypeName` blank | rejected ✅ | rejected |
**This is a regression.** Before #4048 the body was a plain `isAnyBlank()`
check that *did* reject a blank host or port; #4048 meant to add IP/port
validation on top, but placing the new guards as a short-circuiting prefix
disabled the existing check as well.
The guard is not redundant with `MonitorServiceImpl.validate()` — two paths
bypass it: service-discovery monitors skip host validation entirely (`if
(!isStatic && "host".equals(field)) continue;`), and an empty `paramValue`
passes the `required` check (not null) and then skips the validator.
### Fix
```java
return !validateIpDomain(host) || !validPort(port)
|| StringUtils.isBlank(discoveryClientTypeName);
```
Same shape as the sibling `UdpProtocol.isInvalid()`. Both helpers reject
null and blank internally, so the pre-#4048 blank semantics return for free,
and the `String.valueOf(port)` round-trip — which turned a null port into the
non-blank literal `"null"` — is gone.
### Tests
The existing `RegistryProtocolTest` asserted the broken behaviour (it
expected a trailing-dot host and an all-null protocol to be valid, and mutated
`protocol3` where it declared `protocol4`), which is why CI stayed green.
Replaced with 15 per-scenario cases; its two host values are kept as explicit
invalid cases.
- new tests vs. fixed impl: 15 passed
- new tests vs. old impl (mutation check): **7 failed**
- full `hertzbeat-common-core` suite: 633 passed
- checkstyle: passed
Not an availability fix — `MetricsCollect` runs `preCheck()` and `collect()`
in one `try` with a shared `catch` setting `Code.FAIL`, so a bad host
previously failed at connect time instead. The visible change is a clear
`registry collect must have a valid registry protocol param!` instead of an
opaque connection error, without burning a timeout first.
## Checklist
- [x] I have read the [Contributing
Guide](https://hertzbeat.apache.org/docs/community/code_style_and_quality_guide)
- [x] I have written the necessary doc or comment.
- [x] I have added the necessary unit tests and all cases have passed.
## Add or update API
- [ ] I have added the necessary [e2e
tests](https://github.com/apache/hertzbeat/tree/master/e2e) and all cases have
passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
