Repository navigation
Users Map JPA implementation - #12871
Conversation
|
Integration testsuite was run locally using both PostgreSQL and CockroachDB. Only one test is currently failing, It is unclear to me what exactly is the purpose of this test. It mentions Jira 3296, in which concurrent logins with the same user failed with the same exception shown above, but |
There was a problem hiding this comment.
I think this is missing a unique index for username + realmid, as a username should exist only once per realm
There was a problem hiding this comment.
Another thought: as the two missing indexes haven been found by tests, there should be two new tests for unique email in realm and unique user name in realm.
There was a problem hiding this comment.
... but these new tests should be in a new issue and pull request.
There was a problem hiding this comment.
Agreed. If that is in place, then having an index targeting only the realmid might not be necessary, right?
There was a problem hiding this comment.
It would not be necessary if the realmid is the first column of that index. If it is the second one, it will still be necessary AFAIK. I was arguing above that an index with userid first might be more efficient for the constraint, as a lot of usernames would share the same realmid, but not vice versa. A load test would still need to prove that. You can decide to defer sich optimizations.
There was a problem hiding this comment.
On the missing tests, I've checked UserTest and it does have tests for the unique username and unique emails. But these are enforced at the provider level, so it never reaches the DB where we would notice the missing constraints. Do you think we should add tests that work at the entity level directly (bypassing the provider) to ensure the constraints are in place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking that up. I see that this happens for example in MapUserAdapter for those stores that don't implement that constraint on the store level. This will be "best effort". Hm. It would be great to have an isolated test on the JPA map store, which we don't have and probably never will have. So let's skip the automated test, I'll do a manual test once the index is there by trying to update the DB.
So it is only to add the index.
There was a problem hiding this comment.
Another thought: as the two missing indexes haven been found by tests, there should be two new tests for unique email in realm and unique user name in realm.
Note to my future self: before writing a comment like this, have a look at the existing code.
There was a problem hiding this comment.
The old JPA store had a separate field email_constraint; when the realm had unique email enabled, that column would be filled, and the DB would ensure that there wouldn't be two users with the same email address.
I would assume that there should be another extracted column as this is also in the map store stored in a separate field - plus a unique index for email_constraint + realmId.
There was a problem hiding this comment.
Thanks for spotting this, will add the index.
@sguilhen - reading the issue it could happen with federation enabled, that a user attribute is written during user login. None of the other map storage implementations does any concurrency checking, only JPA map has optimistic locking. Therefore, only JPA can see the possible inconsistency, and will return an error. To avoid something like that, pessimistic locking can help. @vramik and me had a look and this is now part of #12241. There is another issue #12717 that would make it more generic, and then one could also lock users, and not only usersessions, and then this test can succeed. Until then, let's discuss on how to make that happen ... maybe disable the test, and add a new issue, or make it part of #12717 to re-enable it for the map storage. And then we could try to prioritize to get those to PRs in. |
|
As we discussed in the team call, this test seems to be aiming at the workaround/fix that was added for KEYCLOAK-3296 to fix the issue with concurrent logins. If we come across such an issue with the map storage (which might not even happen given that the concurrent login test currently succeeds), we will fix it using a different mechanism - the locking support you mentioned above instead of the HQL used in KEYCLOAK-3296. So the test that focuses on the fix for the legacy store issue doesn't really make sense when map storage is in place. Thus we've agreed on ignoring this test if the map storage is enabled. |
Issue to revisit this once the lock for update mechanism is in place and all JPA stores are completed: #12910 |
69ff1b3 to
040bb5a
Compare
|
@ahus1 I've addressed all the suggestions, please have another look (specially at the indexes) when you have some time. |
There was a problem hiding this comment.
I'm approving this, see my comment below about naming the columns and if you want to change those.
I see that this uses a CollectionTable, let's see how this performs with in a load test as this would always load all roles and groups of a user - this is outside of this PR. Maybe we could use the same structure I was implementing for the composite roles - let's first see if this works well over there before we apply that here.
| <column name="role_id" type="KC_KEY"/> | ||
| </createTable> | ||
| <createIndex tableName="kc_user_role" indexName="user_role_userId"> | ||
| <column name="user_id"/> |
There was a problem hiding this comment.
Looking at this again today, I found user_id and I thought if that should be userid without the underscore. I also noticed that I used role_id in my other PR. Do you have an opinion on this? I'm approving it anyway, as if this would need to change, it would also need to change in the other PR as well.
There was a problem hiding this comment.
Funny you mentioned this because yesterday I was looking into some of our schemas and I noticed the very same inconsistency in naming the columns. I personally like it better with underscores, but I don't have a strong opinion on which format should be used. On the other hand I really think we should review the schemas and go for a single and unified format for the column names.
There was a problem hiding this comment.
I've created #12929 to follow up on this.
Yeah, I followed the approach taken by @vramik for the authorization entities. He mentioned this might be reviewed in the future, so I decided to stick to the same format. If we later on decide we should move away from the collection tables then we can prepare a PR for all entities that use them, including the user entity in this PR. |
Closes #9662