Conversation
| * @param conn the connection | ||
| * @throws SQLException sql exception | ||
| */ | ||
| protected void markTransactionSerializable(Connection conn) throws SQLException { |
There was a problem hiding this comment.
Is this deletion intentional?
There was a problem hiding this comment.
Yes to fix the build check. markTransactionSerializable is added twice in this file.
There was a problem hiding this comment.
Yes, unrelated but this is to fix the broken build.
|
|
||
| /** Convenience constructor using the default step param separator {@code __}. */ | ||
| public ParamEvaluator(ExprEvaluator exprEvaluator, ObjectMapper objectMapper) { | ||
| this(exprEvaluator, objectMapper, "__"); |
There was a problem hiding this comment.
nit: Would it make sense to keep this a private final and call it DEFAULT_STEP_PARAM_SEPARATOR? I always find it weird to pass in string literals for defaults.
There was a problem hiding this comment.
Alternatively I think we could use both @AllArgsConstructor and @RequiredArgsConstructor from lombok and set private final String stepParamSeparator = "__";
There was a problem hiding this comment.
Both annotations generate the same 2-arg constructor when stepParamSeparator has an initializer, Lombok's @AllArgsConstructor also skips initialized final fields.
There was a problem hiding this comment.
That seems so dumb 🤦🏻
| @@ -353,8 +358,8 @@ private Map<String, Parameter> getReferencedParams( | |||
| /** Extract step id and param name from the reference. It handles `__`, `___`, and `____`. */ | |||
There was a problem hiding this comment.
Are these javadocs accurate anymore?
| @Getter | ||
| @Setter | ||
| public class ParamEvaluatorProperties implements StepParamSeparator { | ||
| private String stepParamSeparator = "__"; |
There was a problem hiding this comment.
If possible add DEFAULT_STEP_PARAM_SEPARATOR in constant and refer it here as well as in MaestroIdValidator instead of creating DEFAULT_STEP_PARAM_SEPARATOR variable there.
| int idx2 = refParam.lastIndexOf(STEP_PARAM_SEPARATOR); | ||
| int idx1 = refParam.indexOf(stepParamSeparator); | ||
| int idx2 = refParam.lastIndexOf(stepParamSeparator); | ||
| // ___ or ____ case |
There was a problem hiding this comment.
The inline comment on the idx1 != idx2 branch still says // ___ or ____ case, which assumes __ as the separator. The Javadoc can be updated. Suggest updating to something like // separator overlap case to stay accurate regardless of configured separator.
| private final StepActionProperties stepAction; | ||
|
|
||
| /** Returns the param evaluator properties, defaulting to {@code __} separator if not set. */ | ||
| public ParamEvaluatorProperties getParamEvaluator() { |
There was a problem hiding this comment.
The custom getter that overrides Lombok's @Getter to return a default when null works, but the other properties (queue, sel, stepAction) don't do this. Consider whether initializing the field directly at initialization time or using @DefaultValue would be more consistent.
There was a problem hiding this comment.
The asymmetry is intentional. queue, sel, and stepAction are pre-existing required fields, if absent the app should fail to start. paramEvaluator is a new optional field added for backward compatibility, so it needs a sensible default when not configured. I considered both alternatives:
@DefaultValuerequires dropping@AllArgsConstructorand writing a manual constructor which seems more disruptive- Field initializer doesn't work since
@AllArgsConstructorgenerates a constructor that overwrites it, so Spring Boot's constructor binding still passes null when the property is absent
Happy to switch to a manual constructor with @DefaultValue if you prefer consistency over brevity.
4 participants
Pull Request type
./gradlew build --write-locksto refresh dependencies)NOTE: Please remember to run
./gradlew spotlessApplyto fix any format violations.Changes in this PR
The step parameter separator (default
__) used in cross-step references likestep1__paramwas previously hardcoded. As a result, any step ID or parameter name containing__was rejected by validation.This PR makes the separator configurable via maestro.param-evaluator.step-param-separator. Netflix OSS users keep the existing behavior (__ by default). Teams can now choose a different separator (e.g.,
___), allowing__to appear freely in step IDs and parameter names without triggering validation errors.