iemejia commented on code in PR #3865:
URL: https://github.com/apache/avro/pull/3865#discussion_r3566894344
##########
lang/java/avro/src/main/java/org/apache/avro/SystemLimitException.java:
##########
@@ -28,13 +28,19 @@
* The following system properties can be set to limit the size of bytes,
* strings and collection types to be allocated:
* <ul>
- * <li><tt>org.apache.avro.limits.byte.maxLength</tt></li> limits the maximum
- * size of <tt>byte</tt> types.</li>
- * <li><tt>org.apache.avro.limits.collectionItems.maxLength</tt></li> limits
the
+ * <li><tt>org.apache.avro.limits.bytes.maxLength</tt> limits the maximum size
+ * of <tt>bytes</tt> types.</li>
+ * <li><tt>org.apache.avro.limits.collectionItems.maxLength</tt> limits the
* maximum number of <tt>map</tt> and <tt>list</tt> items that can be read at
* once single sequence.</li>
Review Comment:
Fixed in e47f206f4d: 'read at once single sequence' -> 'read in a single
sequence'.
##########
lang/java/avro/src/test/java/org/apache/avro/TestSystemLimitException.java:
##########
@@ -110,6 +111,46 @@ void testCheckMaxStringLength() {
"String length 1024 exceeds maximum allowed");
}
+ @Test
+ void testCheckMaxCollectionAllocation() {
+ // With a small custom limit, cumulative allocations beyond it are
rejected.
+ System.setProperty(MAX_COLLECTION_ALLOCATION_PROPERTY, "1000");
+ resetLimits();
+
+ // Values within the limit pass through and return the running total.
+ assertEquals(0L, checkMaxCollectionAllocation(0L, 0L));
+ assertEquals(1000L, checkMaxCollectionAllocation(0L, 1000L));
+ assertEquals(1000L, checkMaxCollectionAllocation(400L, 600L));
+
+ // A single block over the limit is rejected.
+ SystemLimitException ex = assertThrows(SystemLimitException.class, () ->
checkMaxCollectionAllocation(0L, 1001L));
+ assertTrue(ex.getMessage().contains("exceeds the maximum allowed of
1000"), ex.getMessage());
+
+ // Cumulative blocks that cross the limit are rejected.
+ ex = assertThrows(SystemLimitException.class, () ->
checkMaxCollectionAllocation(600L, 401L));
+ assertTrue(ex.getMessage().contains("exceeds the maximum allowed of
1000"), ex.getMessage());
+
+ // Negative arguments are rejected as malformed.
+ Exception nex = assertThrows(AvroRuntimeException.class, () ->
checkMaxCollectionAllocation(-1L, 10L));
+ assertEquals(ERROR_NEGATIVE, nex.getMessage());
+ nex = assertThrows(AvroRuntimeException.class, () ->
checkMaxCollectionAllocation(10L, -1L));
+ assertEquals(ERROR_NEGATIVE, nex.getMessage());
+
+ // Additive overflow is rejected rather than wrapping to a small value.
+ assertThrows(SystemLimitException.class, () ->
checkMaxCollectionAllocation(Long.MAX_VALUE, 10L));
+ }
+
+ @Test
+ void testCheckMaxCollectionAllocationDefaultsToHeapFraction() {
+ // With no property set, the default is derived from the heap and is well
+ // below Integer.MAX_VALUE on a normally sized JVM, yet generous enough for
+ // legitimate small collections.
+ resetLimits();
+ assertEquals(1024L, checkMaxCollectionAllocation(0L, 1024L));
+ // A pathologically large zero-byte collection is rejected without
allocating.
+ assertThrows(SystemLimitException.class, () ->
checkMaxCollectionAllocation(0L, (long) Integer.MAX_VALUE - 8));
Review Comment:
Fixed in e47f206f4d: the assertion now uses MAX_ARRAY_VM_LIMIT + 1 so it
exceeds the cap regardless of heap size (the default is heap-derived then
clamped to MAX_ARRAY_VM_LIMIT, so Integer.MAX_VALUE - 8 alone wouldn't exceed
it on a very large heap).
--
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]
