Timestamp attributes conversion logic Improvements.#4597
Closed
Thisara-Welmilla wants to merge 1 commit into
Closed
Timestamp attributes conversion logic Improvements.#4597Thisara-Welmilla wants to merge 1 commit into
Thisara-Welmilla wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughCentralizes LDAP timestamp parsing/normalization in LDAPUtil, adds exception-throwing post-retrieval attribute hooks in AbstractUserStoreManager, updates ReadOnly/UniqueID LDAP managers to use the new utilities and exception-aware hooks, and updates tests to call the utility directly. ChangesLDAP attribute processing and timestamp handling
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.42.3)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}
Comment on lines
+9204
to
+9208
| */ | ||
| protected void processAttributesAfterRetrievalWithException(String userName, Map<String, String> userAttributes, | ||
| String profileName) throws UserStoreException { | ||
|
|
||
| processAttributesAfterRetrieval(userName, userAttributes, profileName); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| */ | |
| protected void processAttributesAfterRetrievalWithException(String userName, Map<String, String> userAttributes, | |
| String profileName) throws UserStoreException { | |
| processAttributesAfterRetrieval(userName, userAttributes, profileName); | |
| protected void processAttributesAfterRetrievalWithException(String userName, Map<String, String> userAttributes, | |
| String profileName) throws UserStoreException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Processing attributes after retrieval for user: " + userName + ", profile: " + profileName); | |
| } | |
| processAttributesAfterRetrieval(userName, userAttributes, profileName); |
Comment on lines
+9246
to
+9250
| */ | ||
| protected void processAttributesAfterRetrievalWithIDWithException(String userID, Map<String, String> userAttributes, | ||
| String profileName) throws UserStoreException { | ||
|
|
||
| processAttributesAfterRetrievalWithID(userID, userAttributes, profileName); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| */ | |
| protected void processAttributesAfterRetrievalWithIDWithException(String userID, Map<String, String> userAttributes, | |
| String profileName) throws UserStoreException { | |
| processAttributesAfterRetrievalWithID(userID, userAttributes, profileName); | |
| protected void processAttributesAfterRetrievalWithIDWithException(String userID, Map<String, String> userAttributes, | |
| String profileName) throws UserStoreException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Processing attributes after retrieval for userID: " + userID + ", profile: " + profileName); | |
| } | |
| processAttributesAfterRetrievalWithID(userID, userAttributes, profileName); |
Comment on lines
4857
to
+4862
| protected void processAttributesAfterRetrieval(String userName, Map<String, String> userStorePropertyValues, | ||
| String profileName) { | ||
|
|
||
| try { | ||
| processAttributesAfterRetrieval(userStorePropertyValues); | ||
| } catch (UserStoreException e) { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| protected void processAttributesAfterRetrieval(String userName, Map<String, String> userStorePropertyValues, | |
| String profileName) { | |
| try { | |
| processAttributesAfterRetrieval(userStorePropertyValues); | |
| } catch (UserStoreException e) { | |
| protected void processAttributesAfterRetrieval(String userName, Map<String, String> userStorePropertyValues, | |
| String profileName) { | |
| if (logger.isDebugEnabled()) { | |
| logger.debug("Processing attributes after retrieval for user: " + userName); | |
| } | |
| try { | |
| processAttributesAfterRetrieval(userStorePropertyValues); |
Comment on lines
+4874
to
+4876
| private void processAttributesAfterRetrieval(Map<String, String> userStorePropertyValues) | ||
| throws UserStoreException { | ||
|
|
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| private void processAttributesAfterRetrieval(Map<String, String> userStorePropertyValues) | |
| throws UserStoreException { | |
| throws UserStoreException { | |
| if (logger.isDebugEnabled()) { | |
| logger.debug("Processing attributes with timestamp conversions"); | |
| } | |
| String timestampAttributesProperty = Optional.ofNullable(realmConfig |
Comment on lines
+3215
to
+3220
| protected void processAttributesAfterRetrievalWithID(String userID, Map<String, | ||
| String> userStorePropertyValues, String profileName) { | ||
|
|
||
| try { | ||
| processAttributesAfterRetrievalWithID(userStorePropertyValues); | ||
| } catch (UserStoreException e) { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| protected void processAttributesAfterRetrievalWithID(String userID, Map<String, | |
| String> userStorePropertyValues, String profileName) { | |
| try { | |
| processAttributesAfterRetrievalWithID(userStorePropertyValues); | |
| } catch (UserStoreException e) { | |
| protected void processAttributesAfterRetrievalWithID(String userID, Map<String, | |
| String> userStorePropertyValues, String profileName) { | |
| log.debug("Processing attributes after retrieval for userID: " + userID); | |
| try { | |
| processAttributesAfterRetrievalWithID(userStorePropertyValues); | |
| } catch (UserStoreException e) { |
Comment on lines
+3230
to
3233
|
|
||
| private void processAttributesAfterRetrievalWithID(Map<String, | ||
| String> userStorePropertyValues) throws UserStoreException { | ||
|
|
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| private void processAttributesAfterRetrievalWithID(Map<String, | |
| String> userStorePropertyValues) throws UserStoreException { | |
| private void processAttributesAfterRetrievalWithID(Map<String, | |
| String> userStorePropertyValues) throws UserStoreException { | |
| log.debug("Processing timestamp attributes for user store properties"); | |
| String timestampAttributesProperty = Optional.ofNullable(realmConfig |
Comment on lines
+146
to
+148
| public static String convertToStandardTimeFormat(RealmConfiguration realmConfig, String dateTimestamp) | ||
| throws UserStoreException { | ||
|
|
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| public static String convertToStandardTimeFormat(RealmConfiguration realmConfig, String dateTimestamp) | |
| throws UserStoreException { | |
| public static String convertToStandardTimeFormat(RealmConfiguration realmConfig, String dateTimestamp) | |
| throws UserStoreException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Converting LDAP timestamp to standard format"); | |
| } |
Comment on lines
+160
to
+163
| throw new UserStoreException("Invalid timestamp format for pattern: " + userstoreTimestampFormat, e); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| throw new UserStoreException("Invalid timestamp format for pattern: " + userstoreTimestampFormat, e); | |
| } | |
| } | |
| throw new UserStoreException("Invalid timestamp format for pattern: " + userstoreTimestampFormat, e); | |
| } | |
| } | |
| log.warn("Custom timestamp pattern not found or equals default, deriving pattern for timestamp"); |
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.java (1)
4917-4923:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical bug: Using date value as DateTimeFormatter pattern.
The method receives a date value (e.g.,
"20250813145607Z") but line 4919 uses this value as aDateTimeFormatterpattern viaDateTimeFormatter.ofPattern(date). This will throwIllegalArgumentExceptionat runtime because date values are not valid formatter patterns.The original implementation likely used the constant
GENARALIZE_DATE_TIME_FORMATdefined at line 143, or derivation fromLDAPUtil.deriveTimestampFormat(...).🐛 Proposed fix to use the correct pattern source
If this method should derive the pattern from the input value:
protected String convertDateFormatFromLDAP(String date) { - DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern(date); + String pattern = LDAPUtil.deriveTimestampFormat(date); + if (pattern == null) { + pattern = GENARALIZE_DATE_TIME_FORMAT; + } + DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern(pattern); OffsetDateTime offsetDateTime = OffsetDateTime.parse(date, dateTimeFormatter); Instant instant = offsetDateTime.toInstant(); return instant.toString(); }Or if you want to use the fixed generalized time format:
protected String convertDateFormatFromLDAP(String date) { - DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern(date); + DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern(GENARALIZE_DATE_TIME_FORMAT); OffsetDateTime offsetDateTime = OffsetDateTime.parse(date, dateTimeFormatter); Instant instant = offsetDateTime.toInstant(); return instant.toString(); }🤖 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/ldap/ReadOnlyLDAPUserStoreManager.java` around lines 4917 - 4923, The convertDateFormatFromLDAP method is mistakenly passing the input date string as a DateTimeFormatter pattern; replace DateTimeFormatter.ofPattern(date) with a real pattern source — e.g., use the existing GENARALIZE_DATE_TIME_FORMAT constant or call LDAPUtil.deriveTimestampFormat(date) to get the correct pattern, then build the DateTimeFormatter from that pattern and parse the input (OffsetDateTime.parse or the appropriate parse method) to produce the Instant and return its toString(); update references in convertDateFormatFromLDAP to use the derived/fixed pattern instead of the date value.core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManager.java (1)
3243-3266:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDebug logs may expose PII.
The debug logging of
userStorePropertyValuesat lines 3245-3246 and 3262-3263 could expose sensitive user attributes such as email addresses, phone numbers, or other claim values. Even though debug logging is typically disabled in production, this poses a compliance risk (GDPR/CCPA) if debug is enabled during troubleshooting.Consider logging only attribute names rather than the full map with values, or redacting sensitive claim types.
🛡️ Proposed fix to avoid logging PII
if (ArrayUtils.isNotEmpty(timestampAttributes)) { if (logger.isDebugEnabled()) { - logger.debug("Retrieved user store properties before type conversions: " + userStorePropertyValues); + logger.debug("Retrieved user store properties before type conversions. Attribute keys: " + userStorePropertyValues.keySet()); } Map<String, String> convertedTimestampAttributeValues = new HashMap<>(); for (String attribute : timestampAttributes) { if (userStorePropertyValues.get(attribute) != null) { convertedTimestampAttributeValues.put(attribute, LDAPUtil.convertToStandardTimeFormat(realmConfig, userStorePropertyValues.get(attribute))); } } if (logger.isDebugEnabled()) { logger.debug("Converted timestamp attribute values: " + convertedTimestampAttributeValues); } userStorePropertyValues.putAll(convertedTimestampAttributeValues); if (logger.isDebugEnabled()) { - logger.debug("Retrieved user store properties after type conversions: " + userStorePropertyValues); + logger.debug("Retrieved user store properties after type conversions. Attribute keys: " + userStorePropertyValues.keySet()); } }🤖 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/ldap/UniqueIDReadOnlyLDAPUserStoreManager.java` around lines 3243 - 3266, The debug logs currently print full userStorePropertyValues (in the block handling timestampAttributes) which can expose PII; change the logger.debug calls that reference userStorePropertyValues to log only non-sensitive information—e.g., log the set of attribute names (userStorePropertyValues.keySet()) or a sanitized map with values redacted—while leaving the conversion logic using timestampAttributes and LDAPUtil.convertToStandardTimeFormat(realmConfig, ...) unchanged; ensure both pre-conversion and post-conversion logger.debug calls for userStorePropertyValues are replaced with the safer key-only or redacted variant.
🧹 Nitpick comments (1)
core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java (1)
9198-9209: ⚡ Quick winInconsistent deprecation pattern: consider deprecating
processAttributesAfterRetrieval.You've added
processAttributesAfterRetrievalWithExceptionas the new exception-throwing variant and it delegates toprocessAttributesAfterRetrieval. However, unlike theWithIDvariants in hunk 2, you haven't deprecated the oldprocessAttributesAfterRetrievalmethod. This creates an inconsistency:
processAttributesAfterRetrievalWithID→ deprecated ✓processAttributesAfterRetrieval→ not deprecated ✗For consistency and to guide subclass implementers toward the new exception-throwing API, consider marking
processAttributesAfterRetrievalas@Deprecatedwith similar documentation pointing toprocessAttributesAfterRetrievalWithException.🤖 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/common/AbstractUserStoreManager.java` around lines 9198 - 9209, Mark the old non-exception variant processAttributesAfterRetrieval(...) as `@Deprecated` and update its Javadoc to point implementers to the new processAttributesAfterRetrievalWithException(...) method; keep the current delegation behavior (have processAttributesAfterRetrieval call processAttributesAfterRetrievalWithException or vice versa per existing pattern), and ensure the deprecation Javadoc includes the recommended replacement and exception guidance so subclasses are directed to implement the exception-throwing API.
🤖 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 24-35: deriveTimestampFormat is failing to recognize 12-digit
timestamps without fractions (e.g., 202508131456Z); update LDAPUtil: add a
dedicated regex for 12-digit no-fraction timestamps (similar to
NO_FRACTION_TIMESTAMP_PATTERN but matching exactly 12 digits before the
timezone) and in deriveTimestampFormat(...) map that new pattern to the correct
formatter (e.g., "uuuuMMddHHmmX"/equivalent constant) alongside the existing
mappings for 14-digit and fractional patterns; also review and consolidate the
existing one-digit/two-digit/three-digit fraction patterns
(ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN,
ONE_DIGIT_FRACTION_TIMESTAMP_PATTERN, etc.) to avoid duplicate/incorrect 12 vs
14 digit matches so the function returns the 12-digit no-fraction format instead
of falling through to the unsupported branch.
- Around line 153-161: The code only catches DateTimeParseException around the
OffsetDateTime.parse(... DateTimeFormatter.ofPattern(userstoreTimestampFormat)
...) call, but DateTimeFormatter.ofPattern(userstoreTimestampFormat) can throw
IllegalArgumentException for invalid pattern syntax; update the try/catch around
the DateTimeFormatter.ofPattern and OffsetDateTime.parse so that
IllegalArgumentException is caught and rethrown as a UserStoreException (similar
to the existing DateTimeParseException handling), referencing the
userstoreTimestampFormat variable and the DateTimeFormatter.ofPattern(...)
invocation to ensure malformed pattern errors are wrapped and reported properly.
---
Outside diff comments:
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.java`:
- Around line 4917-4923: The convertDateFormatFromLDAP method is mistakenly
passing the input date string as a DateTimeFormatter pattern; replace
DateTimeFormatter.ofPattern(date) with a real pattern source — e.g., use the
existing GENARALIZE_DATE_TIME_FORMAT constant or call
LDAPUtil.deriveTimestampFormat(date) to get the correct pattern, then build the
DateTimeFormatter from that pattern and parse the input (OffsetDateTime.parse or
the appropriate parse method) to produce the Instant and return its toString();
update references in convertDateFormatFromLDAP to use the derived/fixed pattern
instead of the date value.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManager.java`:
- Around line 3243-3266: The debug logs currently print full
userStorePropertyValues (in the block handling timestampAttributes) which can
expose PII; change the logger.debug calls that reference userStorePropertyValues
to log only non-sensitive information—e.g., log the set of attribute names
(userStorePropertyValues.keySet()) or a sanitized map with values redacted—while
leaving the conversion logic using timestampAttributes and
LDAPUtil.convertToStandardTimeFormat(realmConfig, ...) unchanged; ensure both
pre-conversion and post-conversion logger.debug calls for
userStorePropertyValues are replaced with the safer key-only or redacted
variant.
---
Nitpick comments:
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java`:
- Around line 9198-9209: Mark the old non-exception variant
processAttributesAfterRetrieval(...) as `@Deprecated` and update its Javadoc to
point implementers to the new processAttributesAfterRetrievalWithException(...)
method; keep the current delegation behavior (have
processAttributesAfterRetrieval call
processAttributesAfterRetrievalWithException or vice versa per existing
pattern), and ensure the deprecation Javadoc includes the recommended
replacement and exception guidance so subclasses are directed to implement the
exception-throwing API.
🪄 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: 90c1896a-56b4-41b3-9cb6-500e11464afc
📒 Files selected for processing (4)
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.java
Comment on lines
+24
to
+35
| // Regex patterns for LDAP timestamp formats. | ||
| private static final Pattern NO_FRACTION_TIMESTAMP_PATTERN = Pattern.compile("^\\d{14}([-+]\\d{4}|Z)$"); | ||
| private static final Pattern THREE_DIGIT_FRACTION_TIMESTAMP_PATTERN = | ||
| Pattern.compile("^\\d{14}[,\\.]\\d{3}([-+]\\d{4}|Z)$"); | ||
| 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)$"); | ||
| private static final Pattern ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN = | ||
| Pattern.compile("^\\d{12}[,\\.]\\d{1}([-+]\\d{4}|Z)$"); |
There was a problem hiding this comment.
deriveTimestampFormat(...) misses 12-digit timestamps without fractions.
At Line 180, the documented/default pattern includes uuuuMMddHHmm[,S][.S]X, but implementation only recognizes the 12-digit variant when a 1-digit fraction exists (Lines 208-209). Inputs like 202508131456Z incorrectly fall through to Line 174 as unsupported.
💡 Proposed fix
+ private static final Pattern NO_SECONDS_NO_FRACTION_TIMESTAMP_PATTERN =
+ Pattern.compile("^\\d{12}([-+]\\d{4}|Z)$");
private static final Pattern ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN =
Pattern.compile("^\\d{12}[,\\.]\\d{1}([-+]\\d{4}|Z)$"); // Case 4: 14 digits with 1-digit fraction (seconds precision).
else if (ONE_DIGIT_FRACTION_WITH_SECONDS_TIMESTAMP_PATTERN.matcher(timestamp).matches()) {
return timestamp.contains(",") ? "uuuuMMddHHmmss,SX" : "uuuuMMddHHmmss.SX";
}
- // Case 5: 12 digits with 1-digit fraction (minutes precision).
+ // Case 5: 12 digits with no fractional seconds (minutes precision).
+ else if (NO_SECONDS_NO_FRACTION_TIMESTAMP_PATTERN.matcher(timestamp).matches()) {
+ return "uuuuMMddHHmmX";
+ }
+ // Case 6: 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";
}Also applies to: 191-210
🤖 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 24 - 35, deriveTimestampFormat is failing to recognize 12-digit
timestamps without fractions (e.g., 202508131456Z); update LDAPUtil: add a
dedicated regex for 12-digit no-fraction timestamps (similar to
NO_FRACTION_TIMESTAMP_PATTERN but matching exactly 12 digits before the
timezone) and in deriveTimestampFormat(...) map that new pattern to the correct
formatter (e.g., "uuuuMMddHHmmX"/equivalent constant) alongside the existing
mappings for 14-digit and fractional patterns; also review and consolidate the
existing one-digit/two-digit/three-digit fraction patterns
(ONE_DIGIT_FRACTION_WITH_MINUTES_TIMESTAMP_PATTERN,
ONE_DIGIT_FRACTION_TIMESTAMP_PATTERN, etc.) to avoid duplicate/incorrect 12 vs
14 digit matches so the function returns the 12-digit no-fraction format instead
of falling through to the unsupported branch.
Thisara-Welmilla
force-pushed
the
improve-timestamp-convertion-logic
branch
from
7b8d9ef to
aa667c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManagerTest.java`:
- Around line 147-154: The test in UniqueIDReadOnlyLDAPUserStoreManagerTest is
nondeterministic because it treats both acceptance and rejection of the "+05"
timezone as passing; make it deterministic by asserting the expected behavior:
replace the try/catch block calling
LDAPUtil.convertToStandardTimeFormat(realmConfig, "20250813145607+05") with a
single assertion that it throws a UserStoreException (e.g., use
assertThrows(UserStoreException.class, () ->
LDAPUtil.convertToStandardTimeFormat(...))) so the test fails if behavior
drifts.
🪄 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: a44bb96e-3a10-4210-b021-fde4c57c855b
📒 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
🚧 Files skipped from review as they are similar to previous changes (4)
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/util/LDAPUtil.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManager.java
Thisara-Welmilla
force-pushed
the
improve-timestamp-convertion-logic
branch
from
aa667c8 to
530f6e0
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Issue: