8000 [KEYCLOAK-10735] implement search and filter for count endpoint by unly · Pull Request #6140 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[KEYCLOAK-10735] implement search and filter for count endpoint #6140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 3, 2020
Merged

[KEYCLOAK-10735] implement search and filter for count endpoint #6140

merged 1 commit into from
Feb 3, 2020

Conversation

unly
Copy link
Contributor
@unly unly commented Jul 2, 2019

@unly unly force-pushed the KEYCLOAK-10735 branch from 2e5af9c to e4b5c9e Compare July 2, 2019 14:46
@unly
Copy link
Contributor Author
unly commented Jul 3, 2019

looks like the testsuite that runs on travis isn't really stable. ran it twice and failed on 4 different tasks but also succeeded on every task at least once 😄

@unly unly force-pushed the KEYCLOAK-10735 branch from e4b5c9e to 4a9c78d Compare July 4, 2019 08:40
@stianst
Copy link
Contributor
stianst commented Jul 17, 2019

Thanks for the PR. Can you please remove formatting changes as it is making it significantly more work to review the PR?

@unly unly force-pushed the KEYCLOAK-10735 branch from 4a9c78d to aff5658 Compare July 17, 2019 14:53
@unly
Copy link
Contributor Author
unly commented Jul 17, 2019

Hi @stianst I just noticed how many auto format changes there were 😄 the update should be easier to review. we tried to do the /count endpoint analogous to the users endpoint

@unly unly force-pushed the KEYCLOAK-10735 branch 2 times, most recently from 4b02825 to d6bd395 Compare August 16, 2019 14:17
@unly
Copy link
Contributor Author
unly commented Oct 11, 2019

Hi @stianst I rebased the PR and the tests keep on failing but it doesn't seem to be related to the changes made in this PR. Can someone have a look at this PR?

@stianst
Copy link
Contributor
stianst commented Oct 17, 2019

@unly crossdc-server tests are screwed up so you can ignore those. Will try to get someone to review this asap, but we do have a bit of a backlog of reviews to get through, so please bear with us.

@hmlnarik hmlnarik requested a review from vramik January 10, 2020 13:28
@hmlnarik
Copy link
Contributor

@vramik Could you please have a look at this PR?

vramik
vramik suggested changes Jan 14, 2020
View reviewed changes
Copy link
Contributor
@vramik vramik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unly thank you very much for the PR. Overall it looks good to me, there can be used assertThat instread of assertEquals and assertTrue in UsersTest. assertThat has much more readable output.

There are SQLExceptions when using the new endpoints:

MariaDB/MySQL

...
Caused by: java.sql.SQLException: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB/MySQL server version for the right syntax to use near '))' at line 1
Query is: select count(usergroupm0_.USER_ID) as col_0_0_ from USER_GROUP_MEMBERSHIP usergroupm0_ cross join USER_ENTITY userentity1_ where usergroupm0_.USER_ID=userentity1_.ID and userentity1_.REALM_ID=? and (userentity1_.SERVICE_ACCOUNT_CLIENT_LINK is null) and (lower(userentity1_.USERNAME) like ? or lower(concat(userentity1_.FIRST_NAME, ' ', userentity1_.LAST_NAME)) like ? or userentity1_.EMAIL like ?) and (usergroupm0_.GROUP_ID in ()), parameters ['admin-client-test','%user%','%user%','%user%']

MSSQL

...
Caused by: com.microsoft.sqlserver.jdbc.SQLServerException: Incorrect syntax near ')'.

Postgresql

...
Caused by: org.postgresql.util.PSQLException: ERROR: syntax error at or near ")"

Oracle

...
Caused by: Error : 936, Position : 223, Sql = select count(usergroupm0_.USER_ID) as col_0_0_ from USER_GROUP_MEMBERSHIP usergroupm0_ cross join USER_ENTITY userentity1_ where usergroupm0_.USER_ID=userentity1_.ID and userentity1_.REALM_ID=:1  and (usergroupm0_.GROUP_ID in ()), OriginalSql = select count(usergroupm0_.USER_ID) as col_0_0_ from USER_GROUP_MEMBERSHIP usergroupm0_ cross join USER_ENTITY userentity1_ where usergroupm0_.USER_ID=userentity1_.ID and userentity1_.REALM_ID=? and (usergroupm0_.GROUP_ID in ()), Error Msg = ORA-00936: missing expression

@unly
Copy link
Contributor Author
unly commented Jan 15, 2020

