Aias00 commented on code in PR #7325:
URL: https://github.com/apache/shenyu/pull/7325#discussion_r4113953247
##########
shenyu-common/src/main/java/org/apache/shenyu/common/dto/convert/selector/CacheUpstream.java:
##########
@@ -80,6 +80,25 @@ public class CacheUpstream extends CommonUpstream {
* @param builder builder
*/
public CacheUpstream(final Builder builder) {
+ boolean statusValue = builder.statusValue;
+ if (!builder.statusSet) {
+ statusValue = defaultStatus();
+ }
+ setUpstreamHost(builder.upstreamHost);
+ setProtocol(builder.protocol);
+ setUpstreamUrl(builder.upstreamUrl);
+ setStatus(statusValue);
+ setTimestamp(builder.timestamp);
+ this.cacheType = builder.cacheType;
+ this.url = builder.url;
+ this.password = builder.password;
Review Comment:
[nit] Six of these assignments can never carry a value.
`Builder` exposes fluent setters only for `upstreamHost`, `protocol`,
`upstreamUrl`, `weight`, `timestamp`, `cacheType`, `url` and `database` (see
Builder at :330-526). There is **no** `password(...)`, `master(...)`,
`mode(...)`, `maxIdle(...)`, `minIdle(...)`, `maxActive(...)` or `maxWait(...)`
method, so those fields stay at their Java defaults (`null` / `0`) no matter
what a caller intends.
Copying them is harmless but misleading — it reads as if the builder
supported them. Two ways out, pick one:
```java
// a) add the missing setters so the fields are reachable
public Builder password(final String password) { this.password = password;
return this; }
// ... same for master, mode, maxIdle, minIdle, maxActive, maxWait
// b) drop the six dead assignments until someone actually needs them
```
Option (a) is preferable if the cache plugin is supposed to be configurable
with credentials and pool sizing — right now it is not reachable through this
API at all.
--
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]
