8000 Rework the result for the session search to contain a single result per user sessions by ahus1 · Pull Request #29204 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Rework the result for the session search to contain a single result per user sessions - #29204

Merged
ahus1 merged 1 commit into
keycloak:mainfrom
ahus1:is-29203-searching-sessions-optimization
May 3, 2024
Merged

Rework the result for the session search to contain a single result per user sessions#29204
ahus1 merged 1 commit into
keycloak:mainfrom
ahus1:is-29203-searching-sessions-optimization

Conversation

@ahus1
@ahus1 ahus1 commented May 2, 2024 •
edited
Loading
Copy link
Copy Markdown
Member

Closes #29203

This now returns only one entry per user session id and offline flag. A user session can then contain more than once client session, and both clients are presented in the UI. In the UI, the user can then log out the user session as before. As they are not logging out a single client session, this is IMHO how the UI should behave.

@ahus1 ahus1 self-assigned this May 2, 2024
This was referenced May 2, 2024
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
hmlnarik
hmlnarik reviewed May 2, 2024
View reviewed changes
@hmlnarik hmlnarik left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @ahus1. Couple of comments left for further discussion.

Comment thread rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/SessionsResource.java Resolved
Comment thread rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/SessionsResource.java Resolved
Comment thread rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/SessionsResource.java Resolved
Comment thread rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/SessionsResource.java Resolved
@ahus1
ahus1 force-pushed the is-29203-searching-sessions-optimization branch from e9b911e to 63f6dcf Compare May 2, 2024 14:54
@ahus1 ahus1 changed the title Filter user sessions in the backend as much as possible Rework the result for the session search to contain a single result per user sessions May 2, 2024
@ahus1
ahus1 force-pushed the is-29203-searching-sessions-optimization branch 2 times, most recently from 7c2d3a3 to 001dcd4 Compare May 2, 2024 15:02
keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed May 2, 2024
View reviewed changes
@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@keycloak-github-bot
keycloak-github-bot Bot commented May 2, 2024
Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testExternalExchangeCreateNewUserUsingMappers

Keycloak CI - Base IT

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testExternalExchange

Keycloak CI - Bas 8000 e IT

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testAccountLinkNoTokenStore

Keycloak CI - Base IT

java.lang.AssertionError: Unexpected page. Current Page URL: http://localhost:8280/exchange-linking/link?realm=child&provider=parent-idp
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest.testAccountLinkNoTokenStore(BrokerLinkAndTokenExchangeTest.java:494)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

Report flaky test

org.keycloak.testsuite.client.ClientTypesTest#testUpdateClientWithClientType

Keycloak CI - Base IT (4)

java.lang.AssertionError: Not expected to update client
	at org.junit.Assert.fail(Assert.java:89)
	at org.keycloak.testsuite.client.ClientTypesTest.testUpdateClientWithClientType(ClientTypesTest.java:112)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeWithDynamicScopesEnabled

Keycloak CI - Base IT (6)

org.keycloak.common.VerificationException: Token not set
	at org.keycloak.TokenVerifier.parse(TokenVerifier.java:401)
	at org.keycloak.testsuite.oauth.ClientTokenExchangeTest.testExchange(ClientTokenExchangeTest.java:309)
	at org.keycloak.testsuite.oauth.ClientTokenExchangeTest.testExchangeWithDynamicScopesEnabled(ClientTokenExchangeTest.java:1021)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testClientExchange

Keycloak CI - Base IT (6)

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testIntrospectTokenAfterImpersonation

Keycloak CI - Base IT (6)

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testPublicClientNotAllowed

Keycloak CI - Base IT (6)

java.lang.AssertionError: expected:<403> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeUsingServiceAccount

Keycloak CI - Base IT (6)

org.keycloak.common.VerificationException: Token not set
	at org.keycloak.TokenVerifier.parse(TokenVerifier.java:401)
	at org.keycloak.testsuite.oauth.ClientTokenExchangeTest.testExchangeUsingServiceAccount(ClientTokenExchangeTest.java:347)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonation

Keycloak CI - Base IT (6)

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonationUsingPublicClient

Keycloak CI - Base IT (6)

java.lang.AssertionError: expected:<200> but was:<400>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

@ahus1
ahus1 marked this pull request as ready for review May 2, 2024 16:20
@ahus1
ahus1 requested a review from a team as a code owner May 2, 2024 16:20
@ahus1
ahus1 requested a review from hmlnarik May 2, 2024 16:20
@ahus1
ahus1 commented May 2, 2024
Copy link
Copy Markdown
Member Author

@hmlnarik - this is now ready for review when you have the time. IMHO the unstable tests are unrelated to this change. Thanks!

@hmlnarik
hmlnarik commented May 3, 2024 •
edited
Loading
Copy link
Copy Markdown
Contributor

@ahus1 The change did not start Javascript CI tests since those are skipped. Could you please add

rest/admin-ui-ext js

to .github/actions/conditional/conditions? It should be there in any case.

@ahus1
Rework the result for the session search to contain a single result p…
8049bd5 
…er user sessions

Closes keycloak#29203

Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
@ahus1
ahus1 force-pushed the is-29203-searching-sessions-optimization branch from 001dcd4 to 8049bd5 Compare May 3, 2024 10:10
@ahus1
ahus1 requested a review from a team as a code owner May 3, 2024 10:10
@ahus1
ahus1 commented May 3, 2024
Copy link
Copy Markdown
Member Author

@hmlnarik - added the change, and now the JS tests ran and are all green. Other parts of the build are still running, still I assume they will be green as they have been green before.

hmlnarik
hmlnarik approved these changes May 3, 2024
View reviewed changes
@hmlnarik hmlnarik left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @ahus1! Approved.

@ahus1
ahus1 enabled auto-merge (rebase) May 3, 2024 10:57
@ahus1
ahus1 merged commit df47dee into keycloak:main May 3, 2024
This was referenced May 3, 2024
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments
@hmlnarik hmlnarik hmlnarik approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@ahus1 ahus1

Labels

flaky-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Revisit SessionsResource#realmSessions as it current loads all sessions into memory

2 participants

@ahus1 @hmlnarik
0