hey @vramik I updated the PR but it's been a while since I originally raised it and therefore I'm not a 100% sure. Can you please have another look? Also how can I efficiently test it with all those different implementations? So far I mostly tested the unit tests

@vramik
Copy link
Contributor
vramik commented Jan 15, 2020 •
edited
Loading

@unly thanks for the update and for the patience for this PR. Unfortunately I still get the SQLExceptions. There is link to https://github.com/keycloak/keycloak/blob/master/docs/tests-db.md where you can find useful information about DB testing. Especially in section "Using built-in profiles to run database tests using docker containers"

Following command should show the issue in your environment with MySQL.

mvn -f testsuite/integration-arquillian/pom.xml clean verify -Pdb-mysql,jpa,auth-server-wildfly -Dtest=UsersTest

Let me know if you need anything. Thanks

@unly
Copy link
Contributor Author
unly commented Jan 17, 2020

Hey @vramik I tried to run those tests but nothing seems to work for me. First, I tried https://github.com/keycloak/keycloak/blob/master/docs/tests-db.md#mysql to get started. But all I got is an error in testSaml20AuthnResponseNonAsciiNameDefaultLatin2. The same error occurs on master.

When I run mvn -f testsuite/integration-arquillian clean verify -Pdb-mysql it runs test for a long while (more than an hour for sure) and in the end I just get [ERROR] Tests run: 2869, Failures: 1, Errors: 391, Skipped: 191. Just by the huge amount of other errors I find it hard to find the SQL query errors you listed above. On master a similar amount of errors. Is there something i'm missing? I would expect the master to succeed and my branch to fail on something related to my changes. Thankful for any kind of help 😄

@vramik
Copy link
Contributor
vramik commented Jan 20, 2020

@unly mvn -f testsuite/integration-arquillian/pom.xml clean verify -Pdb-mysql,jpa,auth-server-wildfly -Dtest=UsersTest should run only UsersTest test class, what you've added. It seems you've omitted part of the command.

Please note you should have keycloak-server-dist built. When I perform following steps I get the issue:

  • mvn clean install -Pdistribution -DskipTests
  • mvn -f testsuite/integration-arquillian/pom.xml clean verify -Pdb-mysql,jpa,auth-server-wildfly -Dtest=UsersTest

Together it takes ~10 minutes in my environment.

integration-arquillian-tests-base module is the one where the test is run.

@unly
Copy link
Contributor Author
unly commented Jan 21, 2020

Hi @vramik thank you so much for the exact commands. This is a life saver. Now I was able to reproduce the errors for MySql. Based on this I updated the commit and ran the tests again for MySql and Postgres and it succeeded on both databases. Can you have another look?

@unly
Copy link
Contributor Author
unly commented Jan 21, 2020

Another thing I was wondering is what this about the complicated commands to run tests. Often I just rely on travis to run tests for me as I can't make them run locally. When I check out a repo from GitHub I would expect mvn install to compile and run tests. The markdown page provides commands to tests with the database and I immediately get a ton of errors and failures. Do you have any plans to update this to make the master pass without any external requirements? It's hard for me to precheck correctly and push with a certain confidence that it's working.

@vramik
Copy link
Contributor
vramik commented Jan 23, 2020

@unly thank you for the fix, I can confirm it behaves correctly now.

Regarding complicated testsuite: You're right, it's a complicated thing, especially for people which are not familiar with it. We're not testing everything when you run mvn clean install because it would take a lifetime :) also for some test it is required to have special configuration. So it became quite a complex testsuite. Let's see if we cvan somehow improve it in future. For now it is what it is :)

vramik
vramik approved these changes Jan 23, 2020
View reviewed changes
Copy link
Contributor
@vramik vramik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hmlnarik it's ready to be merged imo.

https://keycloak-jenkins.com/job/universal-test-pipeline-server/1230/

@unly
Copy link
Contributor Author
unly commented Jan 31, 2020

Thanks @vramik thanks for reviewing. what about this pr? @hmlnarik is it ready to merge?

hmlnarik
hmlnarik approved these changes Feb 3, 2020
View reviewed changes
Copy link
Contributor
@hmlnarik hmlnarik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @unly and @vramik !

@hmlnarik
Copy link
Contributor
hmlnarik commented Feb 3, 2020

Travis errors are irrelevant

@hmlnarik hmlnarik merged commit 01a42f4 into keycloak:master Feb 3, 2020
@abstractj abstractj mentioned this pull request Mar 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vramik vramik vramik approved these changes

@hmlnarik hmlnarik hmlnarik approved these changes

Assignees

@hmlnarik hmlnarik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@unly @stianst @hmlnarik @vramik
0