Events Map JPA implementation by sguilhen · Pull Request #12233 · keycloak/keycloak

sguilhen · 2022-05-27T23:13:34Z

sguilhen · 2022-05-30T12:40:22Z

@vramik I've borrowed the above code from your authz PR. Once that is merged I'll update.

sguilhen · 2022-05-30T12:41:30Z

Question: should the expiration be part of this index? It is used on every query to avoid returning expired entities.

Not sure to be honest. It might make sense to have it due to being part of each search as you said. On the other hand expiration field would be changed very often which would mean the need for re-calculating the index very often.

I'd go without it for now as database should be able to uset currently existing index and then "just" filter non-expired objects. And if it turns out the index is needed it can be added later, wdyt?

Seems reasonable to me, will remove it from the index.

sguilhen · 2022-05-30T12:43:37Z

Question: This is similar to the attributes tables we have for other entities, and those have an index for the name/value. I haven´t added such index here because the details are never queried by name or value, they are always loaded in their entirety, so to me only the fk_root index should be enough. Thoughts?

+1 for only having fk_root index

sguilhen · 2022-05-30T12:46:29Z

@vramik I've added the delegate for now, although we might decide to simply load everything - including the metadata - in read with parameters. You mentioned you would follow this approach for the user sessions, and if you do I'll update this one to be consistent with what you are doing there.

Changed, both event entities no longer have a delegate.

vramik

Thank you @sguilhen for the PR! I've done first round of review and have some comments, please see inline. I'll take another look once #12299 is merged.

vramik · 2022-05-31T11:30:19Z

Didn't you mean?

Suggested change

private static final Map<String, String> FIELD_TO_JASON_PROP = new HashMap<>();

private static final Map<String, String> FIELD_TO_JSON_PROP = new HashMap<>();

Ah yes, definitely.. nice catch!

vramik · 2022-05-31T11:42:25Z

dtto

Suggested change

private static final Map<String, String> FIELD_TO_JASON_PROP = new HashMap<>();

private static final Map<String, String> FIELD_TO_JSON_PROP = new HashMap<>();

vramik · 2022-06-08T15:47:33Z

I believe we shouldn't transform ResouceType to String at this level, the searchable field states that it expects ResourceType.class as a filed type. I'd suggest rather change MapFiledsPredicates class as mentioned in #12299 (comment) but fist we should decide how the search should behave for ResourceType.CUSTOM (#12299 (comment))

Agreed, will revert this change.

vramik · 2022-06-08T15:48:06Z

out of curiosity: why this change?

In the first version I've pushed I had it as a stream to be able to map it to String, but I've reverted that change and this is something that wasn't properly reverted. Will change it.

vramik · 2022-06-08T15:49:21Z

+1 for only having fk_root index

vramik · 2022-06-08T15:53:39Z

Not sure to be honest. It might make sense to have it due to being part of each search as you said. On the other hand expiration field would be changed very often which would mean the need for re-calculating the index very often.

I'd go without it for now as database should be able to uset currently existing index and then "just" filter non-expired objects. And if it turns out the index is needed it can be added later, wdyt?

hmlnarik · 2022-06-15T14:46:42Z

Could you please use a different separator than . for eventsStore.admin, e.g. eventsStore-admin or (perhaps better) even adminEventsStore?

vramik

LGTM, thank you @sguilhen!

Depending whether #12230 will be merged first, this one will require rebase due to the EXPIRATION removal.

hmlnarik · 2022-06-20T20:01:48Z

@sguilhen #12230 is merged, could you please rebase?

Closes keycloak#9667

sguilhen · 2022-06-20T20:24:51Z

@hmlnarik Rebased!

vramik

Thanks @sguilhen, great work!

vramik

@sguilhen one question popped up - I assumed we should filter expired entities on the database level, but I cannot see where it is done. I can see expired entities are handled on model level within MapEventStoreProvider, but that way we'd load all entities from database. Did I got it wrong?

vramik · 2022-06-21T09:12:43Z

@sguilhen one question popped up - I assumed we should filter expired entities on the database level, but I cannot see where it is done. I can see expired entities are handled on model level within MapEventStoreProvider, but that way we'd load all entities from database. Did I got it wrong?

I've just seen #12623. Just to be sure, we'd tackle this within that issue?

sguilhen · 2022-06-21T11:31:57Z

@sguilhen one question popped up - I assumed we should filter expired entities on the database level, but I cannot see where it is done. I can see expired entities are handled on model level within MapEventStoreProvider, but that way we'd load all entities from database. Did I got it wrong?

I've just seen #12623. Just to be sure, we'd tackle this within that issue?

Yes, that's correct. Idea is to handle this in JpaMapKeycloakTransaction for all expirable entities, so the criteria builders for each area don't have to append expiration criteria to every query.

hmlnarik

Approving per @vramik 's review

sguilhen requested a review from vramik May 27, 2022 23:13

sguilhen assigned hmlnarik May 27, 2022

sguilhen added area/storage Indicates an issue that touches storage (change in data layout or data manipulation) team/storage-sig labels May 27, 2022

sguilhen force-pushed the 9667-events-jpa-store branch 4 times, most recently from 6144978 to 9543526 Compare May 29, 2022 02:37

sguilhen commented May 30, 2022

View reviewed changes

sguilhen force-pushed the 9667-events-jpa-store branch 4 times, most recently from c38877b to 9467c46 Compare June 8, 2022 15:20

vramik reviewed Jun 8, 2022

View reviewed changes

sguilhen force-pushed the 9667-events-jpa-store branch from 9467c46 to 5e02672 Compare June 8, 2022 18:13

sguilhen requested a review from vramik June 10, 2022 12:00

sguilhen force-pushed the 9667-events-jpa-store branch from 5e02672 to 278871d Compare June 10, 2022 13:20

hmlnarik reviewed Jun 15, 2022

View reviewed changes

Comment thread model/map-jpa/src/main/resources/META-INF/admin-events/jpa-admin-events-changelog-1.xml Outdated

hmlnarik reviewed Jun 15, 2022

View reviewed changes

hmlnarik mentioned this pull request Jun 15, 2022

Implementation for feature Expiration for Admin Events #12476 #12501

Merged

sguilhen force-pushed the 9667-events-jpa-store branch 2 times, most recently from c3240ba to 57dd4b4 Compare June 17, 2022 13:45

vramik previously approved these changes Jun 20, 2022

View reviewed changes

Events Map JPA implementation

5569262

Closes keycloak#9667

sguilhen dismissed vramik’s stale review via 5569262 June 20, 2022 20:23

sguilhen force-pushed the 9667-events-jpa-store branch from 57dd4b4 to 5569262 Compare June 20, 2022 20:23

hmlnarik requested a review from vramik June 20, 2022 20:50

vramik approved these changes Jun 21, 2022

View reviewed changes

vramik reviewed Jun 21, 2022

View reviewed changes

hmlnarik approved these changes Jun 21, 2022

View reviewed changes

hmlnarik merged commit 7d96f3a into keycloak:main Jun 21, 2022

sguilhen deleted the 9667-events-jpa-store branch October 19, 2022 14:05

	private static final Map<String, String> FIELD_TO_JASON_PROP = new HashMap<>();
	private static final Map<String, String> FIELD_TO_JSON_PROP = new HashMap<>();

Conversation

sguilhen commented May 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sguilhen May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vramik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vramik left a comment

Choose a reason for hiding this comment

Uh oh!

hmlnarik commented Jun 20, 2022

Uh oh!

sguilhen commented Jun 20, 2022

Uh oh!

vramik left a comment

Choose a reason for hiding this comment

Uh oh!

vramik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vramik commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sguilhen commented Jun 21, 2022

Uh oh!

hmlnarik left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sguilhen May 30, 2022 •

edited

Loading

vramik left a comment •

edited

Loading

vramik commented Jun 21, 2022 •

edited

Loading