Timestamp attributes conversion logic Improvements.#4599
Timestamp attributes conversion logic Improvements.#4599Thisara-Welmilla wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR refactors LDAP timestamp processing to centralize conversion logic in ChangesLDAP Timestamp Conversion Centralization with Exception-Throwing API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.javaThanks 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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
![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
🧹 Nitpick comments (2)
core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/util/LDAPUtil.java (1)
159-161: 💤 Low valueInclude the timestamp value in parse error messages for easier debugging.
Lines 160 and 171 include only the pattern in the error message, but not the actual timestamp that failed to parse. Line 174 does include the timestamp. Consider making these consistent.
Proposed improvement
} catch (DateTimeParseException e) { - throw new UserStoreException("Invalid timestamp format for pattern: " + userstoreTimestampFormat, e); + throw new UserStoreException("Invalid timestamp '" + dateTimestamp + "' for pattern: " + userstoreTimestampFormat, e); }} catch (DateTimeParseException e) { - throw new UserStoreException("Invalid timestamp format for pattern: " + derivedTimeStampPattern, e); + throw new UserStoreException("Invalid timestamp '" + dateTimestamp + "' for pattern: " + derivedTimeStampPattern, e); }Also applies to: 170-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/util/LDAPUtil.java` around lines 159 - 161, Update the DateTimeParseException handling in LDAPUtil to include the actual timestamp string alongside the pattern in the thrown UserStoreException message: use the existing variables userstoreTimestamp and userstoreTimestampFormat (or similarly named locals in the same method) to build the message, e.g. "Invalid timestamp '...'' for pattern '...'", and apply the same change to both catch blocks referenced (the one currently throwing "Invalid timestamp format for pattern: " + userstoreTimestampFormat and the similar block at lines ~170-172) so the error consistently contains the failing timestamp and the pattern while still passing the original exception as the cause.core/org.wso2.carbon.user.core/src/test/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManagerTest.java (1)
148-155: 💤 Low valueTautological assertion provides no value.
The assertion
assertTrue(e instanceof UserStoreException, ...)inside acatch (UserStoreException e)block is always true by definition. Consider asserting on the exception message content instead, or removing the assertion entirely since the test is designed to pass regardless of whether the exception is thrown.🔧 Suggested simplification
try { LDAPUtil.convertToStandardTimeFormat(realmConfig, "20250813145607+05"); // If this doesn't throw an exception, it's supported. } catch (UserStoreException e) { // Expected - 2-digit timezone not supported by current regex. - assertTrue(e instanceof UserStoreException, - "Should throw UserStoreException for 2-digit timezone"); + // Exception thrown as expected for unsupported 2-digit timezone format. }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/org.wso2.carbon.user.core/src/test/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManagerTest.java` around lines 148 - 155, The catch block in UniqueIDReadOnlyLDAPUserStoreManagerTest (catch (UserStoreException e)) contains a tautological assertion assertTrue(e instanceof UserStoreException, ...) which is always true; remove that assertion and either (a) assert on the exception content (e.g., check e.getMessage() contains the expected regex/timezone error) or (b) simply leave the catch empty/comment that the exception is expected so the test passes; locate the LDAPUtil.convertToStandardTimeFormat call and replace the tautological assert with a meaningful assertion on e.getMessage() or delete the assertion entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/util/LDAPUtil.java`:
- Around line 31-32: The constant ONE_DIGIT_FRACTION_TIMESTAMP_PATTERN is dead
code and duplicates ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN; remove
the ONE_DIGIT_FRACTION_TIMESTAMP_PATTERN declaration from LDAPUtil and ensure
deriveTimestampFormat and any other methods reference the remaining
ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN (or consolidate to that single
constant) so there are no unresolved references.
- Around line 207-210: The minute-fraction patterns (returned as
"uuuuMMddHHmm,SX"/"uuuuMMddHHmm.SX") are wrong because DateTimeFormatter 'S' is
fraction-of-second; update LDAPUtil to stop returning these misleading patterns
for ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN and instead handle them
explicitly in the parsing logic (e.g., in the method that builds/parses
timestamps such as parseGeneralizedTime). Specifically, when
ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN matches, parse the base
timestamp with "uuuuMMddHHmm", extract the fraction digits after ',' or '.',
convert that fraction-of-minute into seconds/nanos by computing nanos =
(fraction / 10^len) * 60 * 1_000_000_000 (or equivalent integer math to avoid
loss), then apply those seconds/nanos to the parsed LocalDateTime/Instant; do
not rely on a DateTimeFormatter pattern containing 'S'. Ensure you reference and
change handling for both comma and dot separators (',' and '.') and update any
tests or callers that expect the old pattern behavior.
---
Nitpick comments:
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/util/LDAPUtil.java`:
- Around line 159-161: Update the DateTimeParseException handling in LDAPUtil to
include the actual timestamp string alongside the pattern in the thrown
UserStoreException message: use the existing variables userstoreTimestamp and
userstoreTimestampFormat (or similarly named locals in the same method) to build
the message, e.g. "Invalid timestamp '...'' for pattern '...'", and apply the
same change to both catch blocks referenced (the one currently throwing "Invalid
timestamp format for pattern: " + userstoreTimestampFormat and the similar block
at lines ~170-172) so the error consistently contains the failing timestamp and
the pattern while still passing the original exception as the cause.
In
`@core/org.wso2.carbon.user.core/src/test/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManagerTest.java`:
- Around line 148-155: The catch block in
UniqueIDReadOnlyLDAPUserStoreManagerTest (catch (UserStoreException e)) contains
a tautological assertion assertTrue(e instanceof UserStoreException, ...) which
is always true; remove that assertion and either (a) assert on the exception
content (e.g., check e.getMessage() contains the expected regex/timezone error)
or (b) simply leave the catch empty/comment that the exception is expected so
the test passes; locate the LDAPUtil.convertToStandardTimeFormat call and
replace the tautological assert with a meaningful assertion on e.getMessage() or
delete the assertion entirely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2c84ce22-a6e7-41d6-8eb0-02e1fa92a7ce
📒 Files selected for processing (5)
core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/util/LDAPUtil.javacore/org.wso2.carbon.user.core/src/test/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManagerTest.java
| private static final Pattern ONE_DIGIT_FRACTION_TIMESTAMP_PATTERN = | ||
| Pattern.compile("^\\d{12}[,\\.]\\d{1}([-+]\\d{4}|Z)$"); |
There was a problem hiding this comment.
Dead code: ONE_DIGIT_FRACTION_TIMESTAMP_PATTERN is defined but never used.
This pattern is identical to ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN (line 35-36) and is never referenced in deriveTimestampFormat. Remove this unused constant.
Proposed fix
private static final Pattern TWO_DIGIT_FRACTION_TIMESTAMP_PATTERN =
Pattern.compile("^\\d{14}[,\\.]\\d{2}([-+]\\d{4}|Z)$");
- private static final Pattern ONE_DIGIT_FRACTION_TIMESTAMP_PATTERN =
- Pattern.compile("^\\d{12}[,\\.]\\d{1}([-+]\\d{4}|Z)$");
private static final Pattern ONE_DIGIT_FRACTION_WITH_SECONDS_TIMESTAMP_PATTERN =
Pattern.compile("^\\d{14}[,\\.]\\d{1}([-+]\\d{4}|Z)$");📝 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.
| private static final Pattern ONE_DIGIT_FRACTION_TIMESTAMP_PATTERN = | |
| Pattern.compile("^\\d{12}[,\\.]\\d{1}([-+]\\d{4}|Z)$"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/util/LDAPUtil.java`
around lines 31 - 32, The constant ONE_DIGIT_FRACTION_TIMESTAMP_PATTERN is dead
code and duplicates ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN; remove
the ONE_DIGIT_FRACTION_TIMESTAMP_PATTERN declaration from LDAPUtil and ensure
deriveTimestampFormat and any other methods reference the remaining
ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN (or consolidate to that single
constant) so there are no unresolved references.
| // Case 5: 12 digits with 1-digit fraction (minutes precision). | ||
| else if (ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN.matcher(timestamp).matches()) { | ||
| return timestamp.contains(",") ? "uuuuMMddHHmm,SX" : "uuuuMMddHHmm.SX"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Java DateTimeFormatter fraction-of-second S pattern without seconds field
💡 Result:
In the Java java.time.format.DateTimeFormatter, the pattern letter 'S' is used to represent the fraction-of-second field [1][2]. While it is common to use 'S' in conjunction with the seconds field (e.g., "HH:mm:ss.SSS"), the pattern syntax does not strictly require the seconds field to be present to use 'S' [1][3]. However, if you include 'S' in a pattern without 's' (second-of-minute), the formatter will still attempt to format or parse the nano-of-second field [1][2]. Because 'S' represents a fraction, it effectively works with the time-of-day [1][4]. If your time object does not contain information down to the nanosecond level, or if you omit seconds but expect a meaningful fractional representation, ensure your input data supports the precision you are requesting [5]. To implement this, you can simply include 'S' in your pattern string [5]. For example: DateTimeFormatter formatter = DateTimeFormatter.ofPattern("HH:mm.SSS"); This will output the hour, minute, and a three-digit fractional second [5]. If you require more complex behavior—such as making the fractional part optional or ensuring it only appears if a value exists—it is recommended to use the DateTimeFormatterBuilder rather than a simple pattern string [6][5]. This allows for granular control using the appendFraction method [6][7]: DateTimeFormatter formatter = new DateTimeFormatterBuilder.appendPattern("HH:mm").appendFraction(ChronoField.NANO_OF_SECOND, 0, 3, true) // min 0, max 3, include decimal point.toFormatter; This approach avoids the constraints of fixed pattern letters and provides precise control over how fractional seconds are handled when seconds or other fields might be omitted [7][5].
Citations:
- 1: https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/time/format/DateTimeFormatter.html
- 2: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/time/format/DateTimeFormatter.java
- 3: https://docs.oracle.com/en/java/javase//20/docs/api/java.base/java/time/format/DateTimeFormatter.html
- 4: https://docs.oracle.com/javase/9/docs/api/java/time/format/DateTimeFormatter.html
- 5: https://stackoverflow.com/questions/49797930/java8-localdatetime-and-nano-of-second-strange-formatting
- 6: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/time/format/DateTimeFormatterBuilder.html?is-external=true
- 7: https://stackoverflow.com/questions/44373144/java-8-how-to-create-datetimeformatter-with-milli-micro-or-nano-seconds
🌐 Web query:
RFC 4517 Generalized Time fraction after minutes without seconds semantics fraction of minute
💡 Result:
Under RFC 4517, the GeneralizedTime syntax allows for an optional fraction of a minute when the minute component is present but the second and leap-second components are omitted [1][2]. Specifically, the ABNF definition for GeneralizedTime includes an optional fraction component [3][4]. The semantic interpretation of this fraction depends on which time components are included in the string: - If the minute component is omitted, the fraction represents a fraction of an hour [1][2]. - If the minute component is present, but the second and leap-second components are omitted, the fraction represents a fraction of a minute [1][2]. - If both the minute and second (or leap-second) components are present, the fraction represents a fraction of a second [1][2]. Therefore, a fraction appended after the minutes (without seconds) is semantically valid and represents a fraction of a minute [1][5][2].
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc4517.txt
- 2: https://datatracker.ietf.org/doc/rfc4517/
- 3: https://www.rfc-editor.org/info/rfc4517/
- 4: https://datatracker.ietf.org/doc/html/rfc4517
- 5: https://ldapwiki.com/wiki/Wiki.jsp?page=GeneralizedTime
Fix GeneralizedTime minute-fraction parsing (uuuuMMddHHmm,SX / uuuuMMddHHmm.SX) in LDAPUtil.
RFC 4517 specifies that when only minutes are present, the optional fraction is a fraction of a minute; however, Java DateTimeFormatter pattern S is fraction-of-second (nano-of-second). Using uuuuMMddHHmm,SX/uuuuMMddHHmm.SX therefore misinterprets the LDAP fraction and can produce incorrect timestamps—convert fraction-of-minute to seconds/nanos explicitly (custom parsing/scaling) instead of relying on S.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/util/LDAPUtil.java`
around lines 207 - 210, The minute-fraction patterns (returned as
"uuuuMMddHHmm,SX"/"uuuuMMddHHmm.SX") are wrong because DateTimeFormatter 'S' is
fraction-of-second; update LDAPUtil to stop returning these misleading patterns
for ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN and instead handle them
explicitly in the parsing logic (e.g., in the method that builds/parses
timestamps such as parseGeneralizedTime). Specifically, when
ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN matches, parse the base
timestamp with "uuuuMMddHHmm", extract the fraction digits after ',' or '.',
convert that fraction-of-minute into seconds/nanos by computing nanos =
(fraction / 10^len) * 60 * 1_000_000_000 (or equivalent integer math to avoid
loss), then apply those seconds/nanos to the parsed LocalDateTime/Instant; do
not rely on a DateTimeFormatter pattern containing 'S'. Ensure you reference and
change handling for both comma and dot separators (',' and '.') and update any
tests or callers that expect the old pattern behavior.
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/26865988651
2 participants
Issue: