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]
Skip to content

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

Merged
hmlnarik merged 1 commit into
keycloak:mainfrom
bosch-io:feature/#8749-Add_an_option_to_control_the_order_of_the_event_query_and_admin_event_query
Sep 11, 2022
Merged

Issue #8749: Add an option to control the order of the event query and admin event query#8822
hmlnarik merged 1 commit into
keycloak:mainfrom
bosch-io:feature/#8749-Add_an_option_to_control_the_order_of_the_event_query_and_admin_event_query

Conversation

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

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
hmlnarik commented Sep 2, 2022 •
edited
Loading
Copy link
Copy Markdown
Contributor

@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
hmlnarik commented Sep 2, 2022
Copy link
Copy Markdown
Contributor

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
sschu commented Sep 5, 2022
Copy link
Copy Markdown
Contributor

@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
hmlnarik commented Sep 6, 2022
Copy link
Copy Markdown
Contributor

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
sschu commented Sep 7, 2022
Copy link
Copy Markdown
Contributor

@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
sschu commented Sep 8, 2022
Copy link
Copy Markdown
Contributor

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

@sschu
sschu commented Sep 10, 2022
Copy link
Copy Markdown
Contributor

@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
@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.

Thank you for the PR!

@hmlnarik
hmlnarik commented Sep 11, 2022
Copy link
Copy Markdown
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
sschu commented Sep 13, 2022
Copy link
Copy Markdown
Contributor

@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

1 more reviewer
@hmlnarik hmlnarik hmlnarik approved these changes
Reviewers whose approvals may not affect merge requirements

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