Enforce string values in InstanceSpec's custom properties #1274
Conversation
`InstanceSpec`'s `customProperties` will be fed to `Properties` in `QuorumConfigBuilder::buildConfigProperties`, so we have to make sure it contains only string values. Instead of `ClassCastException` in read phase, this pr complains in `InstanceSpec` construction phase. We could deprecate these constructors in next step by introducing builder for `InstanceSpec`(apache#1222). Fixes apache#1178, CURATOR-663.
kezhuw
force-pushed
the
fix-class-cast-exception-due-to-non-strings-in-custom-properties
branch
from
2330f0a to
50b74fb
Compare
kezhuw
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR enforces that all values in InstanceSpec's customProperties map are strings to prevent ClassCastException errors when the properties are later used in QuorumConfigBuilder::buildConfigProperties. The validation is moved from runtime usage to construction time for earlier error detection.
Key changes:
- Added validation to ensure all custom property values are strings during
InstanceSpecconstruction - Replaced potential runtime
ClassCastExceptionwith earlyIllegalArgumentException
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| private static Map<String, Object> enforceStringMap(Map<String, Object> properties) { | ||
| for (Map.Entry<String, Object> entry : properties.entrySet()) { | ||
| if (!(entry.getValue() instanceof String)) { | ||
| String msg = String.format("property %s has non string value %s", entry.getKey(), entry.getValue()); |
There was a problem hiding this comment.
The error message should include the expected type for clarity. Consider changing to: "Property '%s' must have a string value, but found %s of type %s" and include entry.getValue().getClass().getSimpleName().
| String msg = String.format("property %s has non string value %s", entry.getKey(), entry.getValue()); | |
| String msg = String.format("Property '%s' must have a string value, but found %s of type %s", entry.getKey(), entry.getValue(), entry.getValue() == null ? "null" : entry.getValue().getClass().getSimpleName()); |
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
I'd expect a better solution to change the type signature in the ctor, but it would be a break change anyway. Perhaps we can track these possible break changes that helps in a tracking issue said for 6.0.
I think we can create labels for this ? We should discuss this in dev mail. |
3 participants
InstanceSpec'scustomPropertieswill be fed toPropertiesinQuorumConfigBuilder::buildConfigProperties, so we have to make sure itcontains only string values.
Instead of
ClassCastExceptionin read phase, this pr complains inInstanceSpecconstruction phase.We could deprecate these constructors in next step by introducing builder
for
InstanceSpec(#1222).Fixes #1178, CURATOR-663.