anton-vinogradov commented on code in PR #13262: URL: https://github.com/apache/ignite/pull/13262#discussion_r3475224725
########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringNode.java: ########## @@ -0,0 +1,195 @@ +/* + * 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. + */ + static final ConcurrentHashMap<Thread, IdentityHashMap<String, GridToStringNode>> CATCHED_NODES + = new ConcurrentHashMap<>(); + + /** Last constructed node. */ + static final ThreadLocal<GridToStringNode> LAST_CONSTRUCTED_GRID_TO_STRING_NODE = new ThreadLocal<>(); + + /** The name of the property this node represents. */ + String propName; + + /** Inner buffer. For inner calls. */ + StringBuilder innerBuf = new StringBuilder(0); + + /** + * Base constructor. + * @param propName The name of the property. + */ + GridToStringNode(String propName) { + this.propName = propName; + } + + /** + * Creates a root node for a string value. + * @param str The string value. + * @param addNodes Additional child nodes to include. + * @return A new root node. + */ + static GridToStringNode getRootNode(String str, List<GridToStringNode> addNodes) { + return new GridToStringObjectNode(str, addNodes); + } + + /** + * Creates a root node for an object. + * Checks for recursion and creates either a termination node or a full object node. + * @param obj The object to represent. + * @param cls The class of the object. + * @param addNodes Additional child nodes to include. + * @return A new root node. + */ + static GridToStringNode getRootNode(Object obj, Class<?> cls, List<GridToStringNode> addNodes) { + return recursionTermination(obj) + .orElseGet(() -> new GridToStringObjectNode(null, obj, cls, addNodes)); + } + + /** + * Marks a node in the thread-local cache with a unique, empty string key. + * Used to handle references during object graph traversal. + * @param node The node to mark. + * @return The unique marker string. + */ + static String markNode(GridToStringNode node) { + String result = new String(GridToStringNode.class.getSimpleName()); Review Comment: Two problems on this method: 1. Using `new String(...)` identity as a map key is extremely fragile (see the wrapped-`toString` case). 2. `orElseThrow()` makes `markNode` throw if the thread state wasn't initialized (a nested call on a path where `init()` didn't run). `toString()` must never throw — the original guarded every path with try/catch; this reintroduces throwing from `toString`. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringNode.java: ########## @@ -0,0 +1,195 @@ +/* + * 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. + */ + static final ConcurrentHashMap<Thread, IdentityHashMap<String, GridToStringNode>> CATCHED_NODES + = new ConcurrentHashMap<>(); + + /** Last constructed node. */ + static final ThreadLocal<GridToStringNode> LAST_CONSTRUCTED_GRID_TO_STRING_NODE = new ThreadLocal<>(); + + /** The name of the property this node represents. */ + String propName; + + /** Inner buffer. For inner calls. */ + StringBuilder innerBuf = new StringBuilder(0); + + /** + * Base constructor. + * @param propName The name of the property. + */ + GridToStringNode(String propName) { + this.propName = propName; + } + + /** + * Creates a root node for a string value. + * @param str The string value. + * @param addNodes Additional child nodes to include. + * @return A new root node. + */ + static GridToStringNode getRootNode(String str, List<GridToStringNode> addNodes) { + return new GridToStringObjectNode(str, addNodes); + } + + /** + * Creates a root node for an object. + * Checks for recursion and creates either a termination node or a full object node. + * @param obj The object to represent. + * @param cls The class of the object. + * @param addNodes Additional child nodes to include. + * @return A new root node. + */ + static GridToStringNode getRootNode(Object obj, Class<?> cls, List<GridToStringNode> addNodes) { + return recursionTermination(obj) + .orElseGet(() -> new GridToStringObjectNode(null, obj, cls, addNodes)); + } + + /** + * Marks a node in the thread-local cache with a unique, empty string key. + * Used to handle references during object graph traversal. + * @param node The node to mark. + * @return The unique marker string. + */ + static String markNode(GridToStringNode node) { + String result = new String(GridToStringNode.class.getSimpleName()); + identities() + .orElseThrow() + .put(result, node); + return result; + } + + /** + * Appends inner buffer to last created node. + * @param node Node - new node to fill inner buffer. + */ + static String appendInnerBuffer(GridToStringNode node) { + GridToStringNode parent = LAST_CONSTRUCTED_GRID_TO_STRING_NODE.get(); + parent.innerBuf.append(node.toString()); + return ""; + } + + /** + * Checks if the current context is new, meaning no recursion or inner calls are in progress. + * @return True if the context is new; false otherwise. + */ + static boolean init() { + Thread curThread = Thread.currentThread(); + boolean containsThread = CATCHED_NODES.containsKey(curThread); + if (!containsThread) + CATCHED_NODES.put(curThread, new IdentityHashMap<>()); + return !containsThread; + } + + /** + * Creates a node for a null value if the object is null. + * @param obj The object to check. + * @return An Optional containing a null node if the object is null; otherwise, an empty Optional. + */ + static Optional<GridToStringNode> nullNode(Object obj) { + return obj == null ? Optional.of(new GridToStringNullNode(null)) : Optional.empty(); + } + + /** + * Checks if an object is part of a recursive reference and, if so, + * creates a termination node to break the cycle. + * @param obj The object to check. + * @return An Optional containing a termination node if recursion is detected; + * otherwise, an empty Optional. + */ + static Optional<GridToStringNode> recursionTermination(Object obj) { + return Optional.ofNullable(obj) + .flatMap(NodeRecursionMonitor::findRecursionMonitor) + .map(sameObjNode -> + GridToStringRecursionTerminationNode.of(sameObjNode, obj)); + } + + /** + * Appends the property name (if any) and a placeholder for the value to the builder. + * This method is intended to be overridden by subclasses to provide specific value output. + * @param sb The string builder to append to. + */ + void appendNode(GridStringBuilder sb) { + if (propName != null) + sb.a(propName).a("="); + } + + /** + * Clears the thread-local cache of nodes and recursion prevention storage. + */ + static void clear() { + CATCHED_NODES.remove(Thread.currentThread()); + OBJECT_REGISTRY.remove(); + LAST_CONSTRUCTED_GRID_TO_STRING_NODE.remove(); + } + + /** + * Generates the final string representation of this node. + * Initializes a limited-length string builder, appends the node's content, + * clears the cache, and returns the result. + * @return The string representation of the node. + */ + @Override public String toString() { + SBLimitedLength sb = new SBLimitedLength(256); + sb.initLimit(new SBLengthLimit()); + appendNode(sb); + return sb.toString(); + } + + /** Retrieves identity hash map if it was initialized earlier + * @return Optional identity hash map that stores catched nodes + * */ + static Optional<IdentityHashMap<String, GridToStringNode>> identities() { + return Optional.ofNullable(CATCHED_NODES.get(Thread.currentThread())); + } + + /** + * If someone decided to concat outside of default + * {@link org.apache.ignite.internal.util.typedef.internal.S#toString} + * we should replace the substring + * @param obj Object. + */ + public static <T> T recoverObject(T obj) { Review Comment: `recoverObject` is a band-aid for the marker design, and it only handles the case where the *entire* returned string is the marker — not the `prefix + marker + suffix` concatenation, which is exactly the dangerous case. The javadoc ('if someone decided to concat outside of default S.toString') is effectively an admission the mechanism can't be made correct. It's also now invoked on every `a(...)`/`i(...)` in `SBLimitedLength`, adding an identity-map lookup to every append on the render path. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringBuilder.java: ########## @@ -1030,148 +773,33 @@ private static void handleOverflow(SBLimitedLength buf, int size) { */ private static <T> String toStringImpl( Class<T> cls, - SBLimitedLength buf, T obj, Object[] addNames, Object[] addVals, @Nullable boolean[] addSens, int addLen) { assert cls != null; - assert buf != null; assert obj != null; assert addNames != null; assert addVals != null; assert addNames.length == addVals.length; assert addLen <= addNames.length; - - boolean newStr = buf.length() == 0; - - IdentityHashMap<Object, EntryReference> svdObjs = savedObjects.get(); - - if (newStr) - svdObjs.put(obj, new EntryReference(buf.length())); - + boolean isNew = GridToStringNode.init(); try { - int len = buf.length(); - - String s = toStringImpl0(cls, buf, obj, addNames, addVals, addSens, addLen); - - if (newStr) - return s; - - buf.setLength(len); - - return s.substring(len); + List<GridToStringNode> addNodes = + GridToStringNodeFactory.getNodes(addNames, addVals, addSens, addLen); + GridToStringNode node = GridToStringNode.getRootNode(obj, cls, addNodes); + return isNew ? node.toString() : GridToStringNode.markNode(node); Review Comment: Core of the marker mechanism: every non-root `S.toString()` now returns a placeholder marker instead of its value, stitched back later by String identity. This only holds if the caller's `toString()` returns *exactly* that instance untouched. As soon as a class does `"Foo{" + S.toString(Foo.class, this) + "}"` (common across the codebase), the marker is embedded in a larger string, identity recovery fails, and the literal `GridToStringNode` leaks into the log. This silently corrupts output and is the main reason I'd not take the rewrite. ########## 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); + else if (val instanceof Collection) + return new GridToStringCollectionNode(childPropName, (Collection<?>)val); + else if (val instanceof Map) + return new GridToStringMapNode(childPropName, (Map<?, ?>)val); + + String toStrResult = val.toString(); Review Comment: This is where wrapped/custom `toString()` breaks: if `val.toString()` returns anything other than the exact marker instance, `recoverOrCreate` can't match it and the marker text is treated as a literal value. Net effect for a class that wraps `S.toString()`: the inner object renders as `GridToStringNode`. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringNode.java: ########## @@ -0,0 +1,195 @@ +/* + * 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. + */ + static final ConcurrentHashMap<Thread, IdentityHashMap<String, GridToStringNode>> CATCHED_NODES Review Comment: `static ConcurrentHashMap<Thread, ...>` keyed by `Thread` is a classic leak: in a pooled-thread server the keys never die, and any path that skips `clear()` (exception in a user `toString`, reentrancy) accumulates entries that pin the `Thread` and all captured nodes/objects. The value is a per-thread map touched only by its owner, so the `ConcurrentHashMap` buys nothing but overhead. This should be a `ThreadLocal` — which is what the original used and why it didn't leak. (nit: `CATCHED` -> `CACHED`.) ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringBuilder.java: ########## @@ -1713,92 +1271,37 @@ public static String toString(String str, Object... triplets) { propSens[i] = (Boolean)sens; } - - SBLimitedLength sb = threadLocSB.get(); - - boolean newStr = sb.length() == 0; - - try { - return toStringImpl(str, sb, propNames, propVals, propSens, propCnt); - } - finally { - if (newStr) - sb.reset(); - } + return toStringImpl(str, propNames, propVals, propSens, propCnt); } /** * Creates an uniformed string presentation for the binary-like object. * * @param str Output prefix or {@code null} if empty. - * @param buf String builder buffer. * @param propNames Names of object properties. * @param propVals Property values. * @param propSens Sensitive flag of values or {@code null} if all values is not sensitive. * @param propCnt Properties count. * @return String presentation of the object. */ - private static String toStringImpl(String str, SBLimitedLength buf, Object[] propNames, Object[] propVals, + private static String toStringImpl(String str, Object[] propNames, Object[] propVals, boolean[] propSens, int propCnt) { - - boolean newStr = buf.length() == 0; - - if (str != null) - buf.a(str).a(" "); - - buf.a("["); - - appendVals(buf, true, propNames, propVals, propSens, propCnt); - - buf.a(']'); - - if (newStr) - return buf.toString(); - - // Called from another GTSB.toString(), so this string is already in the buffer and shouldn't be returned. - return ""; - } - - /** - * Append additional values to the buffer. - * - * @param buf Buffer. - * @param first First value flag. - * @param addNames Names of additional values to be included. - * @param addVals Additional values to be included. - * @param addSens Sensitive flag of values or {@code null} if all values are not sensitive. - * @param addLen How many additional values will be included. - */ - private static void appendVals(SBLimitedLength buf, - boolean first, - Object[] addNames, - Object[] addVals, - boolean[] addSens, - int addLen - ) { - if (addLen > 0) { - for (int i = 0; i < addLen; i++) { - Object addVal = addVals[i]; - - if (addVal != null) { - if (addSens != null && addSens[i] && !includeSensitive()) - continue; - - GridToStringInclude incAnn = addVal.getClass().getAnnotation(GridToStringInclude.class); - - if (incAnn != null && incAnn.sensitive() && !includeSensitive()) - continue; - } - - if (!first) - buf.a(", "); - else - first = false; - - buf.a(addNames[i]).a('='); - - toString(buf, addVal); - } + boolean isNew = GridToStringNode.init(); + try { + List<GridToStringNode> addNodes = + GridToStringNodeFactory.getNodes(propNames, propVals, propSens, propCnt); + GridToStringNode node = GridToStringNode.getRootNode(str, addNodes); + return isNew ? node.toString() : GridToStringNode.appendInnerBuffer(node); Review Comment: Note the string-property path uses a *different* nested-call mechanism (`appendInnerBuffer` mutating the `LAST_CONSTRUCTED_..` node's `innerBuf`) than the object path (`markNode`). Two parallel, order-dependent mechanisms for 'nested call' is a lot of surface area and another place reentrancy can corrupt output. ########## modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringBuilder.java: ########## @@ -1030,148 +773,33 @@ private static void handleOverflow(SBLimitedLength buf, int size) { */ private static <T> String toStringImpl( Class<T> cls, - SBLimitedLength buf, T obj, Object[] addNames, Object[] addVals, @Nullable boolean[] addSens, int addLen) { assert cls != null; - assert buf != null; assert obj != null; assert addNames != null; assert addVals != null; assert addNames.length == addVals.length; assert addLen <= addNames.length; - - boolean newStr = buf.length() == 0; - - IdentityHashMap<Object, EntryReference> svdObjs = savedObjects.get(); - - if (newStr) - svdObjs.put(obj, new EntryReference(buf.length())); - + boolean isNew = GridToStringNode.init(); Review Comment: The whole scheme depends on perfectly balanced `init()`/`clear()` across arbitrary nested user `toString()` calls; any imbalance leaks thread state into `CATCHED_NODES`. The original `savedObjects` map was balanced with a simple `put`/`remove(obj)` in `finally` per object and degraded gracefully — this global init/clear is much easier to get wrong. ########## 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); Review Comment: The `offset < headLengthLimit` branch above got a `tail == null` guard (good — that's the actual NPE fix), but this `else` branch dereferences `tail` with no guard. If an insert ever targets an offset past the head while `tail` is still null, the same NPE class is back. Add the lazy-create here too, or assert the invariant. ########## 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"); Review Comment: `d(...)`, `r(...)`, `setLength(...)` now throw `UnsupportedOperationException`, widening the public surface of `GridStringBuilder` with throwing methods. `setLength` in particular was used by the old `toStringImpl` rollback path, so making it throw is a behavioral change. Another reason to keep this class minimal rather than expand it. ########## 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<>(); Review Comment: nit: `ArrayList` is a better default here (indexed iteration, less per-node overhead). `LinkedList` + `Collections.unmodifiableList` per object adds up on the hot path. ########## 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"); Review Comment: Any field-descriptor type not covered by the switch silently emits this placeholder into production logs. The original had no such trap. At minimum this should `assert`/throw in tests rather than ship a human-readable 'toString is not implemented yet' string to logs. ########## 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: nit: typo `imlementation` -> `implementation`. ########## 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()); + } Review Comment: Worth a careful look: `overflowed()` compares `length() >= HEAD_LEN`, but this overridden `length()` now includes `tail.getSkipped() + tail.length()`, while `onWrite`/`i(...)` compute substrings against head-only `impl().length()`. Mixing the composite `length()` with head-only `impl().length()` for position math is easy to get subtly wrong — please double-check the boundary arithmetic in `i(int, String)`. ########## 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); Review Comment: nit: typo `Begind index` -> `Begin index`. ########## 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) { Review Comment: nit: typo `aqcuireRecursionMonitor` -> `acquireRecursionMonitor`. ########## 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) { Review Comment: This is the correct fix for the NPE. My suggestion is to ship *this* (the insert overrides + lazy `tail`) plus the `RecursivePayload` test as the entire patch, drop `recoverObject` from it, and split the node-tree work into a separate ticket/RFC. -- 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]
