Issue fixes#295
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds CommonConstants and BasicUtils.truncateString; updates ListenerParameters defaults; replaces inline truncation in LaunchImpl with BasicUtils; configures ObjectMapper to truncate strings on deserialization; adds unit tests and changelog entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant LaunchImpl
participant ListenerParameters
participant BasicUtils
Client->>LaunchImpl: set name
LaunchImpl->>ListenerParameters: getTruncateLimit(), getTruncateReplacement()
LaunchImpl->>BasicUtils: truncateString(name, limit, replacement)
BasicUtils-->>LaunchImpl: truncatedName
LaunchImpl->>LaunchImpl: rq.setName(truncatedName)
LaunchImpl-->>Client: done
note right of BasicUtils #DDEBF7: centralized truncation logic
sequenceDiagram
autonumber
actor Caller
participant ObjectUtils
participant ObjectMapper
participant LimitedStringDeserializer
participant BasicUtils
Caller->>ObjectUtils: clonePojo(obj, Target.class)
ObjectUtils->>ObjectMapper: writeValueAsString(obj)
ObjectUtils->>ObjectMapper: readValue(json, Target.class)
ObjectMapper->>LimitedStringDeserializer: deserialize(String field)
LimitedStringDeserializer->>BasicUtils: truncateString(value, TEN_MEGABYTES, null)
BasicUtils-->>LimitedStringDeserializer: possibly truncated value
LimitedStringDeserializer-->>ObjectMapper: String
ObjectMapper-->>ObjectUtils: Target instance
ObjectUtils-->>Caller: cloned Target
note right of LimitedStringDeserializer #FFF2CC: enforces max string length on deserialization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ 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: 3
🧹 Nitpick comments (6)
src/main/java/com/epam/reportportal/utils/CommonConstants.java (1)
27-31: Use bit shifts for powers of two (nit) and consider long for future-proofing.Avoid Math.pow with ints and potential double rounding; bit shifts are clearer. If you expect larger units later, consider long.
Apply:
- public static final int KILOBYTES = (int) Math.pow(2, 10); - public static final int MEGABYTES = (int) Math.pow(KILOBYTES, 2); + public static final int KILOBYTES = 1 << 10; // 1024 + public static final int MEGABYTES = 1 << 20; // 1_048_576 public static final int TEN_MEGABYTES = 10 * MEGABYTES;src/main/java/com/epam/reportportal/service/LaunchImpl.java (1)
267-289: Unify attribute truncation through BasicUtils (consistency + edge cases).Current manual truncation duplicates logic and shares the same small-limit bug. Delegate to BasicUtils to centralize behavior.
- int limit = getParameters().getAttributeLengthLimit(); - String replacement = getParameters().getTruncateReplacement(); - return attributes.stream().map(attribute -> { - ItemAttributesRQ updated = attribute; - int keyLength = ofNullable(updated.getKey()).map(String::length).orElse(0); - if (keyLength > limit && keyLength > replacement.length()) { - updated = new ItemAttributesRQ( - updated.getKey().substring(0, limit - replacement.length()) + replacement, - updated.getValue(), - updated.isSystem() - ); - } - int valueLength = ofNullable(updated.getValue()).map(String::length).orElse(0); - if (valueLength > limit && valueLength > replacement.length()) { - updated = new ItemAttributesRQ( - updated.getKey(), - updated.getValue().substring(0, limit - replacement.length()) + replacement, - updated.isSystem() - ); - } - return updated; - }).collect(Collectors.toSet()); + final int limit = getParameters().getAttributeLengthLimit(); + final String replacement = getParameters().getTruncateReplacement(); + return attributes.stream().map(a -> { + final String key = ofNullable(a.getKey()).map(k -> BasicUtils.truncateString(k, limit, replacement)).orElse(null); + final String value = ofNullable(a.getValue()).map(v -> BasicUtils.truncateString(v, limit, replacement)).orElse(null); + if (Objects.equals(key, a.getKey()) && Objects.equals(value, a.getValue())) { + return a; + } + return new ItemAttributesRQ(key, value, a.isSystem()); + }).collect(Collectors.toSet());src/main/java/com/epam/reportportal/utils/BasicUtils.java (4)
27-36: Clarify edge cases in Javadoc (contract guarantees and errors)Document the exact contract and edge cases for callers.
- /** - * Truncates string to the specified limit. If string length exceeds the limit, it will be cut and the truncateReplacement will be - * appended. - * - * @param string string to truncate - * @param limit maximum allowed length - * @param truncateReplacement string to append if truncation happens, defaults to {@link CommonConstants#DEFAULT_TRUNCATE_REPLACEMENT} - * if null - * @return truncated string if original length exceeds the limit, original string otherwise - */ + /** + * Truncates a string to the specified limit (UTF-16 code units) and guarantees the result length ≤ {@code limit}. + * If truncation occurs, appends {@code truncateReplacement} (or {@link CommonConstants#DEFAULT_TRUNCATE_REPLACEMENT} when {@code null}). + * + * Edge cases: + * - {@code limit < 0} → {@link IllegalArgumentException} + * - {@code limit == 0} → empty string + * - {@code replacement.length() >= limit} → returns the replacement truncated to {@code limit} + * + * @param string string to truncate (non-null) + * @param limit maximum allowed length, must be ≥ 0 + * @param truncateReplacement string to append if truncation happens; defaults to {@link CommonConstants#DEFAULT_TRUNCATE_REPLACEMENT} if null + * @return original string when {@code string.length() <= limit}; otherwise a truncated string whose length ≤ {@code limit} + */
37-44: Optional: add convenience overloadCommon call sites can omit the replacement parameter.
@Nonnull public static String truncateString(@Nonnull String string, int limit) { return truncateString(string, limit, null); }
41-42: Optional: Unicode-safe truncationIf truncation near surrogate pairs/emojis matters, use code points to avoid splitting multi-unit chars.
Example approach:
final int cpLimit = limit - replacement.length(); final int end = string.offsetByCodePoints(0, Math.max(0, string.codePointCount(0, Math.min(string.length(), cpLimit)))); return string.substring(0, end) + replacement;
37-44: Add boundary tests for safetyPlease add tests for:
- limit < 0 → IllegalArgumentException
- limit == 0 → ""
- replacement length >= limit (e.g., "..." with limit 2 → "..")
- normal truncation and no-truncation paths
I can draft JUnit cases mirroring these scenarios if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)src/main/java/com/epam/reportportal/listeners/ListenerParameters.java(6 hunks)src/main/java/com/epam/reportportal/service/LaunchImpl.java(1 hunks)src/main/java/com/epam/reportportal/utils/BasicUtils.java(1 hunks)src/main/java/com/epam/reportportal/utils/CommonConstants.java(1 hunks)src/main/java/com/epam/reportportal/utils/ObjectUtils.java(3 hunks)src/test/java/com/epam/reportportal/utils/ObjectUtilsTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/main/java/com/epam/reportportal/service/LaunchImpl.java (1)
src/main/java/com/epam/reportportal/utils/BasicUtils.java (1)
BasicUtils(22-45)
src/test/java/com/epam/reportportal/utils/ObjectUtilsTest.java (2)
src/main/java/com/epam/reportportal/utils/ObjectUtils.java (1)
ObjectUtils(34-99)src/main/java/com/epam/reportportal/utils/CommonConstants.java (1)
CommonConstants(22-32)
src/main/java/com/epam/reportportal/utils/ObjectUtils.java (2)
src/main/java/com/epam/reportportal/utils/CommonConstants.java (1)
CommonConstants(22-32)src/main/java/com/epam/reportportal/utils/BasicUtils.java (1)
BasicUtils(22-45)
src/main/java/com/epam/reportportal/listeners/ListenerParameters.java (1)
src/main/java/com/epam/reportportal/utils/CommonConstants.java (1)
CommonConstants(22-32)
src/main/java/com/epam/reportportal/utils/BasicUtils.java (1)
src/main/java/com/epam/reportportal/utils/CommonConstants.java (1)
CommonConstants(22-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
CHANGELOG.md (1)
6-7: Changelog updates—LGTM.Entries are clear and correctly linked to the issues.
src/main/java/com/epam/reportportal/service/LaunchImpl.java (1)
257-259: Good move: centralized truncation via BasicUtils.Switching to BasicUtils.truncateString improves consistency and reuse.
src/main/java/com/epam/reportportal/listeners/ListenerParameters.java (3)
45-45: IO pool sizing—LGTM.Adding a lower bound of 1 avoids zero-thread edge cases on constrained environments.
75-77: Verify units: 62 MiB vs previous 65,000,000 bytes—comment may be misleading.62L * MEGABYTES equals 65,011,712 bytes, slightly larger than prior 65,000,000. If the intent was to reduce the limit to add headroom, consider using decimal MB or updating the comment to “62 MiB”.
Suggested options:
- // Due to shortcoming of payload calculation mechanism this value is set to 62 megabytes rather than 65 megabytes - public static final long DEFAULT_BATCH_PAYLOAD_LIMIT = 62L * MEGABYTES; + // Due to a shortcoming in payload size calculation, use 62 MiB instead of 65 MB + public static final long DEFAULT_BATCH_PAYLOAD_LIMIT = 62L * MEGABYTES;or to actually lower the byte count:
- public static final long DEFAULT_BATCH_PAYLOAD_LIMIT = 62L * MEGABYTES; + public static final long DEFAULT_BATCH_PAYLOAD_LIMIT = 62L * 1_000_000L; // 62 MB (decimal)
218-218: Centralizing default truncate replacement—LGTM.Referencing CommonConstants.DEFAULT_TRUNCATE_REPLACEMENT removes duplication and keeps defaults consistent.
Also applies to: 299-299
src/main/java/com/epam/reportportal/utils/ObjectUtils.java (1)
36-59: Global String deserializer truncation: confirm no unintended side effects.This will truncate every String during deserialization (including clonePojo) to TEN_MEGABYTES. Validate that IDs, URLs, or tokens aren’t expected to exceed this and that no sensitive fields rely on exact preservation beyond this length.
Would you like a targeted test to ensure fields like launch UUIDs or issue locators are unaffected?
src/main/java/com/epam/reportportal/utils/BasicUtils.java (1)
22-25: Private constructor pattern: OKPrevents instantiation as intended.
There was a problem hiding this comment.
Pull Request Overview
This PR implements bug fixes to address string field truncation and I/O pool sizing issues in the ReportPortal client. The changes introduce automatic truncation of overly long string fields to prevent failures and enforce a minimum I/O pool size to avoid under-provisioned threads.
Key changes:
- Added string truncation utilities with configurable limits and replacement text
- Implemented automatic JSON deserialization truncation for all string fields
- Enforced minimum I/O pool size of 1 thread
- Adjusted default batch payload limit from 65MB to 62MB
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/main/java/com/epam/reportportal/utils/CommonConstants.java |
New constants file defining size limits and default truncation replacement |
src/main/java/com/epam/reportportal/utils/BasicUtils.java |
New utility class with string truncation functionality |
src/main/java/com/epam/reportportal/utils/ObjectUtils.java |
Enhanced with custom JSON deserializer for automatic string truncation |
src/main/java/com/epam/reportportal/service/LaunchImpl.java |
Refactored to use new truncation utility instead of inline logic |
src/main/java/com/epam/reportportal/listeners/ListenerParameters.java |
Updated I/O pool size calculation and batch payload limits |
src/test/java/com/epam/reportportal/utils/ObjectUtilsTest.java |
New tests for cloning behavior with truncation |
src/test/java/com/epam/reportportal/utils/BasicUtilsTest.java |
New parameterized tests for string truncation scenarios |
CHANGELOG.md |
Updated with issue fix descriptions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/com/epam/reportportal/utils/BasicUtilsTest.java (3)
25-41: Add edge-case rows to strengthen coverage.
Consider covering: string length equals limit, replacement length equals limit, and empty input.Apply this diff inside testData():
return new Object[][]{ // 1. String is longer than limit, truncateReplacement is less than limit {"HelloWorld", 5, "..", "Hel.."}, // 2. String is less than limit {"Hello", 10, "...", "Hello"}, // 3. String is longer than limit, truncateReplacement is longer than limit {"HelloWorld", 3, "foobar", "Hel"}, // 4. Limit is 0 {"Hello", 0, "...", ""}, // 5. Limit is less than 0 - {"Hello", -3, "...", ""} + {"Hello", -3, "...", ""}, + // 6. String length equals limit + {"Hello", 5, "...", "Hello"}, + // 7. truncateReplacement length equals limit + {"HelloWorld", 3, "xyz", "Hel"}, + // 8. Empty input + {"", 3, "...", ""} };
19-24: Add a targeted test for null replacement using the default constant.
Validates integration with CommonConstants.DEFAULT_TRUNCATE_REPLACEMENT without hard-coding its value.Apply this diff:
import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.endsWith; @@ @MethodSource("testData") public void test_truncate_string_scenarios(String input, int limit, String replacement, String expected) { assertThat(BasicUtils.truncateString(input, limit, replacement), equalTo(expected)); } + + @Test + public void test_truncate_string_null_replacement_uses_default_and_respects_limit() { + String input = "HelloWorld"; + int limit = 5; + String result = BasicUtils.truncateString(input, limit, null); + assertThat(result.length(), equalTo(limit)); + assertThat(result, endsWith(CommonConstants.DEFAULT_TRUNCATE_REPLACEMENT)); + } }Also applies to: 43-48
43-45: Improve parameterized test display names for readability in reports.Apply this diff:
- @ParameterizedTest + @ParameterizedTest(name = "[{index}] input=\"{0}\" | limit={1} | repl=\"{2}\"")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/com/epam/reportportal/utils/BasicUtilsTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/com/epam/reportportal/utils/BasicUtilsTest.java (1)
src/main/java/com/epam/reportportal/utils/BasicUtils.java (1)
BasicUtils(22-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
src/test/java/com/epam/reportportal/utils/BasicUtilsTest.java (1)
1-15: License header looks good.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Chores