Copilot commented on code in PR #37377:
URL: https://github.com/apache/shardingsphere/pull/37377#discussion_r2617279630
##########
infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/pagination/engine/RowNumberPaginationContextEngine.java:
##########
@@ -129,15 +129,38 @@ private PaginationContext
createPaginationWithRowNumber(final Collection<BinaryO
return new PaginationContext(offset, rowCount, params);
}
- private RowNumberValueSegment createRowNumberValueSegment(final
ExpressionSegment expression, final boolean boundOpened) {
+ private RowNumberValueSegment createRowNumberValueSegment(final
ExpressionSegment expression, final boolean boundOpened, final List<Object>
params) {
int startIndex = expression.getStartIndex();
int stopIndex = expression.getStopIndex();
if (expression instanceof LiteralExpressionSegment) {
return new NumberLiteralRowNumberValueSegment(startIndex,
stopIndex, Long.parseLong(((LiteralExpressionSegment)
expression).getLiterals().toString()), boundOpened);
}
if (expression instanceof ParameterMarkerExpressionSegment) {
+ int parameterIndex = ((ParameterMarkerExpressionSegment)
expression).getParameterMarkerIndex();
+ // The parameter index derived from the parser might be incorrect
in complex subqueries.
+ // If the parameter value is not a number, it is not a valid
pagination value, so we ignore it.
+ if (isInvalidParameter(params, parameterIndex)) {
+ return null;
+ }
return new ParameterMarkerRowNumberValueSegment(startIndex,
stopIndex, ((ParameterMarkerExpressionSegment)
expression).getParameterMarkerIndex(), boundOpened);
}
return new ExpressionRowNumberValueSegment(startIndex, stopIndex,
expression, boundOpened);
}
+
+ private boolean isInvalidParameter(final List<Object> params, final int
parameterIndex) {
+ if (null != params && !params.isEmpty() && parameterIndex <
params.size()) {
+ Object value = params.get(parameterIndex);
Review Comment:
When value is null, the condition !(value instanceof Number) evaluates to
true, and then isStringNumber(value) is called with null, which will cause a
NullPointerException in value.toString() at line 160. Add a null check before
calling isStringNumber, or handle null values earlier in the condition chain.
```suggestion
Object value = params.get(parameterIndex);
if (value == null) {
return true;
}
```
##########
infra/binder/core/src/test/java/org/apache/shardingsphere/infra/binder/context/segment/select/pagination/engine/RowNumberPaginationContextEngineTest.java:
##########
@@ -116,6 +117,17 @@ void
assertCreatePaginationContextWhenParameterMarkerRowNumberValueSegment() {
assertFalse(paginationContext.getRowCountSegment().isPresent());
}
+ @Test
+ void assertCreatePaginationContextWithInvalidParameter() {
+ Projection projectionWithRowNumberAlias = new ColumnProjection(null,
ROW_NUMBER_COLUMN_NAME, ROW_NUMBER_COLUMN_ALIAS, mock(DatabaseType.class));
+ ProjectionsContext projectionsContext = new ProjectionsContext(0, 0,
false, Collections.singleton(projectionWithRowNumberAlias));
+ ColumnSegment left = new ColumnSegment(0, 10, new
IdentifierValue(ROW_NUMBER_COLUMN_NAME));
+ ParameterMarkerExpressionSegment right = new
ParameterMarkerExpressionSegment(0, 10, 0);
+ BinaryOperationExpression expression = new
BinaryOperationExpression(0, 0, left, right, "<", null);
+ PaginationContext paginationContext =
paginationContextEngine.createPaginationContext(Collections.singletonList(expression),
projectionsContext, Arrays.asList("5214|1521|5152", 10));
+ assertFalse(paginationContext.getRowCountSegment().isPresent());
+ }
Review Comment:
The test only covers the case where the parameter at index 0 is invalid. It
should also test scenarios where: 1) the parameter index is out of bounds, 2)
the params list is null, 3) the params list is empty, and 4) a valid numeric
string like "10" is provided (which should be treated as valid according to the
isStringNumber logic). These cases would ensure the defensive checks work
correctly in all scenarios.
##########
infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/pagination/engine/RowNumberPaginationContextEngine.java:
##########
@@ -129,15 +129,38 @@ private PaginationContext
createPaginationWithRowNumber(final Collection<BinaryO
return new PaginationContext(offset, rowCount, params);
}
- private RowNumberValueSegment createRowNumberValueSegment(final
ExpressionSegment expression, final boolean boundOpened) {
+ private RowNumberValueSegment createRowNumberValueSegment(final
ExpressionSegment expression, final boolean boundOpened, final List<Object>
params) {
int startIndex = expression.getStartIndex();
int stopIndex = expression.getStopIndex();
if (expression instanceof LiteralExpressionSegment) {
return new NumberLiteralRowNumberValueSegment(startIndex,
stopIndex, Long.parseLong(((LiteralExpressionSegment)
expression).getLiterals().toString()), boundOpened);
}
if (expression instanceof ParameterMarkerExpressionSegment) {
+ int parameterIndex = ((ParameterMarkerExpressionSegment)
expression).getParameterMarkerIndex();
+ // The parameter index derived from the parser might be incorrect
in complex subqueries.
+ // If the parameter value is not a number, it is not a valid
pagination value, so we ignore it.
+ if (isInvalidParameter(params, parameterIndex)) {
+ return null;
+ }
return new ParameterMarkerRowNumberValueSegment(startIndex,
stopIndex, ((ParameterMarkerExpressionSegment)
expression).getParameterMarkerIndex(), boundOpened);
}
return new ExpressionRowNumberValueSegment(startIndex, stopIndex,
expression, boundOpened);
}
+
+ private boolean isInvalidParameter(final List<Object> params, final int
parameterIndex) {
+ if (null != params && !params.isEmpty() && parameterIndex <
params.size()) {
+ Object value = params.get(parameterIndex);
+ return !(value instanceof Number) && !isStringNumber(value);
+ }
+ return false;
+ }
Review Comment:
Add JavaDoc documentation for the isInvalidParameter method to explain its
purpose, parameters, and return value. The comment on lines 140-141 should be
moved to proper JavaDoc format.
##########
infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/pagination/engine/RowNumberPaginationContextEngine.java:
##########
@@ -129,15 +129,38 @@ private PaginationContext
createPaginationWithRowNumber(final Collection<BinaryO
return new PaginationContext(offset, rowCount, params);
}
Review Comment:
The method signature has changed to include a params parameter, but there is
no JavaDoc documentation explaining this parameter or the new null return
behavior. Add documentation to describe when this method returns null and what
the params parameter is used for.
```suggestion
/**
* Creates a RowNumberValueSegment based on the given expression.
*
* @param expression the expression segment representing the row number
value
* @param boundOpened whether the bound is opened (inclusive or
exclusive)
* @param params the list of parameter values, typically from a prepared
statement, used to resolve parameter markers in the expression
* @return a RowNumberValueSegment representing the row number value, or
{@code null} if the parameter value is not a valid number for pagination
*/
```
##########
infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/select/pagination/engine/RowNumberPaginationContextEngine.java:
##########
@@ -129,15 +129,38 @@ private PaginationContext
createPaginationWithRowNumber(final Collection<BinaryO
return new PaginationContext(offset, rowCount, params);
}
- private RowNumberValueSegment createRowNumberValueSegment(final
ExpressionSegment expression, final boolean boundOpened) {
+ private RowNumberValueSegment createRowNumberValueSegment(final
ExpressionSegment expression, final boolean boundOpened, final List<Object>
params) {
int startIndex = expression.getStartIndex();
int stopIndex = expression.getStopIndex();
if (expression instanceof LiteralExpressionSegment) {
return new NumberLiteralRowNumberValueSegment(startIndex,
stopIndex, Long.parseLong(((LiteralExpressionSegment)
expression).getLiterals().toString()), boundOpened);
}
if (expression instanceof ParameterMarkerExpressionSegment) {
+ int parameterIndex = ((ParameterMarkerExpressionSegment)
expression).getParameterMarkerIndex();
+ // The parameter index derived from the parser might be incorrect
in complex subqueries.
+ // If the parameter value is not a number, it is not a valid
pagination value, so we ignore it.
+ if (isInvalidParameter(params, parameterIndex)) {
+ return null;
+ }
return new ParameterMarkerRowNumberValueSegment(startIndex,
stopIndex, ((ParameterMarkerExpressionSegment)
expression).getParameterMarkerIndex(), boundOpened);
}
return new ExpressionRowNumberValueSegment(startIndex, stopIndex,
expression, boundOpened);
}
+
+ private boolean isInvalidParameter(final List<Object> params, final int
parameterIndex) {
+ if (null != params && !params.isEmpty() && parameterIndex <
params.size()) {
+ Object value = params.get(parameterIndex);
+ return !(value instanceof Number) && !isStringNumber(value);
+ }
+ return false;
+ }
+
+ private boolean isStringNumber(final Object value) {
+ try {
+ Long.parseLong(value.toString());
+ return true;
+ } catch (final NumberFormatException ex) {
+ return false;
+ }
+ }
Review Comment:
Add JavaDoc documentation for the isStringNumber method to explain its
purpose, parameters, and return value. This will help maintainers understand
the validation logic.
--
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]
