8000 Issue #8749: Add an option to control the order of the event query and admin event query by leischt · Pull Request #8822 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Issue #8749: Add an option to control the order of the event query and admin event query #8822

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
Sep 11, 2022
Merged

Issue #8749: Add an option to control the order of the event query and admin event query #8822

merged 1 commit into from
Sep 11, 2022

Conversation

leischt
Copy link
Contributor
@leischt leischt commented Nov 15, 2021 •
edited
Loading

Closes #8749

@leischt leischt mentioned this pull request Nov 15, 2021
Closed
@leischt leischt force-pushed the feature/#8749-Add_an_option_to_control_the_order_of_the_event_query_and_admin_event_query branch 2 times, most recently from 79b7c32 to 6313936 Compare June 13, 2022 07:09
@leischt leischt force-pushed the feature/#8749-Add_an_option_to_control_the_order_of_the_event_query_and_admin_event_query branch from 6313936 to 07f8446 Compare August 25, 2022 07:29
@stianst stianst added the priority/important Must be worked on very soon label Sep 2, 2022
@hmlnarik
Copy link
Contributor
hmlnarik commented Sep 2, 2022 •
edited
Loading

@leischt Thank you for the PR. The test failures seem relevant. Have you checked those?
@mhajas Could you please check the hot-rod failure?

@hmlnarik
Copy link
Contributor
hmlnarik commented Sep 2, 2022

It seems that the test is neither using testingProvider nor setting the time of the events, hence the event time is 0 and the order is then undefined.
Thanks @mhajas for this finding!

@sschu sschu force-pushed the feature/#8749-Add_an_option_to_control_the_order_of_the_event_query_and_admin_event_query branch from 07f8446 to b3b43ca Compare September 5, 2022 12:55
@sschu
Copy link
Contributor
sschu commented Sep 5, 2022

@hmlnarik I added timestamps for the events similarly how it is done in other tests.

@sschu sschu force-pushed the feature/#8749-Add_an_option_to_control_the_order_of_the_event_query_and_admin_event_query branch from b3b43ca to 255b14c Compare September 5, 2022 17:35
@hmlnarik
Copy link
Contributor
hmlnarik commented Sep 6, 2022

Thank you @sschu, the code after update looks good. Could you please check the model test failures in legacy-jpa profiles though?
Please update the commit messages and squash the commits, keycloak#10656 is not related to this PR.

@sschu sschu force-pushed the feature/#8749-Add_an_option_to_control_the_order_of_the_event_query_and_admin_event_query branch from 255b14c to 97126b4 Compare September 7, 2022 10:24
@sschu
Copy link
Contributor
sschu commented Sep 7, 2022

@hmlnarik Sorry for the delay, I stumbled across a really nasty bug not setting the realm as expected when creating events using the AdminEventBuilder. That led to events not properly cleaned up after tests and led to some longer debugging sessions. But the respective tests are green now locally, so I hope the CI agrees.

Btw. do you have any idea why this test here: https://github.com/keycloak/keycloak/blob/aec8e6af509af1aa83b3049edf38763a3b000e7c/testsuite/model/src/test/java/org/keycloak/testsuite/model/events/AdminEventQueryTest.java#L78-#L100
has to be broken up into two withRealm sections? If I put everything int he same section, the assertion fails since all four events are found instead of two (at least when using the MapStorageProvider).

@sschu
Copy link
Contributor
sschu commented Sep 8, 2022

@hmlnarik The failing test is AccountRestServiceWithUserProfileTest>AccountRestServiceTest.listApplicationsThirdParty:966 expected:<[]> but was:<[web-origins]>, probably not related.

@sschu
Copy link
Contributor
sschu commented Sep 10, 2022

@hmlnarik Now everything is green (except code check, but that seems to always fail currently).

hmlnarik
hmlnarik approved these changes Sep 11, 2022
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.

Thank you for the PR!

@hmlnarik
Copy link
Contributor

Btw. do you have any idea why this test here: https://github.com/keycloak/keycloak/blob/aec8e6af509af1aa83b3049edf38763a3b000e7c/testsuite/model/src/test/java/org/keycloak/testsuite/model/events/AdminEventQueryTest.java#L78-#L100
has to be broken up into two withRealm sections? If I put everything int he same section, the assertion fails since all four events are found instead of two (at least when using the MapStorageProvider).

I am sad to hear this had crossed the development of your tests.

Does this happen for e.g. map-jpa or only for map profile? If the latter, this might be because the ConcurrentHashMap implementation of the MapKeycloakTransaction is relaxed in this regard:

keycloak/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapKeycloakTransaction.java

Lines 187 to 199 in 1d2d3e5

Stream<V> updatedAndNotRemovedObjectsStream = this.map.read(queryParameters)
.filter(filterOutAllBulkDeletedObjects)
.map(this::getUpdated) // If the object has been removed, tx.get will return null, otherwise it will return me.getValue()
.filter(Objects::nonNull)
.map(this::registerEntityForChanges);
// In case of created values stored in MapKeycloakTransaction, we need filter those according to the filter
Stream<V> res = mapMcb == null
? updatedAndNotRemovedObjectsStream
: Stream.concat(
createdValuesStream(mapMcb.getKeyFilter(), mapMcb.getEntityFilter()),
updatedAndNotRemovedObjectsStream
);

This has been a conscious decision for this development storage - to avoid implementing tricky merging results coming from the underlying store with those from the current transaction. In the server code it does not hurt since the request processing either writes or reads the same data in a transaction, not both at the same time. For the tests, the right way would be exactly the way you did with splitting the transaction. In fact, such merging will be only implemented properly in the tree store (#9742).

@hmlnarik hmlnarik merged commit 7e5b45f into keycloak:main Sep 11, 2022
@sschu
Copy link
Contributor
sschu commented Sep 13, 2022

@hmlnarik Indeed, it only happens on map, not on map-jpa. So I assume in the future I will consider tests on map primarily?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmlnarik hmlnarik hmlnarik approved these changes

Assignees

@hmlnarik hmlnarik

Labels
priority/important Must be worked on very soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an option to control the order of the event query and admin event query
4 participants
@leischt @hmlnarik @sschu @stianst
0