JNG-6025 BreakRecursion #230
Conversation
gaborflorian
requested review from
robertcsakany,
bencegelei and
noherczeg
and removed request for
robertcsakany
WalkthroughThe changes introduce a new configuration parameter Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant Provider as SelectStatementExecutorProvider
participant Executor as SelectStatementExecutor
Config->>Provider: Set maximumRecursionCount
Provider->>Executor: Create with maximumRecursionCount
Executor-->>Executor: Track recursion depth
alt Recursion depth exceeded
Executor--xExecutor: Stop further recursion
end
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModuleConfiguration.java (1)
60-60: Consider making the default value configurable via system property.For better flexibility, consider allowing the default value to be overridden via system property, similar to other configuration parameters.
Example implementation:
- Integer rdbmsDaoMaximumRecursionCount = 3; + Integer rdbmsDaoMaximumRecursionCount = Integer.getInteger("judo.rdbms.dao.maximum.recursion.count", 3);judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/executors/SelectStatementExecutor.java (5)
177-177: Ensure Type Consistency Between Field and Constructor ParameterThe field
maximumRecursionCountis declared asint, but the constructor parameter isInteger. Consider changing the constructor parameter tointfor consistency and to avoid unnecessary boxing and unboxing.
775-776: Consider Refactoring Method Signature for MaintainabilityThe
runQuerymethod has a large number of parameters, which can make it difficult to read and maintain. Consider encapsulating related parameters into a separate object or class to improve readability and reduce complexity.
776-776: Prefer Deque over Stack for Mutable LIFO OperationsThe
Stackclass is considered legacy and is synchronized, which may lead to unnecessary overhead. Consider usingDeque<SubSelect>(e.g.,ArrayDeque) for better performance and modern API practices.
1092-1092: Clarify Reference Equality Check in Recursion CounterIn the recursion count check
subSelectStack.stream().filter(s -> s == subSelect).count(), you're using==to compare references. Ensure that this is intentional and thatSubSelectinstances are not duplicated elsewhere. If logical equality is desired, use.equals()instead.
1093-1093: Handle Maximum Recursion Depth ReachedWhen the
maximumRecursionCountis reached, the recursion stops silently. Consider logging a warning or throwing an exception to inform that the recursion limit has been hit, which can aid in debugging.judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/SelectStatementExecutorProvider.java (1)
77-80: Remove Unnecessary Default Value AssignmentThe field
maximumRecursionCountis initialized with a default value of3, but it is immediately overwritten by dependency injection. Since Guice will inject the value or use the default provided by the@JudoConfigurationQualifiers.RdbmsDaoMaximumRecursionCount, the explicit initialization is unnecessary.Apply this diff to remove the default value assignment:
@Inject(optional = true) @JudoConfigurationQualifiers.RdbmsDaoMaximumRecursionCount - private Integer maximumRecursionCount = 3; + private Integer maximumRecursionCount;judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoConfigurationQualifiers.java (1)
30-34: Maintain Alphabetical Order of QualifiersFor better readability and maintainability, consider placing the new qualifier
RdbmsDaoMaximumRecursionCountin alphabetical order within the class.Apply this diff to reorder the qualifiers:
public @interface RdbmsDaoChunkSize {} + @Qualifier + @Target({ ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD }) + @Retention(RetentionPolicy.RUNTIME) + public @interface RdbmsDaoMaximumRecursionCount {} + @Qualifier @Target({ ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD }) @Retention(RetentionPolicy.RUNTIME) public @interface RdbmsDaoOptimisticLockEnabled {} - - @Qualifier - @Target({ ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD }) - @Retention(RetentionPolicy.RUNTIME) - public @interface RdbmsDaoMaximumRecursionCount {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/executors/SelectStatementExecutor.java(14 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoConfigurationQualifiers.java(1 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModule.java(4 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModuleConfiguration.java(1 hunks)judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/SelectStatementExecutorProvider.java(2 hunks)judo-runtime-core-spring/src/main/java/hu/blackbelt/judo/runtime/core/spring/JudoDefaultSpringConfiguration.java(2 hunks)
🔇 Additional comments (5)
judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModuleConfiguration.java (1)
60-60: Document the new configuration parameter and justify the default value.The new
rdbmsDaoMaximumRecursionCountparameter is introduced to prevent stack overflow on recursive derived calls, but it lacks documentation explaining:
- The purpose and impact of this parameter
- Why 3 was chosen as the default value
- What happens when the recursion limit is reached
- Recommendations for adjusting this value based on use cases
Let's check if there are any existing documentation patterns in the codebase:
judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/dao/rdbms/SelectStatementExecutorProvider.java (1)
92-92: Ensure NonNull Annotation ConsistencyAlthough
maximumRecursionCountis injected and expected to be non-null, it might not be explicitly annotated with@NonNull. To prevent potentialNullPointerException, ensure that it is validated or defaulted if not provided.judo-runtime-core-spring/src/main/java/hu/blackbelt/judo/runtime/core/spring/JudoDefaultSpringConfiguration.java (2)
267-267: Document the rationale for the default recursion limit.The default value of 3 for
maximumRecursionCountseems arbitrary. Please add a comment explaining:
- Why 3 was chosen as the default value
- What are the implications of this limit
- How users should determine if they need to adjust this value
277-277: LGTM! The configuration looks good.The
maximumRecursionCountis properly integrated into theSelectStatementExecutorbuilder, which aligns with the PR objective of preventing stack overflow in recursive derived calls.judo-runtime-core-guice/src/main/java/hu/blackbelt/judo/runtime/core/guice/JudoDefaultModule.java (1)
90-90: LGTM! The parameter integration is thorough and consistent.The
rdbmsDaoMaximumRecursionCountparameter is properly:
- Added to the builder
- Included in the constructor
- Integrated into the configuration
- Bound for dependency injection
The implementation follows the established pattern for configuration parameters in the codebase.
Also applies to: 143-143, 196-196, 273-273
| long cnt = subSelectStack.stream().filter(s -> s == subSelect).count(); | ||
| if (cnt < maximumRecursionCount) { | ||
| subSelectStack.push(subSelect); | ||
| runSubQuery(jdbcTemplate, query, subSelect, e.getKey(), results, | ||
| mask != null ? (Map<String, Object>) mask.get(subSelect.getTransferRelation().getName()) : null, | ||
| queryParameters); | ||
| queryParameters, subSelectStack); | ||
| subSelectStack.pop(); | ||
| } |
There was a problem hiding this comment.
Use Try-Finally Block to Ensure Stack Integrity
When pushing subSelect onto subSelectStack, if an exception occurs during runSubQuery, the pop() method may not be called, leading to stack inconsistency. Wrap the runSubQuery call in a try-finally block to ensure subSelectStack.pop() is always executed.
Apply this diff to fix the issue:
subSelectStack.push(subSelect);
+try {
runSubQuery(jdbcTemplate, query, subSelect, e.getKey(), results,
mask != null ? (Map<String, Object>) mask.get(subSelect.getTransferRelation().getName()) : null,
queryParameters, subSelectStack);
+} finally {
subSelectStack.pop();
+}
- subSelectStack.pop();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| long cnt = subSelectStack.stream().filter(s -> s == subSelect).count(); | |
| if (cnt < maximumRecursionCount) { | |
| subSelectStack.push(subSelect); | |
| runSubQuery(jdbcTemplate, query, subSelect, e.getKey(), results, | |
| mask != null ? (Map<String, Object>) mask.get(subSelect.getTransferRelation().getName()) : null, | |
| queryParameters); | |
| queryParameters, subSelectStack); | |
| subSelectStack.pop(); | |
| } | |
| long cnt = subSelectStack.stream().filter(s -> s == subSelect).count(); | |
| if (cnt < maximumRecursionCount) { | |
| subSelectStack.push(subSelect); | |
| try { | |
| runSubQuery(jdbcTemplate, query, subSelect, e.getKey(), results, | |
| mask != null ? (Map<String, Object>) mask.get(subSelect.getTransferRelation().getName()) : null, | |
| queryParameters, subSelectStack); | |
| } finally { | |
| subSelectStack.pop(); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/executors/SelectStatementExecutor.java (2)
177-177: Add documentation for the new recursion limit parameter.The new
maximumRecursionCountfield and constructor parameter would benefit from documentation explaining:
- The purpose of the recursion limit
- Valid value ranges
- Default value implications
Add JavaDoc comments:
+ /** + * Maximum allowed depth for recursive derived calls to prevent stack overflow. + * Must be a positive integer. Default value is 3. + */ private final int maximumRecursionCount; @Builder public SelectStatementExecutor(@NonNull final AsmModel asmModel, @NonNull final RdbmsModel rdbmsModel, @NonNull final TransformationTraceService transformationTraceService, @NonNull final QueryFactory queryFactory, @NonNull final RdbmsParameterMapper<ID> rdbmsParameterMapper, @NonNull final RdbmsResolver rdbmsResolver, @NonNull final DataTypeManager dataTypeManager, @NonNull final IdentifierProvider<ID> identifierProvider, @NonNull final RdbmsBuilder<ID> rdbmsBuilder, @NonNull final MetricsCollector metricsCollector, @NonNull final Integer chunkSize, + /** + * @param maximumRecursionCount Maximum depth allowed for recursive derived calls. + * Must be a positive integer. + */ @NonNull final Integer maximumRecursionCount)Also applies to: 190-191
775-776: Add parameter validation and documentation for the subSelectStack.The new
subSelectStackparameter is added consistently across methods but lacks validation and documentation.Add parameter validation and documentation:
/** * Run logical subquery and return result set. * ... + * @param subSelectStack Stack to track recursive query execution depth * @return result set */ private QueryResult<ID> runQuery( ... + @NonNull final Stack<SubSelect> subSelectStack // Prevent null stack ) { Objects.requireNonNull(subSelectStack, "subSelectStack must not be null");Apply similar changes to
mapResultsandexecuteSubQuerymethods.Also applies to: 898-900, 1261-1262
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/executors/SelectStatementExecutor.java(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ⏳ Build, test and deploy artifacts
🔇 Additional comments (1)
judo-runtime-core-dao-rdbms/src/main/java/hu/blackbelt/judo/runtime/core/dao/rdbms/executors/SelectStatementExecutor.java (1)
1092-1102: Well-implemented recursion control!The implementation effectively prevents stack overflow by:
- Tracking recursion depth using a stack
- Limiting recursive calls based on a configurable threshold
- Ensuring proper stack maintenance with try-finally blocks
| long cnt = subSelectStack.stream().filter(s -> s == subSelect).count(); | ||
| if (cnt < maximumRecursionCount) { | ||
| subSelectStack.push(subSelect); | ||
| try { | ||
| runSubQuery(jdbcTemplate, query, subSelect, e.getKey(), results, | ||
| mask != null ? (Map<String, Object>) mask.get(subSelect.getTransferRelation().getName()) : null, | ||
| queryParameters, subSelectStack); | ||
| } finally { | ||
| subSelectStack.pop(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve validation and error handling for recursion depth control.
The recursion depth check is implemented correctly, but there are two improvements needed:
- Validate that
maximumRecursionCountis positive in the constructor - Handle the case when maximum recursion depth is reached
Add constructor validation and error handling:
public SelectStatementExecutor(..., @NonNull final Integer maximumRecursionCount) {
+ if (maximumRecursionCount <= 0) {
+ throw new IllegalArgumentException("maximumRecursionCount must be positive");
+ }
this.maximumRecursionCount = maximumRecursionCount;
...
}
private void mapResults(...) {
...
if (cnt < maximumRecursionCount) {
subSelectStack.push(subSelect);
try {
runSubQuery(...);
} finally {
subSelectStack.pop();
}
+ } else {
+ log.warn("Maximum recursion depth {} reached for subSelect {}",
+ maximumRecursionCount, subSelect.getTransferRelation().getName());
}Committable suggestion skipped: line range outside the PR's diff.
JNG-6025 BreakRecursion