f1amingo commented on PR #11088:
URL: https://github.com/apache/rocketmq/pull/11088#issuecomment-5631275398
Reviewed the change — the validation itself is correct. One design
suggestion on the attribute abstraction:
All existing `Attribute` subclasses (`StringAttribute`, `BooleanAttribute`,
`EnumAttribute`, `LongRangeAttribute`) only model a value shape — none embeds a
domain concept, and `TopicNameAttribute` currently serves a single attribute.
Since `verify` is already the extension point, could we make the validation
pluggable on `StringAttribute` instead of introducing a domain-typed subclass?
```java
public class StringAttribute extends Attribute {
private final Consumer<String> validator;
public StringAttribute(String name, boolean changeable) {
this(name, changeable, null);
}
public StringAttribute(String name, boolean changeable, Consumer<String>
validator) {
super(name, changeable);
this.validator = validator;
}
@Override
public void verify(String value) {
checkNotNull(value);
if (validator != null) {
validator.accept(value);
}
}
}
```
and wire it at the declaration site:
```java
public static final StringAttribute LITE_BIND_TOPIC_ATTRIBUTE = new
StringAttribute(
"lite.bind.topic", true, value -> {
TopicValidator.ValidateResult result =
TopicValidator.validateTopic(value);
if (!result.isValid()) {
throw new RuntimeException(result.getRemark());
}
});
```
The two-arg constructor remains unchanged, so existing attributes like
`lite.sub.wildcard` keep their behavior, and `TopicNameAttribute` can be
dropped entirely. Not a blocker for correctness — just keeps the framework
abstraction shape-only.
--
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]
