JDK 21 modernization #3
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- src/main/java/org/azeckoski/reflectutils/FieldUtils.java:335-1052 now relies solely on standard collections, removes the BeanUtils adapter paths, and fixes indexed/mapped setters so populate-from-params works again on JDK 21. - src/main/java/org/azeckoski/reflectutils/DeepUtils.java:233-268 and ReflectUtilTest companions regained their behaviour by delegating to the updated FieldUtils, restoring all populate-from-params results. - src/main/java/org/azeckoski/reflectutils/ClassData.java:16-208 safely handles inaccessible JDK internals by catching InaccessibleObjectException, keeping reflective caches functional without illegal opens. Dependency & Codec Refresh - pom.xml:93-112 adds Jackson databind/XML (and drops commons-beanutils) to power the new transcoders. - src/main/java/org/azeckoski/reflectutils/transcoders/JSONTranscoder.java:1-92 and XMLTranscoder.java:1-123 wrap Jackson APIs; the XML decoder now post-processes numeric/boolean strings back into their native types. - New JUnit 5 coverage for the codecs lives in src/test/java/org/azeckoski/reflectutils/TranscodersTest.java:1-68. Collection Defaults & Converters - src/main/java/org/azeckoski/reflectutils/ConstructorUtils.java:432-448 selects ArrayList/LinkedHashSet/LinkedHashMap when instantiating interfaces; tests align in src/test/java/org/azeckoski/reflectutils/ ConstructorUtilsTest.java:166-181. - src/main/java/org/azeckoski/reflectutils/ConversionUtils.java:117-160 and CollectionConverter.java:17-46 now use ConcurrentHashMap/ArrayList so conversions stay thread-safe without custom map implementations. Docs & Tooling - README.md:1-45 and AGENTS.md:1-30 document the JDK 21 toolchain and Jackson-based workflow; .java-version pins the local runtime to 21. - GitHub Actions (.github/workflows/ci.yml:1-23) runs mvn clean verify on Temurin 21. Tests - mvn clean verify Everything now builds and the full JUnit suite passes on JDK 21 with no access violations.
WalkthroughModernizes project to Java 21, replaces legacy BeanUtils/refmap/lifecycle features, adopts Jackson for JSON/XML, updates collections to standard JDK types, adds GitHub Actions CI, refreshes documentation, and migrates tests to JUnit 5. Internal reflection/access and construction logic refined; numerous obsolete classes, HTML docs, and custom maps removed. Changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/azeckoski/reflectutils/ConversionUtils.java (1)
185-193: Restore thread-safe variable converter list
variableConvertersused to be backed by a thread-safeVector. Replacing it with a plainArrayListintroduces data races: concurrentconvert(...)calls iterate the list while other threads may add/clear converters, leading to stale reads orConcurrentModificationException. We need a concurrency-safe collection here.Apply this diff to switch to
CopyOnWriteArrayList:-import java.util.ArrayList; +import java.util.ArrayList; ... -import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; ... - if (variableConverters == null) { - variableConverters = new ArrayList<VariableConverter>(); + if (variableConverters == null) { + variableConverters = new CopyOnWriteArrayList<>();
🧹 Nitpick comments (6)
src/main/java/org/azeckoski/reflectutils/ClassDataCacher.java (1)
119-119: Consider ConcurrentHashMap for better concurrent read performance.The
Collections.synchronizedMap(new WeakHashMap<>())approach is correct and provides thread-safety. However,ConcurrentHashMapwith weak keys (via a customWeakReferencewrapper or a library like Guava'sMapMaker) would offer better concurrent read performance by allowing multiple threads to read without blocking.For a reflection cache with mostly reads after warmup, this could improve throughput in multi-threaded scenarios. That said, the current implementation is functionally correct and simpler.
Example refactor using
ConcurrentHashMapwith custom weak key handling:-reflectionCache = Collections.synchronizedMap(new WeakHashMap<Class<?>, ClassFields>()); +// Note: This requires a custom WeakReference key wrapper or a library like Guava's Cache with weak keys +reflectionCache = new ConcurrentHashMap<>();Alternatively, if staying with standard JDK without custom wrappers, the current approach is reasonable.
src/test/java/org/azeckoski/reflectutils/ArrayUtilsTest.java (1)
35-69: Optional: prefer assertNull for null checks.Use assertNull(...) instead of assertEquals(null, ...) for clearer failure messages in this test.
src/main/java/org/azeckoski/reflectutils/DeepUtils.java (1)
346-356: Minor: pre-size LinkedHashMap when cloning beans to MAP to reduce rehashing.Build the map after you have the types to size it appropriately.
Apply this diff near the bean->MAP branch:
- if (CopyDestination.MAP.equals(dest)) { - copy = new LinkedHashMap<String, Object>(); + if (CopyDestination.MAP.equals(dest)) { // maps should pick up all readable fields if (ignoreTransient) { filter = FieldsFilter.SERIALIZABLE; } else { filter = FieldsFilter.READABLE; } } else { copy = getConstructorUtils().constructClass(beanClass); // make new bean } ClassFields<?> cf = getFieldUtils().analyzeClass(beanClass); Map<String, Class<?>> types = cf.getFieldTypes(filter); + if (CopyDestination.MAP.equals(dest)) { + copy = new LinkedHashMap<String, Object>(Math.max(16, types.size())); + }Also applies to: 359-361
src/test/java/org/azeckoski/reflectutils/FieldUtilsTest.java (1)
105-141: Prefer assertThrows and assertNull in tests.Replace try/catch with assertThrows(...) and assertEquals(null, ...) with assertNull(...) to modernize and improve clarity.
src/test/java/org/azeckoski/reflectutils/DeepUtilsTest.java (1)
318-319: Optional: adopt assertThrows and assertNull across this class.Modernize exception assertions and null checks for clearer failure output.
src/test/java/org/azeckoski/reflectutils/ReflectUtilTest.java (1)
133-141: Prefer assertThrows and assertNull style in tests.Replace try/catch with assertThrows(...) and assertEquals(null, ...) with assertNull(...) for cleaner tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
.github/workflows/ci.yml(1 hunks)AGENTS.md(1 hunks)README.md(1 hunks)pom.xml(1 hunks)readme.txt(0 hunks)src/main/java/org/azeckoski/reflectutils/ClassData.java(4 hunks)src/main/java/org/azeckoski/reflectutils/ClassDataCacher.java(3 hunks)src/main/java/org/azeckoski/reflectutils/ClassFields.java(7 hunks)src/main/java/org/azeckoski/reflectutils/ClassProperty.java(2 hunks)src/main/java/org/azeckoski/reflectutils/ConstructorUtils.java(5 hunks)src/main/java/org/azeckoski/reflectutils/ConversionUtils.java(4 hunks)src/main/java/org/azeckoski/reflectutils/DeepUtils.java(3 hunks)src/main/java/org/azeckoski/reflectutils/FieldUtils.java(12 hunks)src/main/java/org/azeckoski/reflectutils/Lifecycle.java(0 hunks)src/main/java/org/azeckoski/reflectutils/LifecycleManager.java(0 hunks)src/main/java/org/azeckoski/reflectutils/ReflectUtils.java(0 hunks)src/main/java/org/azeckoski/reflectutils/annotations/package.html(0 hunks)src/main/java/org/azeckoski/reflectutils/beanutils/DefaultFieldAdapter.java(0 hunks)src/main/java/org/azeckoski/reflectutils/beanutils/DynaBeanAdapter.java(0 hunks)src/main/java/org/azeckoski/reflectutils/beanutils/FieldAdapter.java(0 hunks)src/main/java/org/azeckoski/reflectutils/beanutils/FieldAdapterManager.java(0 hunks)src/main/java/org/azeckoski/reflectutils/beanutils/package.html(0 hunks)src/main/java/org/azeckoski/reflectutils/converters/CollectionConverter.java(3 hunks)src/main/java/org/azeckoski/reflectutils/converters/DateConverter.java(2 hunks)src/main/java/org/azeckoski/reflectutils/converters/MapConverter.java(3 hunks)src/main/java/org/azeckoski/reflectutils/converters/NumberConverter.java(4 hunks)src/main/java/org/azeckoski/reflectutils/converters/package.html(0 hunks)src/main/java/org/azeckoski/reflectutils/map/ArrayOrderedMap.java(0 hunks)src/main/java/org/azeckoski/reflectutils/map/OrderedMap.java(0 hunks)src/main/java/org/azeckoski/reflectutils/map/package.html(0 hunks)src/main/java/org/azeckoski/reflectutils/package.html(0 hunks)src/main/java/org/azeckoski/reflectutils/refmap/FinalizableReference.java(0 hunks)src/main/java/org/azeckoski/reflectutils/refmap/FinalizableReferenceQueue.java(0 hunks)src/main/java/org/azeckoski/reflectutils/refmap/FinalizableSoftReference.java(0 hunks)src/main/java/org/azeckoski/reflectutils/refmap/FinalizableWeakReference.java(0 hunks)src/main/java/org/azeckoski/reflectutils/refmap/Finalizer.java(0 hunks)src/main/java/org/azeckoski/reflectutils/refmap/ReferenceMap.java(0 hunks)src/main/java/org/azeckoski/reflectutils/refmap/ReferenceType.java(0 hunks)src/main/java/org/azeckoski/reflectutils/refmap/package.html(0 hunks)src/main/java/org/azeckoski/reflectutils/transcoders/HTMLTranscoder.java(0 hunks)src/main/java/org/azeckoski/reflectutils/transcoders/JSONTranscoder.java(1 hunks)src/main/java/org/azeckoski/reflectutils/transcoders/Transcoder.java(1 hunks)src/main/java/org/azeckoski/reflectutils/transcoders/TranscoderUtils.java(0 hunks)src/main/java/org/azeckoski/reflectutils/transcoders/XMLTranscoder.java(1 hunks)src/main/java/org/azeckoski/reflectutils/transcoders/package.html(0 hunks)src/main/java/overview.html(0 hunks)src/test/java/org/azeckoski/reflectutils/ArrayUtilsTest.java(2 hunks)src/test/java/org/azeckoski/reflectutils/ClassFieldsTest.java(36 hunks)src/test/java/org/azeckoski/reflectutils/ConstructorUtilsTest.java(19 hunks)src/test/java/org/azeckoski/reflectutils/ConversionUtilsTest.java(10 hunks)src/test/java/org/azeckoski/reflectutils/DeepUtilsTest.java(13 hunks)src/test/java/org/azeckoski/reflectutils/FieldUtilsTest.java(22 hunks)src/test/java/org/azeckoski/reflectutils/ReflectUtilTest.java(36 hunks)src/test/java/org/azeckoski/reflectutils/TranscodersTest.java(1 hunks)src/test/java/org/azeckoski/reflectutils/map/ArrayOrderedMapTest.java(0 hunks)
💤 Files with no reviewable changes (28)
- src/main/java/org/azeckoski/reflectutils/ReflectUtils.java
- src/main/java/org/azeckoski/reflectutils/package.html
- src/main/java/org/azeckoski/reflectutils/map/OrderedMap.java
- src/main/java/org/azeckoski/reflectutils/beanutils/FieldAdapterManager.java
- readme.txt
- src/test/java/org/azeckoski/reflectutils/map/ArrayOrderedMapTest.java
- src/main/java/org/azeckoski/reflectutils/LifecycleManager.java
- src/main/java/org/azeckoski/reflectutils/transcoders/TranscoderUtils.java
- src/main/java/org/azeckoski/reflectutils/transcoders/package.html
- src/main/java/overview.html
- src/main/java/org/azeckoski/reflectutils/beanutils/FieldAdapter.java
- src/main/java/org/azeckoski/reflectutils/refmap/FinalizableSoftReference.java
- src/main/java/org/azeckoski/reflectutils/refmap/FinalizableReference.java
- src/main/java/org/azeckoski/reflectutils/refmap/FinalizableReferenceQueue.java
- src/main/java/org/azeckoski/reflectutils/refmap/FinalizableWeakReference.java
- src/main/java/org/azeckoski/reflectutils/Lifecycle.java
- src/main/java/org/azeckoski/reflectutils/beanutils/DefaultFieldAdapter.java
- src/main/java/org/azeckoski/reflectutils/refmap/Finalizer.java
- src/main/java/org/azeckoski/reflectutils/transcoders/HTMLTranscoder.java
- src/main/java/org/azeckoski/reflectutils/beanutils/DynaBeanAdapter.java
- src/main/java/org/azeckoski/reflectutils/map/package.html
- src/main/java/org/azeckoski/reflectutils/map/ArrayOrderedMap.java
- src/main/java/org/azeckoski/reflectutils/beanutils/package.html
- src/main/java/org/azeckoski/reflectutils/refmap/ReferenceType.java
- src/main/java/org/azeckoski/reflectutils/refmap/package.html
- src/main/java/org/azeckoski/reflectutils/converters/package.html
- src/main/java/org/azeckoski/reflectutils/annotations/package.html
- src/main/java/org/azeckoski/reflectutils/refmap/ReferenceMap.java
🧰 Additional context used
🧬 Code graph analysis (5)
src/main/java/org/azeckoski/reflectutils/FieldUtils.java (4)
src/main/java/org/azeckoski/reflectutils/ConstructorUtils.java (1)
ConstructorUtils(53-859)src/main/java/org/azeckoski/reflectutils/ClassFields.java (1)
ClassFields(54-1170)src/main/java/org/azeckoski/reflectutils/ArrayUtils.java (1)
ArrayUtils(28-321)src/main/java/org/azeckoski/reflectutils/ReflectUtils.java (1)
ReflectUtils(70-763)
src/test/java/org/azeckoski/reflectutils/ReflectUtilTest.java (1)
src/main/java/org/azeckoski/reflectutils/ReflectUtils.java (1)
ReflectUtils(70-763)
src/main/java/org/azeckoski/reflectutils/transcoders/XMLTranscoder.java (2)
src/main/java/org/azeckoski/reflectutils/TypeReference.java (1)
TypeReference(27-51)src/main/java/org/azeckoski/reflectutils/converters/MapConverter.java (1)
SuppressWarnings(34-86)
src/test/java/org/azeckoski/reflectutils/ConstructorUtilsTest.java (2)
src/test/java/org/azeckoski/reflectutils/classes/TestNoPubConstructor.java (1)
TestNoPubConstructor(22-38)src/main/java/org/azeckoski/reflectutils/ConstructorUtils.java (1)
ConstructorUtils(53-859)
src/main/java/org/azeckoski/reflectutils/transcoders/JSONTranscoder.java (1)
src/main/java/org/azeckoski/reflectutils/TypeReference.java (1)
TypeReference(27-51)
🔇 Additional comments (16)
src/main/java/org/azeckoski/reflectutils/ClassDataCacher.java (2)
19-21: LGTM! Standard library imports replace custom refmap infrastructure.The addition of
CollectionsandWeakHashMapimports aligns with the PR's goal of modernizing to standard JDK collections and removing the custom ReferenceMap implementation.
134-136: LGTM! Null-check prevents potential NPE.The addition of the null-check before clearing
this.reflectionCacheis a defensive programming improvement that prevents a potentialNullPointerExceptionif the cache hasn't been initialized yet whensetReflectionCacheis called with a non-null argument.src/main/java/org/azeckoski/reflectutils/converters/MapConverter.java (1)
19-19: Approve LinkedHashMap migration
Confirmed no remainingArrayOrderedMapreferences and nomap.get(<numeric>)usages—LinkedHashMap preserves insertion order and is safe here.src/main/java/org/azeckoski/reflectutils/converters/CollectionConverter.java (2)
17-18: LGTM! Modernized import from Vector to ArrayList.This change aligns with the broader JDK 21 modernization effort, replacing the legacy synchronized Vector with the standard ArrayList.
36-36: LGTM! Default collection type modernized from Vector to ArrayList.The switch from Vector to ArrayList is appropriate:
- Vector's synchronization overhead is unnecessary for most use cases
- ArrayList is the standard choice for non-thread-safe lists
- Return type remains the Collection interface, preserving API compatibility
Also applies to: 48-48
src/main/java/org/azeckoski/reflectutils/converters/DateConverter.java (2)
133-133: LGTM! Replaced deprecated Long constructor with parseLong.The change from
new Long(stringValue)toLong.parseLong(stringValue)correctly modernizes the code by avoiding the deprecated constructor (since JDK 9) and returns a primitive long suitable for the subsequent numeric comparisons.
160-160: LGTM! Cleaner use of local variable.Using the
calendarlocal variable (assigned at line 158) instead of re-casting is a minor readability improvement with no functional change.src/main/java/org/azeckoski/reflectutils/converters/NumberConverter.java (1)
145-145: LGTM! Replaced deprecated wrapper constructors with valueOf.All six wrapper type conversions correctly modernized:
new Byte(...)→Byte.valueOf(...)new Short(...)→Short.valueOf(...)new Integer(...)→Integer.valueOf(...)new Long(...)→Long.valueOf(...)new Float(...)→Float.valueOf(...)new Double(...)→Double.valueOf(...)The valueOf factory methods are the recommended replacements for the deprecated constructors (since JDK 9) and enable caching for small values. This change aligns with the existing String-to-Number conversion logic (lines 242-267) which already uses valueOf.
Also applies to: 159-159, 173-173, 178-178, 187-187, 192-192
src/test/java/org/azeckoski/reflectutils/ArrayUtilsTest.java (1)
20-36: JUnit 5 migration looks good.Imports, annotations, and class declaration are correct and consistent with Jupiter.
src/test/java/org/azeckoski/reflectutils/FieldUtilsTest.java (1)
17-20: JUnit 5 migration LGTM.Imports, annotations, and class declaration are consistent across the file.
Also applies to: 34-34
src/test/java/org/azeckoski/reflectutils/DeepUtilsTest.java (3)
27-30: JUnit 5 migration looks good.Imports, annotations, and class declaration are correct.
Also applies to: 45-45
455-456: Using LinkedHashMap in tests matches production changes.Good alignment with DeepUtils’s MAP cloning behavior.
247-257: Wrapper-based expectations for numeric fields are correct.Using Long.valueOf(...) avoids primitive/wrapper surprises in assertions.
src/test/java/org/azeckoski/reflectutils/ReflectUtilTest.java (2)
55-55: Numeric wrapper assertions are correct post-modernization.Using Long.valueOf/Integer.valueOf in expectations aligns with API returning wrappers.
Also applies to: 64-64, 116-116, 267-269, 308-308, 559-559, 1028-1029, 1069-1069
17-20: JUnit 5 migration LGTM – no legacy imports found
Automated scans found noimport junit.frameworkorimport org.junit(non-Jupiter); please manually confirm no JUnit 3/4 annotations remain.src/main/java/org/azeckoski/reflectutils/DeepUtils.java (1)
309-311: Approve LinkedHashMap replacement. No remaining references to ArrayOrderedMap or commons-beanutils found.
Comment on lines
+994
to
+1013
| try { | ||
| int length = ArrayUtils.size((Object[]) indexedObject); | ||
| if (index >= length) { | ||
| indexedObject = ArrayUtils.resize((Object[]) indexedObject, index + 1); | ||
| setSimpleValue(obj, name, indexedObject); | ||
| } | ||
| } else { | ||
| // this better be a list | ||
| if (indexedObject == null | ||
| || ! List.class.isAssignableFrom(indexedObject.getClass())) { | ||
| throw new IllegalArgumentException("Field (" + name + ") does not appear to be indexed (not an array or a list): " | ||
| + (indexedObject == null ? "NULL" : indexedObject.getClass()) ); | ||
| Class<?> componentType = ArrayUtils.type((Object[]) indexedObject); | ||
| Object convert = ReflectUtils.getInstance().convert(value, componentType); | ||
| Array.set(indexedObject, index, convert); | ||
| } catch (Exception e) { | ||
| throw new IllegalArgumentException("Failed to set index (" + index + ") for array of size (" + Array.getLength(indexedObject) + ") to value: " + value, e); | ||
| } | ||
| } else { | ||
| if (!List.class.isAssignableFrom(indexedObject.getClass())) { | ||
| throw new IllegalArgumentException("Field (" + name + ") does not appear to be indexed (not an array or a list): " + indexedObject.getClass()); | ||
| } | ||
| List<Object> list = (List<Object>) indexedObject; | ||
| try { | ||
| if (index < 0) { | ||
| list.add(value); |
There was a problem hiding this comment.
Primitive arrays now throw ClassCastException in setIndexedValue.
Line 996 onwards casts indexedObject to Object[], but primitive arrays (e.g., int[]) are not Object[]. Any write through setFieldValue(bean, "ints[0]", 1) will now explode with ClassCastException, breaking previously working code. Please restore component handling via java.lang.reflect.Array so both primitive and object arrays remain supported.
- int length = ArrayUtils.size((Object[]) indexedObject);
+ int length = Array.getLength(indexedObject);
if (index >= length) {
- indexedObject = ArrayUtils.resize((Object[]) indexedObject, index + 1);
- setSimpleValue(obj, name, indexedObject);
+ Object resized = Array.newInstance(indexedObject.getClass().getComponentType(), index + 1);
+ System.arraycopy(indexedObject, 0, resized, 0, length);
+ indexedObject = resized;
+ setSimpleValue(obj, name, indexedObject);
}
- Class<?> componentType = ArrayUtils.type((Object[]) indexedObject);
+ Class<?> componentType = indexedObject.getClass().getComponentType();
Object convert = ReflectUtils.getInstance().convert(value, componentType);
Array.set(indexedObject, index, convert);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| int length = ArrayUtils.size((Object[]) indexedObject); | |
| if (index >= length) { | |
| indexedObject = ArrayUtils.resize((Object[]) indexedObject, index + 1); | |
| setSimpleValue(obj, name, indexedObject); | |
| } | |
| } else { | |
| // this better be a list | |
| if (indexedObject == null | |
| || ! List.class.isAssignableFrom(indexedObject.getClass())) { | |
| throw new IllegalArgumentException("Field (" + name + ") does not appear to be indexed (not an array or a list): " | |
| + (indexedObject == null ? "NULL" : indexedObject.getClass()) ); | |
| Class<?> componentType = ArrayUtils.type((Object[]) indexedObject); | |
| Object convert = ReflectUtils.getInstance().convert(value, componentType); | |
| Array.set(indexedObject, index, convert); | |
| } catch (Exception e) { | |
| throw new IllegalArgumentException("Failed to set index (" + index + ") for array of size (" + Array.getLength(indexedObject) + ") to value: " + value, e); | |
| } | |
| } else { | |
| if (!List.class.isAssignableFrom(indexedObject.getClass())) { | |
| throw new IllegalArgumentException("Field (" + name + ") does not appear to be indexed (not an array or a list): " + indexedObject.getClass()); | |
| } | |
| List<Object> list = (List<Object>) indexedObject; | |
| try { | |
| if (index < 0) { | |
| list.add(value); | |
| try { | |
| int length = Array.getLength(indexedObject); | |
| if (index >= length) { | |
| Object resized = Array.newInstance( | |
| indexedObject.getClass().getComponentType(), | |
| index + 1 | |
| ); | |
| System.arraycopy(indexedObject, 0, resized, 0, length); | |
| indexedObject = resized; | |
| setSimpleValue(obj, name, indexedObject); | |
| } | |
| Class<?> componentType = indexedObject.getClass().getComponentType(); | |
| Object convert = ReflectUtils.getInstance().convert(value, componentType); | |
| Array.set(indexedObject, index, convert); | |
| } catch (Exception e) { | |
| throw new IllegalArgumentException( | |
| "Failed to set index (" + index + ") for array of size (" | |
| Array.getLength(indexedObject) | |
| ") to value: " + value, e | |
| ); | |
| } |
Comment on lines
+112
to
+134
| if (value instanceof String s) { | ||
| if (s.matches("[-+]?\\d+")) { | ||
| try { | ||
| ReflectUtils.getInstance().setFieldValue(container.getContainer(), key, value); | ||
| } catch (RuntimeException e) { | ||
| throw new RuntimeException("Unknown container type ("+type+") and could not set field on container: " + container, e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private Stack<String> tagStack = new Stack<String>(); | ||
| private Stack<Container> containerStack = new Stack<Container>(); | ||
|
|
||
| private CharArrayWriter contents = new CharArrayWriter(); | ||
| private Types currentType = null; | ||
| // this should be false when there are no contents to read | ||
| private boolean currentContents = false; | ||
|
|
||
| // Event Handlers | ||
| @Override | ||
| public void startElement(String uri, String localName, String name, Attributes attributes) throws SAXException { | ||
| contents.reset(); | ||
| tagStack.push(localName); | ||
| if (tagStack.size() > containerStack.size() + 1) { | ||
| // add a new container to the stack, use the types info from the parent | ||
| Container lastContainer = containerStack.peek(); | ||
| Object newContainerObject = makeContainerObject(currentType); | ||
| String parentName = ( tagStack.size() > 1 ? tagStack.get(tagStack.size()-2) : tagStack.peek() ); | ||
| containerStack.push( new Container(lastContainer.getContainer(), parentName, newContainerObject) ); | ||
| add(lastContainer, parentName, newContainerObject); | ||
| } | ||
| currentType = getDataType(attributes); | ||
| currentContents = false; | ||
| } | ||
|
|
||
| @Override | ||
| public void endElement(String uri, String localName, String name) throws SAXException { | ||
| if (tagStack.size() > containerStack.size()) { | ||
| // only add data when we are above a container | ||
| Object val = null; | ||
| if (currentContents) { | ||
| String content = unescapeXML(contents.toString().trim()); | ||
| val = content; | ||
| if (Types.BOOLEAN.equals(currentType)) { | ||
| val = Boolean.valueOf(content); | ||
| } else if (Types.NUMBER.equals(currentType)) { | ||
| try { | ||
| val = number(content); | ||
| } catch (NumberFormatException e) { | ||
| val = content; | ||
| } | ||
| } else if (Types.DATE.equals(currentType)) { | ||
| try { | ||
| val = new Date(Long.valueOf(content)); | ||
| } catch (NumberFormatException e) { | ||
| val = content; | ||
| } | ||
| if (s.length() < 10) { | ||
| return Integer.valueOf(s); | ||
| } | ||
| return Long.valueOf(s); | ||
| } catch (NumberFormatException ignored) { | ||
| // fall back to double below | ||
| } | ||
| // put the value into the current container | ||
| add(containerStack.peek(), localName, val); | ||
| } | ||
| if (tagStack.isEmpty()) { | ||
| throw new IllegalStateException("tag stack is out of sync, empty while still processing tags: " + localName); | ||
| } else { | ||
| tagStack.pop(); | ||
| } | ||
| // now we need to remove the current container if we are done with it | ||
| while (tagStack.size() < containerStack.size()) { | ||
| if (containerStack.size() <= 1) break; | ||
| containerStack.pop(); | ||
| } | ||
| contents.reset(); | ||
| } | ||
|
|
||
| @Override | ||
| public void characters(char[] ch, int start, int length) throws SAXException { | ||
| // get the text out of the element | ||
| contents.write(ch, start, length); | ||
| currentContents = true; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "parser: " + xml + " => " + map; | ||
| } | ||
| } | ||
|
|
||
| public static String unescapeXML(String string) { | ||
| if (string != null && string.length() > 0) { | ||
| string = string.replace("<","<").replace(">", ">").replace(""", "\"").replace("&", "&").replace("'","'"); | ||
| } | ||
| return string; | ||
| } | ||
|
|
||
| /** | ||
| * This will force a tag or attribute to be valid in XML by replacing the invalid chars with "_", | ||
| * invalid chars are ' ' (space), =, ', ", >, <, & | ||
| * @param string any string | ||
| * @return a valid string | ||
| */ | ||
| public static String convertInvalidChars(String string) { | ||
| if (string != null && string.length() > 0) { | ||
| string = string.replace(' ','_').replace('=','_').replace('"','_').replace('\'','_').replace('<','_').replace('>','_').replace('&','_'); | ||
| } | ||
| return string; | ||
| } | ||
|
|
||
| private static enum Types {STRING,NUMBER,BOOLEAN,DATE,ARRAY,COLLECTION,MAP,BEAN}; | ||
|
|
||
| protected static Types getDataType(Attributes attributes) { | ||
| Types elementType = Types.STRING; | ||
| String value = attributes.getValue("", "type"); | ||
| if (value != null) { | ||
| if ("boolean".equals(value)) { | ||
| elementType = Types.BOOLEAN; | ||
| } else if ("number".equals(value)) { | ||
| elementType = Types.NUMBER; | ||
| } else if ("date".equals(value)) { | ||
| elementType = Types.DATE; | ||
| } else if ("array".equals(value)) { | ||
| elementType = Types.ARRAY; | ||
| } else if ("collection".equals(value)) { | ||
| elementType = Types.COLLECTION; | ||
| } else if ("map".equals(value)) { | ||
| elementType = Types.MAP; | ||
| } else if ("bean".equals(value)) { | ||
| elementType = Types.BEAN; | ||
| } | ||
| } | ||
| return elementType; | ||
| } | ||
|
|
||
| protected static Class<?> getDataClass(Attributes attributes) { | ||
| Class<?> type = String.class; | ||
| String value = attributes.getValue("", "type"); | ||
| if (value != null) { | ||
| if (value.startsWith("class ")) { | ||
| value = value.substring(6); | ||
| } | ||
| // TODO handle the classes? | ||
| } | ||
| return type; | ||
| } | ||
|
|
||
| protected static Object makeContainerObject(Types type) { | ||
| Object newContainer = null; | ||
| if (Types.ARRAY.equals(type) | ||
| || Types.COLLECTION.equals(type)) { | ||
| newContainer = new Vector<Object>(); | ||
| } else { | ||
| // bean, map, unknown | ||
| newContainer = new ArrayOrderedMap<String, Object>(); | ||
| } | ||
| return newContainer; | ||
| } | ||
|
|
||
| /** | ||
| * Converts a string into a number | ||
| * @param s the string | ||
| * @return the number | ||
| * @throws NumberFormatException if the string is not a number | ||
| */ | ||
| @SuppressWarnings("fallthrough") | ||
| protected static Number number(String s) { | ||
| int length = s.length(); | ||
| boolean isFloatingPoint = false; | ||
|
|
||
| for (int i = 0; i < s.length(); i++) { | ||
| char c = s.charAt(i); | ||
| switch (c) { | ||
| case '.': | ||
| case 'e': | ||
| case 'E': | ||
| isFloatingPoint = true; | ||
| case '-': | ||
| case '+': | ||
| length--; | ||
| } | ||
| } | ||
|
|
||
| // more friendly handling of numbers | ||
| Number num = null; | ||
| if (isFloatingPoint) { | ||
| if (length < 10) { | ||
| num = Float.valueOf(s); | ||
| } else if (length < 17) { | ||
| num = Double.valueOf(s); | ||
| } else { | ||
| num = new BigDecimal(s); | ||
| } | ||
| } else { | ||
| if (length < 10) { | ||
| num = Integer.valueOf(s); | ||
| } else if (length < 19) { | ||
| num = Long.valueOf(s); | ||
| } else { | ||
| num = new BigInteger(s); | ||
| } | ||
| } | ||
| return num; | ||
| } | ||
|
|
||
| protected static class Container { | ||
| private boolean root = false; | ||
| public void setRoot(boolean root) { | ||
| this.root = root; | ||
| } | ||
| public boolean isRoot() { | ||
| return root; | ||
| } | ||
| public Object parent; | ||
| public Object getParent() { | ||
| return parent; | ||
| } | ||
| public String key; | ||
| public String getKey() { | ||
| return key; | ||
| } | ||
| public int index; | ||
| public int getIndex() { | ||
| return index; | ||
| } | ||
| public Object container; | ||
| public Object getContainer() { | ||
| return container; | ||
| } | ||
| public void updateIndex(int index) { | ||
| if (index < 0) { | ||
| throw new IllegalArgumentException("invalid index: " + index); | ||
| } | ||
| this.index = index; | ||
| key = null; | ||
| } | ||
| /** | ||
| * Replace the container with a new one based on the parent and settings in this Container | ||
| */ | ||
| public void replaceContainer(Object container) { | ||
| if (container == null) { | ||
| throw new IllegalArgumentException("No null params allowed"); | ||
| } | ||
| if (key != null) { | ||
| FieldUtils.getInstance().setFieldValue(parent, key, container); | ||
| } else if (index >= 0) { | ||
| FieldUtils.getInstance().setIndexedValue(parent, index, container); | ||
| } | ||
| // if not key or index then do nothing except replacing the value | ||
| this.container = container; | ||
| } | ||
| /** | ||
| * Use if parent is non-existent (i.e. this is the root) | ||
| */ | ||
| public Container(Object container) { | ||
| if (container == null) { | ||
| throw new IllegalArgumentException("No null params allowed"); | ||
| } | ||
| this.container = container; | ||
| this.root = true; | ||
| } | ||
| /** | ||
| * Use if parent is keyed | ||
| */ | ||
| public Container(Object parent, String key, Object container) { | ||
| if (parent == null || key == null || container == null) { | ||
| throw new IllegalArgumentException("No null params allowed"); | ||
| if (s.matches("[-+]?\\d*\\.\\d+")) { | ||
| try { | ||
| return Double.valueOf(s); | ||
| } catch (NumberFormatException ignored) { | ||
| return s; | ||
| } | ||
| } | ||
| this.container = container; | ||
| this.key = key; | ||
| this.parent = parent; | ||
| } | ||
| /** | ||
| * Use if parent is indexed | ||
| */ | ||
| public Container(Object parent, int index, Object container) { | ||
| if (parent == null || index < 0 || container == null) { | ||
| throw new IllegalArgumentException("No null params or index < 0 allowed"); | ||
| if ("true".equalsIgnoreCase(s) || "false".equalsIgnoreCase(s)) { | ||
| return Boolean.valueOf(s); | ||
| } | ||
| this.container = container; | ||
| this.index = index; | ||
| this.parent = parent; | ||
| } | ||
| @Override | ||
| public String toString() { | ||
| return "C:root="+root+":parent="+(parent==null?parent:parent.getClass().getSimpleName())+":key="+key+":index="+index+":container="+(container==null?container:container.getClass().getSimpleName()); | ||
| } | ||
| return value; |
There was a problem hiding this comment.
Do not auto-coerce digit-only XML text into numbers
coerceValue mutates every digit-only string into Integer/Long (and decimals into Double). That breaks round-trips for legitimate string data. For example:
Map<String, Object> payload = Map.of("zip", "02138");
XMLTranscoder transcoder = new XMLTranscoder();
String xml = transcoder.encode(payload, null, null);
Map<String, Object> decoded = transcoder.decode(xml);decoded.get("zip") now yields the integer 2138, losing the leading zero. Any textual numeric ID, SKU, or version string is similarly corrupted. Please drop the blanket regex coercion or replace it with a metadata-driven strategy that preserves exact values on encode/decode.
🤖 Prompt for AI Agents
In src/main/java/org/azeckoski/reflectutils/transcoders/XMLTranscoder.java
around lines 112 to 134, the current coerceValue logic automatically converts
digit-only or decimal-only Strings into Integer/Long/Double/Boolean which
corrupts legitimate textual numeric values (e.g., ZIP codes with leading zeros).
Remove the blanket regex-based numeric/boolean coercion and instead preserve
String values by default; if coercion must be supported, implement a
metadata-driven or explicit flag-based pathway so only values annotated or
encoded with type hints are converted during decode. Ensure existing callers
that expect Strings continue to receive String objects and add a configurable
switch or metadata check to opt-in to coercion where necessary.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
works again on JDK 21.
Dependency & Codec Refresh
native types.
Collection Defaults & Converters
ConstructorUtilsTest.java:166-181.
implementations.
Docs & Tooling
Tests
Everything now builds and the full JUnit suite passes on JDK 21 with no access violations.
Summary by CodeRabbit
Refactor
Documentation
Tests
Chores