Copilot commented on code in PR #13262: URL: https://github.com/apache/ignite/pull/13262#discussion_r3467698668
########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringNodeFactory.java: ########## @@ -0,0 +1,164 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.util.tostring; + +import java.util.Collection; +import java.util.LinkedList; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.function.Supplier; + +import static org.apache.ignite.internal.util.tostring.GridToStringNode.identities; + +/** + * A factory class responsible for creating appropriate GridToStringNode instances + * based on the type and value of the object being processed. + */ +public class GridToStringNodeFactory { + /** + * Creates a list of nodes from arrays of property names and values. + * Handles sensitive data exclusion and node reuse from a thread-local cache. + * @param addNames Array of property names. + * @param addVals Array of property values. + * @param addSens Array of flags indicating if a property is sensitive. + * @param addLen The number of elements to process. + * @return A list of constructed GridToStringNode objects. + */ + public static List<GridToStringNode> getNodes(Object[] addNames, + Object[] addVals, + boolean[] addSens, + int addLen) { + List<GridToStringNode> result = new LinkedList<>(); + boolean includeSensitive = GridToStringBuilder.includeSensitive(); + for (int i = 0; i < addLen; i++) { + if (!includeSensitive && shouldBeExcluded(addVals, addSens, i)) + continue; + String propName = String.valueOf(addNames[i]); + GridToStringNode node = recoverOrCreate(propName, addVals[i]); + result.add(node); + } + return result; + } + + /** + * Creates a node for a field based on its descriptor and the parent object. + * This method acts as a dispatcher, routing the creation logic based on the field's type. + * @param obj The parent object containing the field. + * @param fd The descriptor of the field to be processed. + * @return A new GridToStringNode for the field's value. + */ + static GridToStringNode getGridToStringNode(Object obj, GridToStringFieldDescriptor fd) { + String childPropName = fd.getName(); + if (obj == null) + return new GridToStringNullNode(childPropName); + return switch (fd.type()) { + case GridToStringFieldDescriptor.FIELD_TYPE_OBJECT -> { + Supplier<Class<?>> fieldClsSupplier = () -> Optional.of(fd) + .map(GridToStringFieldDescriptor::fieldClass) + .map(Class.class::cast) + .orElseGet(obj::getClass); + yield getGridToStringNode(childPropName, () -> fd.objectValue(obj), fieldClsSupplier); + } + case GridToStringFieldDescriptor.FIELD_TYPE_BYTE -> + getGridToStringNode(childPropName, () -> fd.byteValue(obj), () -> byte.class); + case GridToStringFieldDescriptor.FIELD_TYPE_BOOLEAN -> + getGridToStringNode(childPropName, () -> fd.booleanValue(obj), () -> boolean.class); + case GridToStringFieldDescriptor.FIELD_TYPE_CHAR -> + getGridToStringNode(childPropName, () -> fd.charValue(obj), () -> char.class); + case GridToStringFieldDescriptor.FIELD_TYPE_SHORT -> + getGridToStringNode(childPropName, () -> fd.shortValue(obj), () -> short.class); + case GridToStringFieldDescriptor.FIELD_TYPE_INT -> + getGridToStringNode(childPropName, () -> fd.intField(obj), () -> int.class); + case GridToStringFieldDescriptor.FIELD_TYPE_FLOAT -> + getGridToStringNode(childPropName, () -> fd.floatField(obj), () -> float.class); + case GridToStringFieldDescriptor.FIELD_TYPE_LONG -> + getGridToStringNode(childPropName, () -> fd.longField(obj), () -> long.class); + case GridToStringFieldDescriptor.FIELD_TYPE_DOUBLE -> + getGridToStringNode(childPropName, () -> fd.doubleField(obj), () -> double.class); + default -> new GridToStringValueNode(childPropName, "toString is not implemented yet"); + }; + } + + /** + * The core factory method that creates a node for a given value and its class. + * It is the central point for determining the correct node type for any object. + * Handles nulls, recursion, primitives, arrays, collections, maps, and standard objects. + * @param childPropName The property name for the new node. + * @param valSupplier A supplier to lazily retrieve the value. + * @param childFieldClsSupplier A supplier to lazily retrieve the class of the value. + * @return A new GridToStringNode appropriate for the value. + */ + static GridToStringNode getGridToStringNode(String childPropName, + Supplier<Object> valSupplier, + Supplier<Class<?>> childFieldClsSupplier) { + Object val = valSupplier.get(); + if (val == null) + return new GridToStringNullNode(childPropName); + Optional<GridToStringNode> recursionTermination = NodeRecursionMonitor.findRecursionMonitor(val) + .map(monitor -> GridToStringRecursionTerminationNode.of(monitor, val)); + if (recursionTermination.isPresent()) + return recursionTermination.get(); + Class<?> childFieldCls = childFieldClsSupplier.get(); + if (childFieldCls.isPrimitive()) + return new GridToStringValueNode(childPropName, val); + else if (childFieldCls.isArray()) + return new GridToStringArrayNode(childPropName, (Object[])val, childFieldCls); Review Comment: This will throw `ClassCastException` for primitive arrays (e.g., `int[]` cannot be cast to `Object[]`). The array branch needs to handle primitive arrays separately (e.g., by converting via the existing primitive-array formatting logic) and only cast to `Object[]` for reference-type arrays. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/SBLimitedLength.java: ########## @@ -270,6 +264,124 @@ private GridStringBuilder onWrite(int lenBeforeWrite) { return onWrite(curLen); } + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int offset, String str) { + str = GridToStringNode.recoverObject(str); + int headLengthLimit = lenLimit.getHeadLengthLimit(); + if (offset < headLengthLimit) { + impl().insert(offset, str); + if (lenLimit.overflowed(this)) { + String tailCandidate = impl().substring(headLengthLimit); + if (tail == null) + tail = lenLimit.createTail(); + tail.insert(0, tailCandidate); + impl().setLength(headLengthLimit); + } + return this; + } + tail.insert(offset - headLengthLimit, str); + return this; + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int idx, char[] str, int off, int len) { + StringBuilder strBuilder = new StringBuilder(); + for (int i = 0; i < len; i++) + strBuilder.append(str[i + off]); + return i(idx, strBuilder.toString()); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int off, Object obj) { + return i(off, String.valueOf(obj)); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int off, char[] str) { + return i(off, new String(str)); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int dstOff, CharSequence s) { + s = GridToStringNode.recoverObject(s); + StringBuilder strBuilder = new StringBuilder(); + for (int i = 0; i < s.length(); i++) + strBuilder.append(s.charAt(i)); + return i(dstOff, strBuilder.toString()); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int dstOff, CharSequence s, int start, int end) { + s = GridToStringNode.recoverObject(s); + StringBuilder strBuilder = new StringBuilder(); + for (int i = start; i < end; i++) + strBuilder.append(s.charAt(i)); + return i(dstOff, strBuilder.toString()); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int off, boolean b) { + return super.i(off, String.valueOf(b)); + } Review Comment: These `i(...)` overloads bypass `SBLimitedLength`'s custom insert routing by calling `super.i(...)`, which inserts directly into the head buffer and ignores head/tail limiting. They should delegate to this class’s `i(int, String)` (or equivalent) so inserts beyond the head limit are correctly redirected/handled. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringCollectionNode.java: ########## @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.util.tostring; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Iterator; +import org.apache.ignite.internal.util.GridStringBuilder; + +import static org.apache.ignite.internal.util.tostring.GridToStringBuilder.COLLECTION_LIMIT; +import static org.apache.ignite.internal.util.tostring.GridToStringNodeFactory.getGridToStringNode; + +/** + * A node that represents a collection (e.g., List, Set) in the string representation. + * It creates nodes for each element and formats the output with square brackets. + */ +class GridToStringCollectionNode extends NodeRecursionMonitor { + /** A list of child nodes, each representing an element of the collection. */ + private final Collection<GridToStringNode> col; + + /** The simple class name of the collection being represented. */ + private final String colSimpleClsName; + + /** The rule for appending a hint about skipped elements if the collection is too large. */ + private final LongSequenceSkipRule skipRule; + + /** + * Constructs a new collection node. + * Iterates over the input collection, creates a node for each element, + * and populates the internal list up to the collection size limit. + * @param propName The property name. + * @param col The collection to represent. + */ + GridToStringCollectionNode(String propName, Collection<?> col) { + super(propName, col); + try { + aqcuireRecursionMonitor(this); + colSimpleClsName = col.getClass().getSimpleName(); + this.col = new ArrayList<>(Math.min(col.size(), COLLECTION_LIMIT)); + skipRule = new LongSequenceSkipRule(col::size); + Iterator<?> iter = col.iterator(); + while (iter.hasNext() && this.col.size() != COLLECTION_LIMIT) { + Object obj = iter.next(); + GridToStringNode node = getGridToStringNode(null, () -> obj, obj::getClass); Review Comment: If the collection contains `null`, `obj::getClass` will throw at method-reference creation time. Use a null-safe class supplier (e.g., `() -> obj != null ? obj.getClass() : Object.class`) and let `getGridToStringNode` produce a `GridToStringNullNode` for null values. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/NodeRecursionMonitor.java: ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.util.tostring; + +import java.util.IdentityHashMap; +import java.util.Optional; +import org.apache.ignite.internal.util.GridStringBuilder; + +/** + * Abstract base class for nodes that tracks object references + * to prevent infinite recursion during string representation building. + */ +abstract class NodeRecursionMonitor extends GridToStringNode { + /** Thread-local registry to track objects currently being processed. */ + static final ThreadLocal<IdentityHashMap<Object, NodeRecursionMonitor>> OBJECT_REGISTRY = + ThreadLocal.withInitial(IdentityHashMap::new); + + /** The object being monitored for recursive references. */ + private final Object obj; + + /** Flag indicating if the identity hash code should be appended to the output. */ + boolean hashIsRequired; + + /** + * Constructor. + * @param propName Name of the property. + * @param obj Object to monitor for recursion. + */ + NodeRecursionMonitor(String propName, Object obj) { + super(propName); + this.obj = obj; + } + + /** + * Registers the current node in the thread-local registry for the given object. + * @param node The node to register. + */ + void aqcuireRecursionMonitor(NodeRecursionMonitor node) { + OBJECT_REGISTRY.get().put(obj, node); + } Review Comment: Typo in method name: `aqcuireRecursionMonitor` → `acquireRecursionMonitor`. Also, the `node` parameter is redundant (the method can register `this`) which would simplify call sites. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/SBLimitedLength.java: ########## @@ -270,6 +264,124 @@ private GridStringBuilder onWrite(int lenBeforeWrite) { return onWrite(curLen); } + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int offset, String str) { + str = GridToStringNode.recoverObject(str); + int headLengthLimit = lenLimit.getHeadLengthLimit(); + if (offset < headLengthLimit) { + impl().insert(offset, str); + if (lenLimit.overflowed(this)) { + String tailCandidate = impl().substring(headLengthLimit); + if (tail == null) + tail = lenLimit.createTail(); + tail.insert(0, tailCandidate); + impl().setLength(headLengthLimit); + } + return this; + } + tail.insert(offset - headLengthLimit, str); + return this; + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int idx, char[] str, int off, int len) { + StringBuilder strBuilder = new StringBuilder(); + for (int i = 0; i < len; i++) + strBuilder.append(str[i + off]); + return i(idx, strBuilder.toString()); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int off, Object obj) { + return i(off, String.valueOf(obj)); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int off, char[] str) { + return i(off, new String(str)); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int dstOff, CharSequence s) { + s = GridToStringNode.recoverObject(s); + StringBuilder strBuilder = new StringBuilder(); + for (int i = 0; i < s.length(); i++) + strBuilder.append(s.charAt(i)); + return i(dstOff, strBuilder.toString()); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int dstOff, CharSequence s, int start, int end) { + s = GridToStringNode.recoverObject(s); + StringBuilder strBuilder = new StringBuilder(); + for (int i = start; i < end; i++) + strBuilder.append(s.charAt(i)); + return i(dstOff, strBuilder.toString()); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int off, boolean b) { + return super.i(off, String.valueOf(b)); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int off, char c) { + return super.i(off, String.valueOf(c)); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int off, int i) { + return super.i(off, String.valueOf(i)); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int off, long l) { + return super.i(off, String.valueOf(l)); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int off, float f) { + return super.i(off, String.valueOf(f)); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder i(int off, double d) { + return super.i(off, String.valueOf(d)); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder d(int start, int end) { + throw new UnsupportedOperationException("Not supported by this implementation"); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder d(int idx) { + throw new UnsupportedOperationException("Not supported by this implementation"); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder r(int start, int end, String str) { + throw new UnsupportedOperationException("Not supported by this implementation"); + } + + /** {@inheritDoc} */ + @Override public GridStringBuilder nl() { + return a(CommonUtils.nl()); + } + + /** {@inheritDoc} */ + @Override public int length() { + int length = super.length(); + if (tail != null) + length += tail.getSkipped() + tail.length(); + return length; + } + + /** {@inheritDoc} */ + @Override public void setLength(int len) { + throw new UnsupportedOperationException("setLength is not supported by this imlementation"); + } Review Comment: Typo in exception message: `imlementation` → `implementation`. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringBuilder.java: ########## @@ -1994,87 +1497,13 @@ public static <T> String joinToString( return buf.toString(); } - /** - * Checks that object is already saved. - * In positive case this method inserts hash to the saved object entry (if needed) and name@hash for current entry. - * Further toString operations are not needed for current object. - * - * @param buf String builder buffer. - * @param obj Object. - * @param cls Class. - * @param svdObjs Map with saved objects to handle recursion. - * @return {@code True} if object is already saved and name@hash was added to buffer. - * {@code False} if it wasn't saved previously and it should be saved. - */ - private static boolean handleRecursion( - SBLimitedLength buf, - Object obj, - @NotNull Class cls, - IdentityHashMap<Object, EntryReference> svdObjs - ) { - EntryReference ref = svdObjs.get(obj); - - if (ref == null) - return false; - - int pos = ref.pos; - - String name = cls.getSimpleName(); - String hash = identity(obj); - String savedName = name + hash; - String charsAtPos = buf.impl().substring(pos, pos + savedName.length()); - - if (!buf.isOverflowed() && !savedName.equals(charsAtPos)) { - if (charsAtPos.startsWith(cls.getSimpleName())) { - buf.i(pos + name.length(), hash); - - incValues(svdObjs, obj, hash.length()); - } - else - ref.hashNeeded = true; - } - - buf.a(savedName); - - return true; - } - - /** - * Increment positions of already presented objects afterward given object. - * - * @param svdObjs Map with objects already presented in the buffer. - * @param obj Object. - * @param hashLen Length of the object's hash. - */ - private static void incValues(IdentityHashMap<Object, EntryReference> svdObjs, Object obj, int hashLen) { - int baseline = svdObjs.get(obj).pos; - - for (IdentityHashMap.Entry<Object, EntryReference> entry : svdObjs.entrySet()) { - EntryReference ref = entry.getValue(); - - int pos = ref.pos; - - if (pos > baseline) - ref.pos = pos + hashLen; - } - } - - /** - * - */ - private static class EntryReference { - /** Position. */ - int pos; - - /** First object entry needs hash to be written. */ - boolean hashNeeded; - - /** - * @param pos Position. - */ - private EntryReference(int pos) { - this.pos = pos; - hashNeeded = false; - } + /** */ + private static <T extends Throwable> String handleThrowable(T throwable) { + StringWriter strWriter = new StringWriter(); + PrintWriter printWriter = new PrintWriter(strWriter); + throwable.printStackTrace(printWriter); + printWriter.flush(); + printWriter.close(); + return strWriter.toString(); } Review Comment: Returning full stack traces from `toString()` can leak internal details and potentially sensitive data into logs and monitoring outputs. Consider returning a sanitized/summary string (e.g., exception class + message) and optionally logging the full stack trace through the project’s logging facilities. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringMapNode.java: ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.util.tostring; + +import java.util.HashMap; +import java.util.Iterator; +import java.util.Map; +import org.apache.ignite.internal.util.GridStringBuilder; + +import static org.apache.ignite.internal.util.tostring.GridToStringBuilder.COLLECTION_LIMIT; +import static org.apache.ignite.internal.util.tostring.GridToStringNodeFactory.getGridToStringNode; + +/** + * A node that represents a map (key-value pairs) in the string representation. + * It creates nodes for each key and value and formats the output with curly braces. + */ +class GridToStringMapNode extends NodeRecursionMonitor { + /** An internal map that stores key-value pairs as GridToStringNode instances. */ + private final Map<GridToStringNode, GridToStringNode> map; + + /** The simple class name of the map being represented. */ + private final String mapClsSimpleName; + + /** The rule for appending a hint about skipped elements if the map is too large. */ + private final LongSequenceSkipRule skipRule; + + /** + * Constructs a new map node. + * Iterates over the entries of the input map, creates nodes for keys and values, + * and populates the internal map structure up to the collection size limit. + * @param propName The property name. + * @param map The map to represent. + */ + GridToStringMapNode(String propName, Map<?, ?> map) { + super(propName, map); + try { + aqcuireRecursionMonitor(this); + mapClsSimpleName = map.getClass().getSimpleName(); + this.map = new HashMap<>(); + skipRule = new LongSequenceSkipRule(map::size); + Iterator<? extends Map.Entry<?, ?>> iter = map.entrySet().iterator(); + while (iter.hasNext() && this.map.size() != COLLECTION_LIMIT) { + Map.Entry<?, ?> entry = iter.next(); + Object key = entry.getKey(); + Object val = entry.getValue(); + GridToStringNode keyNode = getGridToStringNode(null, () -> key, key::getClass); + GridToStringNode valNode = getGridToStringNode(null, () -> val, val::getClass); Review Comment: Maps can contain null keys/values; `key::getClass` / `val::getClass` will throw `NullPointerException` in that case. Use null-safe class suppliers (or route nulls to `GridToStringNullNode`) so map stringification is robust. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringArrayNode.java: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.util.tostring; + +import org.apache.ignite.internal.util.GridStringBuilder; + +import static org.apache.ignite.internal.util.tostring.GridToStringBuilder.COLLECTION_LIMIT; +import static org.apache.ignite.internal.util.tostring.GridToStringNodeFactory.getGridToStringNode; + +/** + * A node that represents an array in the string representation. + * It creates nodes for each element of the array and formats the output with square brackets. + */ +class GridToStringArrayNode extends NodeRecursionMonitor { + /** An array of child nodes, each representing an element of the source array. */ + private final GridToStringNode[] nodes; + + /** The rule for appending a hint about skipped elements if the array is too large. */ + private final LongSequenceSkipRule skipRule; + + /** The class object representing the type of the array. */ + private final Class<?> arrType; + + /** + * Constructs a new array node. + * Iterates over the input array, creates a node for each element, + * and populates the internal array up to the collection size limit. + * @param propName The property name. + * @param arr The source array. + * @param arrType The class object of the array's type. + */ + GridToStringArrayNode(String propName, Object[] arr, Class<?> arrType) { + super(propName, arr); + try { + aqcuireRecursionMonitor(this); + this.arrType = arrType; + skipRule = new LongSequenceSkipRule(() -> arr.length); + nodes = new GridToStringNode[Math.min(COLLECTION_LIMIT, arr.length)]; + for (int i = 0; i < nodes.length; i++) { + final int idx = i; + nodes[idx] = getGridToStringNode(null, () -> arr[idx], () -> arr[idx].getClass()); Review Comment: If an array element is `null`, `arr[idx].getClass()` will throw `NullPointerException` during node construction. Use a null-safe class supplier (e.g., return `Object.class` when the value is null), relying on `getGridToStringNode`’s existing null handling for the value itself. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/SBLengthLimit.java: ########## @@ -35,50 +35,44 @@ class SBLengthLimit { /** Length of head part of message. */ private static final int HEAD_LEN = MAX_TO_STR_LEN - TAIL_LEN; - /** */ - private int len; - - /** - * @return Current length. - */ - int length() { - return len; - } - - /** - * - */ - void reset() { - len = 0; - } - /** * @param sb String builder. * @param writtenLen Written length. */ void onWrite(SBLimitedLength sb, int writtenLen) { - len += writtenLen; - if (overflowed(sb) && (sb.getTail() == null || sb.getTail().length() == 0)) { - CircularStringBuilder tail = getTail(); - - int newSbLen = Math.min(sb.length(), HEAD_LEN + 1); + CircularStringBuilder tail = createTail(); + int newSbLen = Math.min(sb.length(), getHeadLengthLimit()); tail.append(sb.impl().substring(newSbLen)); - sb.setTail(tail); - sb.setLength(newSbLen); + sb.impl().setLength(newSbLen); } } - /** */ - CircularStringBuilder getTail() { - return new CircularStringBuilder(TAIL_LEN); + /** Creates empty tail + * @return empty tail */ + CircularStringBuilder createTail() { + return new CircularStringBuilder(getTailLengthLimit()); } /** - * @return {@code True} if reached limit. + * @return {@code True} if this string builder exceeds limit, false otherwise */ boolean overflowed(SBLimitedLength sb) { - return sb.impl().length() > HEAD_LEN; + return sb.length() >= getHeadLengthLimit(); + } Review Comment: `overflowed` is using `sb.length()` (which includes tail + skipped) and `>=`, which can mark the builder overflowed too early (exactly at the head limit) and can also keep it permanently overflowed once a tail exists. This should be based on the head buffer length (`sb.impl().length()`) and should match the intended threshold (typically `>` the head limit). ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/CircularStringBuilder.java: ########## @@ -170,13 +157,102 @@ private CircularStringBuilder appendNull() { return this; } + /** + * Inserts a string into the buffer at the specified logical offset. + * This method is optimized to minimize the number of elements moved by choosing + * to shift elements from the closest end (left or right) to the insertion point. + * + * @param offset The logical position (accounting for skipped characters) + * at which to insert. + * @param valToInsert The string to be inserted. + * @throws StringIndexOutOfBoundsException if the offset is invalid. + */ + public void insert(int offset, String valToInsert) { + int curLength = length(); + int offsetInsideBuf = offset - skipped; + if (offset < 0 || offsetInsideBuf > curLength) + throw new StringIndexOutOfBoundsException("Offset " + offset + " out of bounds for length " + curLength); + if (valToInsert == null) + valToInsert = "null"; + int insertLength = valToInsert.length(); + if (insertLength == 0) + return; + if (offsetInsideBuf == curLength) { + append(valToInsert); + return; + } + int spareSpace = value.length - curLength; + int insertCnt = Math.min(valToInsert.length(), spareSpace + offsetInsideBuf); + if (insertCnt <= 0) { + skipped += valToInsert.length(); + return; + } + int bufStartShiftedOffset = full ? (finishAt + 1) % value.length : 0; + int shiftedOffset = (bufStartShiftedOffset + offsetInsideBuf) % value.length; + int moveRightCnt = ((shiftedOffset <= finishAt ? 0 : curLength) + finishAt + 1) - shiftedOffset; + if (!full || offset - skipped > curLength / 2) { + shiftRight(insertCnt, moveRightCnt); + int charsToSkip = Math.max(0, insertCnt - spareSpace); + finishAt = (finishAt + insertCnt) % value.length; + shiftedOffset = (shiftedOffset + insertCnt) % value.length; + full = curLength + insertCnt >= value.length; + insertStringTail(valToInsert, shiftedOffset, insertCnt); + skipped += charsToSkip; + } + else { + int moveLeftCnt = (curLength - moveRightCnt - insertCnt); + shiftLeft(insertCnt, moveLeftCnt); + insertStringTail(valToInsert, shiftedOffset, insertCnt); + skipped += valToInsert.length(); + } + } + /** * @return Count of skipped elements. */ public int getSkipped() { return skipped; } + /** + * Returns a substring from the logical sequence of characters, accounting for + * the circular buffer structure and any skipped characters. + * + * <p>This method first validates the indices against the total logical length + * (skipped + visible characters). If the requested range is empty or fully within + * the skipped portion, an empty string is returned for efficiency. + * + * <p>It then calculates the physical indices in the internal array. If the + * substring wraps around the end of the circular buffer, it performs a two-part + * copy operation to assemble the result. + * + * @param beginIdx the beginning index, inclusive. + * @param endIdx the ending index, exclusive. + * @return a new String containing the specified subsequence. + * @throws StringIndexOutOfBoundsException if beginIdx or endIdx are negative, + * or if endIdx is greater than the total logical length. + * @throws IllegalArgumentException if beginIdx is greater than endIdx. + */ + public String substring(int beginIdx, int endIdx) { + if (beginIdx < 0 || endIdx < 0 || endIdx > skipped + length()) + throw new StringIndexOutOfBoundsException( + "Some of indexes is out of bounds. Begind index = " + beginIdx + " end index = " + endIdx); + if (beginIdx > endIdx) + throw new IllegalArgumentException( + "Begin index can not be greater then end index (begin = " + beginIdx + " end = " + endIdx + ")"); Review Comment: Exception messages contain multiple typos/grammar issues (e.g., \"Some of indexes\", \"Begind\", \"can not\", \"then\" vs \"than\"). Please correct them to improve diagnosability. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringNode.java: ########## @@ -0,0 +1,175 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.util.tostring; + +import java.util.IdentityHashMap; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; +import org.apache.ignite.internal.util.GridStringBuilder; + +import static org.apache.ignite.internal.util.tostring.NodeRecursionMonitor.OBJECT_REGISTRY; + +/** + * The abstract base class for all nodes in the string representation tree. + * Defines the common interface for appending a node's value to a string builder + * and provides static factory and utility methods for all subclasses. + */ +public abstract class GridToStringNode { + /** + * A thread-local cache for nodes, used to handle references of + * inner toString() calls by mapping temporary markers to actual nodes. + */ + public static final ConcurrentHashMap<Thread, IdentityHashMap<String, GridToStringNode>> CATCHED_NODES + = new ConcurrentHashMap<>(); Review Comment: `CATCHED_NODES` appears to be a misspelling and reduces readability/maintainability. Consider renaming to `CACHED_NODES` (and updating references). If binary compatibility matters, consider keeping the old name as a deprecated alias. ########## modules/core/src/test/java/org/apache/ignite/internal/util/tostring/GridToStringBuilderSelfTest.java: ########## @@ -204,6 +205,106 @@ public void testToStringCheckObjectRecursionPrevention() throws Exception { fut4.get(3_000); } + + /** */ + @Test + public void testNPE() { + boolean success; + try { + SBLimitedLength sbLimitedLength = new SBLimitedLength(256); + sbLimitedLength.initLimit(new SBLengthLimit()); + sbLimitedLength.a("a".repeat(7999)); + sbLimitedLength.i(7000, "asd"); + sbLimitedLength.a("a".repeat(10)); + success = true; + } catch (NullPointerException ignored) { + success = false; + } + Assert.assertTrue(success); + } Review Comment: This test should fail naturally if an exception is thrown; wrapping in a boolean + catching only `NullPointerException` can hide other failures and reduces usefulness of the stack trace. Prefer an assertion that the block does not throw (or omit the try/catch entirely) so any exception fails the test with full diagnostics. -- 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]
