Adding case insensitive searching and sorting by API name to API Resource Management UI. #7578
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| } else if (Pattern.compile(" pr$", Pattern.CASE_INSENSITIVE).matcher(filterString).find()) { | ||
| //with filter PR, there should not be whitespace after. | ||
| filterParts = trimmedFilter.split(" pr| PR| pR| Pr"); | ||
| if (filterParts.length >= 2) { | ||
| filterParts = trimmedFilter.split("(?i) pr$"); | ||
| if (filterParts.length >= 1) { | ||
| setExpressionNodeValues(filterParts[0], " pr", null, expressionNode); | ||
| } | ||
| } else if (Pattern.compile(Pattern.quote(" gt "), Pattern.CASE_INSENSITIVE).matcher(filterString).find()) { |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| } else if (Pattern.compile(" pr$", Pattern.CASE_INSENSITIVE).matcher(filterString).find()) { | |
| //with filter PR, there should not be whitespace after. | |
| filterParts = trimmedFilter.split(" pr| PR| pR| Pr"); | |
| if (filterParts.length >= 2) { | |
| filterParts = trimmedFilter.split("(?i) pr$"); | |
| if (filterParts.length >= 1) { | |
| setExpressionNodeValues(filterParts[0], " pr", null, expressionNode); | |
| } | |
| } else if (Pattern.compile(Pattern.quote(" gt "), Pattern.CASE_INSENSITIVE).matcher(filterString).find()) { | |
| } else if (Pattern.compile(" pr$", Pattern.CASE_INSENSITIVE).matcher(filterString).find()) { | |
| //with filter PR, there should not be whitespace after. | |
| filterParts = trimmedFilter.split("(?i) pr$"); | |
| if (filterParts.length >= 1) { | |
| log.debug("Processing PR filter with attribute: " + filterParts[0]); | |
| setExpressionNodeValues(filterParts[0], " pr", null, expressionNode); | |
| } | |
| } else if (Pattern.compile(Pattern.quote(" gt "), Pattern.CASE_INSENSITIVE).matcher(filterString).find()) { |
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.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 |
There was a problem hiding this comment.
Pull Request Overview
This PR implements case-insensitive searching and sorting by API name in the API Resource Management UI. The changes address a bug where the API resource filter was not working correctly and sorting was based on CURSOR_KEY instead of NAME.
Key changes:
- Modified SQL queries to sort by NAME instead of CURSOR_KEY across all database vendors
- Implemented case-insensitive filtering using LOWER() function in SQL queries
- Fixed pagination attribute mapping to use NAME instead of CURSOR_KEY
- Updated regex pattern in FilterTreeBuilder to handle "pr" operator correctly when NAME contains text
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| FilterTreeBuilder.java | Fixed regex pattern for "pr" operator to match only at end of string and adjusted split logic for single-part filters |
| FilterQueriesUtil.java | Added LOWER() function to both attribute name and parameter value for case-insensitive filtering in LIKE queries |
| SQLConstants.java | Changed ORDER BY clause from CURSOR_KEY to NAME across all database-specific query constants |
| APIResourceManagementConstants.java | Updated pagination attribute mappings from CURSOR_KEY to NAME for BEFORE and AFTER filters |
| String filterString = " LIKE LOWER(?) AND "; | ||
| String attributeNameWithLowerCaseFunction = "LOWER(" + attributeName + ")"; | ||
| filter.append(attributeNameWithLowerCaseFunction).append(filterString); |
There was a problem hiding this comment.
The LOWER() function is incorrectly applied. The parameter placeholder ? should not be wrapped in LOWER() in the SQL string. Instead, the parameter value should be lowercased in Java before being set. Change filterString to " LIKE ? AND " and lowercase the value parameter in setFilterAttributeValue() calls using .toLowerCase().
| String filterString = " LIKE LOWER(?) AND "; | ||
| String attributeNameWithLowerCaseFunction = "LOWER(" + attributeName + ")"; | ||
| filter.append(attributeNameWithLowerCaseFunction).append(filterString); |
There was a problem hiding this comment.
The LOWER() function is incorrectly applied. The parameter placeholder ? should not be wrapped in LOWER() in the SQL string. Instead, the parameter value should be lowercased in Java before being set. Change filterString to " LIKE ? AND " and lowercase the value parameter in setFilterAttributeValue() calls using .toLowerCase().
| String filterString = " LIKE LOWER(?) AND "; | ||
| String attributeNameWithLowerCaseFunction = "LOWER(" + attributeName + ")"; | ||
| filter.append(attributeNameWithLowerCaseFunction).append(filterString); |
There was a problem hiding this comment.
The LOWER() function is incorrectly applied. The parameter placeholder ? should not be wrapped in LOWER() in the SQL string. Instead, the parameter value should be lowercased in Java before being set. Change filterString to " LIKE ? AND " and lowercase the value parameter in setFilterAttributeValue() calls using .toLowerCase().
| filterParts = trimmedFilter.split("(?i) pr$"); | ||
| if (filterParts.length >= 1) { |
There was a problem hiding this comment.
The split operation with pattern "(?i) pr$" will always produce at least one element even when " pr" is not present in the string. The condition filterParts.length >= 1 is always true and doesn't validate that the "pr" operator was actually found. This could lead to incorrect filter processing. Consider checking if the pattern actually matches before splitting, or verify that the split produced meaningful parts.
Malith-19
requested review from
mpmadhavig and
piraveena
and removed request for
Malith-19
|
@Jalina2007 Can you check the copilot suggestions and this filtering issue is only exists for the particular databases. In that case, normally we prepare the queries separately for those to avoid these kind of issues. Can we also look in to that possibility too? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7578 +/- ##
============================================
- Coverage 50.65% 50.62% -0.03%
- Complexity 19196 19201 +5
============================================
Files 2093 2093
Lines 123006 124169 +1163
Branches 25594 25839 +245
============================================
+ Hits 62306 62860 +554
- Misses 52716 53312 +596
- Partials 7984 7997 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @Malith-19, I assume by the "filtering issue", you meant the main issue that we're addressing (case insensitive filtering). The problem is that some databases (MySQL, SQLite) are case-insensitive for the LIKE operator, but databases like PostgreSQL, Oracle, and H2 (which is the one used for testing) are case-sensitive for the same operator. The filter for the filtering is prepared for all platforms, and I found no reasonable way to "know" which database system is currently being used within the FilterQueriesUtil class (please correct me if I'm wrong). So the best way I found to fix this is to use explicit lowercasing of the values without depending on database-specific features. |
|
Hi @Jalina2007 did you test you solution on all the DB types? |
|
Hi @shashimalcse , I ran the server on H2, MySQL, PostgreSQL, and OracleDB XE - everything worked as expected. However, when testing with Microsoft SQL Server XE, the server appeared to hang during startup. This might be a local environment issue on my machine. I’ll test it on another system and let you know. |
|
Hi again, @shashimalcse , I managed to run the server with Microsoft SQL server and it worked as expected. The issue happened to be a configuration issue of the database side. Please let me know if I missed anything. |
|
We need to find another way to handle ordering because your approach might cause pagination issues, and we need to think it through for other resources that use cursor-based pagination. Can we scope this PR to only address the search? |
@shashimalcse how about conditionally applying the resource name for the cursor when the configured DB is case-sensitive? |
|
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. WalkthroughThree private filter builder methods in FilterQueriesUtil were modified to apply LOWER() SQL functions to attribute names and convert filter values to lowercase, enabling case-insensitive LIKE pattern matching in database queries. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Jalina2007
force-pushed
the
fix/product-is-24875-api-resource-search-fix
branch
2 times, most recently
from
dceed48 to
237f450
Compare
|
Hi @shashimalcse , I have removed the commits that tries to fix the ordering and narrowed the PRs scope to only fix the searching. Please let me know if anything needs to be changed. |
|
Hi @Jalina2007 i still see the changes related to ordering in |
Jalina2007
force-pushed
the
fix/product-is-24875-api-resource-search-fix
branch
3 times, most recently
from
30affd7 to
683b422
Compare
…terBuilder() to support case insensitive comparisons.
…instead of inside the query.
Jalina2007
force-pushed
the
fix/product-is-24875-api-resource-search-fix
branch
from
683b422 to
1cdf659
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/api-resource-mgt/org.wso2.carbon.identity.api.resource.mgt/src/main/java/org/wso2/carbon/identity/api/resource/mgt/util/FilterQueriesUtil.java (2)
195-197: Consider usingLocale.ROOTfor consistent case conversion.For database-agnostic behavior, consider using
value.toLowerCase(Locale.ROOT)instead ofvalue.toLowerCase(). This ensures consistent case conversion regardless of the JVM's default locale, though for typical ASCII API names this distinction is minor.Example for line 197:
- filterQueryBuilder.setFilterAttributeValue(count, value.toLowerCase() + "%"); + filterQueryBuilder.setFilterAttributeValue(count, value.toLowerCase(Locale.ROOT) + "%");Apply the same pattern to lines 206 and 215.
Also applies to: 204-206, 213-215
195-196: Be aware of potential index performance impact.Wrapping column names in
LOWER()prevents the database from using standard indexes on those columns. For efficient case-insensitive searches at scale, the database would need function-based or expression indexes onLOWER(column_name). Consider documenting this requirement for production deployments or providing database-specific migration scripts to add appropriate indexes.Also applies to: 204-205, 213-214
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/api-resource-mgt/org.wso2.carbon.identity.api.resource.mgt/src/main/java/org/wso2/carbon/identity/api/resource/mgt/util/FilterQueriesUtil.java(1 hunks)
🔇 Additional comments (2)
components/api-resource-mgt/org.wso2.carbon.identity.api.resource.mgt/src/main/java/org/wso2/carbon/identity/api/resource/mgt/util/FilterQueriesUtil.java (2)
191-198: Case-insensitive search implementation looks correct.The approach of applying LOWER() to the column name in SQL while lowercasing the value in Java before parameter binding is correct and addresses the past Copilot concerns. The placeholder
?is not wrapped in LOWER(), which is the proper way to implement case-insensitive LIKE matching.
195-197: Case-insensitivity inconsistency appears intentional and consistent across codebase.After verifying the FilterQueriesUtil.java implementation, I found that the distinction between case-sensitive equality filters (EQ, NE) and case-insensitive pattern filters (SW, EW, CO) is intentional and consistent across three independent components:
- FilterQueriesUtil.java: EQ/NE case-sensitive; SW/EW/CO case-insensitive
- RoleDAOImpl.java: Same pattern
- FilterQueryBuilderUtil.java: Same pattern
Each component implements equality operators without the
LOWER()function, while pattern-matching operators useLOWER()on both attribute and value. This suggests a deliberate architectural decision to treat exact equality differently from pattern matching.However, no code comments or documentation explain the rationale for this distinction. Verify with the team whether this design aligns with the API resource filtering requirements—specifically whether equality comparisons should be case-sensitive (e.g., "Admin" ≠ "admin") while pattern matches are case-insensitive.
|
Hi @shashimalcse , I'm sorry about the earlier confusion. I have updated it to only include the necessary commits for sorting. |
|
Hi @shashimalcse , Just checking in to see whether you need anything else from my side for the PR. Please let me know if I need to change anything. Thank you. |
4 participants
Related Issue
API resource filter is not working. #24875
Description
To handle the sorting issue, I changed the constants in the class SQLConstants.java which was enough to fix the sorting. This, however introduced another issue in the pagination logic, as that depended on the sorting to be done by CURSOR_KEY which the fix changed to be done by NAME. The pagination logic send the CURSOR_KEY of the last element of the current page to do the filtering at the SQL query, but since I changed the sorting to be done by NAME, sending the CURSOR_KEY does not make sense. So, I changed that logic in the PR link. This created another problem in the class FilterTreeBuilder.java, which checked pagination filters for the operator "pr". This worked earlier because CURSOR_KEY is an integer, but now since it contains text, parts of the NAME values got matched as operators. So I put a regex pattern there to make sure it matches that operator check only when it's at the end of the string. Other operators shouldn't cause this issue because they have spaces on both sides of the matching text. So, I left them untouched.
The fix for case insensitive sorting was straightforeward. I changed the SQL queries by adding the LOWER() function within the query in the file FilterQueriesUtil.java.
Before Change:
After Change:
Follow up actions
I have run related tests in my local environment. If possible, please run any related tests to see if this breaks any other parts of the code, since I have applied changes to some core parts of the system. Please let me know if I have missed anything.
Developer Checklist
Checklist (for reviewing)
General
Functionality
Code
Tests
Security
Documentation
Summary by CodeRabbit