Copilot commented on code in PR #7658:
URL: https://github.com/apache/ignite-3/pull/7658#discussion_r2852778621
##########
modules/api/src/main/java/org/apache/ignite/table/mapper/MapperBuilder.java:
##########
@@ -304,13 +314,9 @@ private static SimpleEntry<String, String>
getColumnToFieldMapping(Field fld) {
}
/**
- * Gets all fields of the given class and its parents (if any).
+ * Provides a Stream of the Class hierarchy for the provided type. Ordered
from Base to Superclasses.
Review Comment:
`typeHierarchy`'s Javadoc claims the stream is "Ordered from Base to
Superclasses", but the implementation starts from `clazz` and iterates via
`getSuperclass()`, i.e., from the most-derived class up the inheritance chain
(excluding `Object`). Please clarify the wording to avoid confusion about the
order.
```suggestion
* Provides a Stream of the Class hierarchy for the provided type,
starting from the given class
* and walking up through its superclasses (excluding {@link Object}).
```
##########
modules/api/src/main/java/org/apache/ignite/table/mapper/MapperBuilder.java:
##########
@@ -152,11 +151,21 @@ private void ensureNotStale() {
* @throws IllegalArgumentException If a field is {@code null} or if the
class has no declared field with the given name.
*/
Review Comment:
The Javadoc for `requireValidField` says it fails if "the class has no
declared field with the given name", but the implementation now walks the full
class hierarchy (including superclasses). Please update the Javadoc to match
the new behavior (e.g., "no field with the given name in the class hierarchy").
##########
modules/api/src/main/java/org/apache/ignite/table/mapper/MapperBuilder.java:
##########
@@ -152,11 +151,21 @@ private void ensureNotStale() {
* @throws IllegalArgumentException If a field is {@code null} or if the
class has no declared field with the given name.
*/
private String requireValidField(String fieldName) {
- try {
- if (fieldName == null || targetType.getDeclaredField(fieldName) ==
null) {
- throw new IllegalArgumentException("Mapping for a column
already exists: " + fieldName);
- }
- } catch (NoSuchFieldException e) {
+ if (fieldName == null) {
+ throw new IllegalArgumentException("Mapping for a column already
exists: " + fieldName);
Review Comment:
`requireValidField` throws `IllegalArgumentException("Mapping for a column
already exists: " + fieldName)` when `fieldName` is null, which is misleading
(this path is validating the field name, not column mapping). Consider throwing
a dedicated message like "Field name must not be null" (and/or using
`Objects.requireNonNull(fieldName, ...)`).
```suggestion
throw new IllegalArgumentException("Field name must not be
null");
```
--
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]
