anton-vinogradov commented on code in PR #13262: URL: https://github.com/apache/ignite/pull/13262#discussion_r3475224761
########## 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. **Suggested fix** — fail fast instead of shipping a placeholder to logs: ```suggestion default -> throw new IllegalStateException("Unexpected field descriptor type: " + fd.type()); ``` ########## 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. **Suggested fix** — lazily create `tail` so this branch can't NPE: ```suggestion if (tail == null) tail = lenLimit.createTail(); tail.insert(offset - headLengthLimit, str); ``` ########## 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`. **Suggested fix** (typo): ```suggestion throw new UnsupportedOperationException("setLength is not supported by this implementation"); ``` ########## 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`. **Suggested fix** (typo): ```suggestion "Some of indexes is out of bounds. Begin index = " + beginIdx + " end index = " + endIdx); ``` ########## 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. **Suggested fix:** ```suggestion List<GridToStringNode> result = new ArrayList<>(); ``` _Note: also add `import java.util.ArrayList;` — applying this single-line suggestion won't add the import._ ########## 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`. **Suggested fix** (typo): ```suggestion void acquireRecursionMonitor(NodeRecursionMonitor node) { ``` _Note: also rename the call site in `GridToStringObjectNode` — a single-line apply won't update it._ -- 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]
