You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@syncope.apache.org by gonzalad <gi...@git.apache.org> on 2016/11/21 17:15:46 UTC
[GitHub] syncope pull request #40: SYNCOPE-971: Case insensitive search
GitHub user gonzalad opened a pull request:
https://github.com/apache/syncope/pull/40
SYNCOPE-971: Case insensitive search
Global setting for activating global insensitive
search.
Insensitive search is activated when search.ignore.case
property is configured to true (default value: false).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gonzalad/syncope SYNCOPE-971
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/syncope/pull/40.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #40
----
commit 2db61c29e7be244e365d834864d9fba97e72c9c1
Author: gonzalad <ad...@yahoo.fr>
Date: 2016-11-21T15:15:57Z
SYNCOPE-971: Case insensitive search
Global setting for activating global insensitive
search.
Insensitive search is activated when search.ignore.case
property is configured to true (default value: false).
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by ilgrosso <gi...@git.apache.org>.
Github user ilgrosso commented on the issue:
https://github.com/apache/syncope/pull/40
Hi @gonzalad thanks for your contribution!
I have noticed that you have defined an additional configuration parameter which globally states if `EQ` and `LIKE` are to be considered as case insensitive or not.
I would rather prefer introducing new operators for this purpose (`IEQ` and `ILIKE` ?).
Naturally, query builders will also need to be adjusted accordingly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by ilgrosso <gi...@git.apache.org>.
Github user ilgrosso commented on the issue:
https://github.com/apache/syncope/pull/40
@gonzalad thanks for your work, the changes now look very good!
I have some troubles when merging your PR directly (as you've forked from the master branch, I think) + there are some minor issues to clean up (unused `ConfDAO`, for example).
Moreover, the CXF dependency must be upgraded to 3.1.9-SNAPSHOT in oder to get the changes pushed yesterday.
The error from `MigrationITCase` is randomly occurring, and it is probably related to some timing issue, so do not care about it.
Finally, once merged this PR, I will apply it to the `master` branch too (with no modifications) + I will port it to the `1_2_X` branch (which will instead require some intervention.
Thanks again!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by gonzalad <gi...@git.apache.org>.
Github user gonzalad commented on the issue:
https://github.com/apache/syncope/pull/40
@ilgrosso : yes I confirm, I forked from master (I didn't know I needed to use another branch sorry !)
Are you taking care of the rest or should I do a little cleanup ?
If I need to do some cleanup, you'll need to tell me which branch should I use for the fork (btw, master branch depends on CXF 3.2.0-SNAPSHOT)
Thanks !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by ilgrosso <gi...@git.apache.org>.
Github user ilgrosso commented on the issue:
https://github.com/apache/syncope/pull/40
Before closing this PR I'll cherry-pick and adapt to `master` + rework for `1_2_X`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by ilgrosso <gi...@git.apache.org>.
Github user ilgrosso commented on the issue:
https://github.com/apache/syncope/pull/40
> Are you taking care of the rest or should I do a little cleanup ?
I am already at work, expect something soon :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by gonzalad <gi...@git.apache.org>.
Github user gonzalad commented on the issue:
https://github.com/apache/syncope/pull/40
Hi @ilgrosso, the problem is I don't know how to create custom FIQL operators.
From what I've seen, we can't.
FiqlParser.operatorsMap is private & non-accessible :
https://github.com/gonzalad/cxf/blob/315e357e850407e9f3233259bab33ea765633271/rt/rs/extensions/search/src/main/java/org/apache/cxf/jaxrs/ext/search/fiql/FiqlParser.java#L100
If you know how to implement custom fiql operators just let me know (from what I've seen in Syncope you use only native fiql operators)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by ilgrosso <gi...@git.apache.org>.
Github user ilgrosso commented on the issue:
https://github.com/apache/syncope/pull/40
@gonzalad let me play a bit with CXF's `FiqlParser`...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope pull request #40: SYNCOPE-971: Case insensitive search
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/syncope/pull/40
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by ilgrosso <gi...@git.apache.org>.
Github user ilgrosso commented on the issue:
https://github.com/apache/syncope/pull/40
@gonzalad With https://github.com/apache/cxf/commit/5156dd256f8f59d22ea2d1ca50197f1b5b3f1788 (for CXF's `3.1.x-fixes`, which is currently used by Syncope's `2_0_X`, but I have also committed to CXF's `3.0.x-fixes` and `master`) I have pushed some changes to allow easier subclassing of `FiqlParser`.
I have also set up a [simple project](https://github.com/ilgrosso/FIQLCustomOperator) to show how to define two custom operators `=~` (case-insensitive equals) and `!~` (case-insensitive not equals).
If you would like to try it yourself, you will need to build CXF 3.1.9-SNAPSHOT from sources.
@sberyozkin WDYT?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by sberyozkin <gi...@git.apache.org>.
Github user sberyozkin commented on the issue:
https://github.com/apache/syncope/pull/40
Hi Francesco, this looks fine.
In general, as far as the use of the CXF Search extension is concerned, it might make it tricky to reuse a CXF ODataParser (delegates to Olingo) which offers a limited OData filter support given that OData offers no such operators - however perhaps Odata functions can be supported later on if ODataParser will be used at some point of time.
Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by ilgrosso <gi...@git.apache.org>.
Github user ilgrosso commented on the issue:
https://github.com/apache/syncope/pull/40
I have just committed to `1_2_X` with 4adf879d3ce446e1054ddc03a929e31ae25bd248
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by gonzalad <gi...@git.apache.org>.
Github user gonzalad commented on the issue:
https://github.com/apache/syncope/pull/40
@ilgrosso : changes made, I've introduced the 2 new operators =~ and !~.
Thanks for the change in CXF fiqlParser !
2 problems in Travis CI :
* the build doesn't pass (cxf changes were not published I think)
* MigrationITCase.migrateFromSyncope12 fails (I don't think this failure is caused by this changed since this test also fails in Pull Request #39)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope pull request #40: SYNCOPE-971: Case insensitive search
Posted by ilgrosso <gi...@git.apache.org>.
Github user ilgrosso commented on a diff in the pull request:
https://github.com/apache/syncope/pull/40#discussion_r89059365
--- Diff: core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/inner/AnySearchTest.java ---
@@ -175,6 +210,28 @@ public void searchWithNotCondition() {
}
@Test
+ public void searchCaseInsensitiveWithNotCondition() {
--- End diff --
This test should be replicated in [SearchITCase](https://github.com/apache/syncope/blob/2_0_X/fit/core-reference/src/test/java/org/apache/syncope/fit/core/SearchITCase.java) (like other test cases in this class)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope pull request #40: SYNCOPE-971: Case insensitive search
Posted by ilgrosso <gi...@git.apache.org>.
Github user ilgrosso commented on a diff in the pull request:
https://github.com/apache/syncope/pull/40#discussion_r89058906
--- Diff: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAnySearchDAO.java ---
@@ -769,7 +783,12 @@ private void fillAttributeQuery(final StringBuilder query, final PlainAttrValue
if (not) {
query.append(" NOT ");
}
- query.append(" LIKE ?").append(setParameter(parameters, cond.getExpression()));
+ query.append(" LIKE ");
--- End diff --
Ah, it seems you're right, good to know!
For additional safety, I have found references for [H2](http://www.h2database.com/html/functions.html#lower), [MySQL](http://www.sqlinfo.net/mysql/mysql_function_upper_lower.php), [PostgreSQL](https://www.postgresql.org/docs/9.3/static/functions-string.html), [Oracle DB](http://www.sqlinfo.net/oracle/oracle_function_upper_lower.php), [MS SQL Server](http://www.sqlinfo.net/sqlserver/sql_server_function_upper_lower.php), hence all of our supported DBMSes
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] syncope issue #40: SYNCOPE-971: Case insensitive search
Posted by ilgrosso <gi...@git.apache.org>.
Github user ilgrosso commented on the issue:
https://github.com/apache/syncope/pull/40
I have just committed to `2_0_X` with 0e214e0046e3e9940cbe57dacf3f8bf56c1b5f9c
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---