Skip to content

Increase column size for keys that refer to entities that can be stor…#11375

Merged
hmlnarik merged 1 commit intokeycloak:mainfrom
sguilhen:11329-fkey-size
Apr 21, 2022
Merged

Increase column size for keys that refer to entities that can be stor…#11375
hmlnarik merged 1 commit intokeycloak:mainfrom
sguilhen:11329-fkey-size

Conversation

@sguilhen
Copy link
Copy Markdown
Contributor

@sguilhen sguilhen commented Apr 18, 2022

…ed in different storages (foreign keys)

Closes #11329

@sguilhen sguilhen added area/storage Indicates an issue that touches storage (change in data layout or data manipulation) team/storage-sig labels Apr 18, 2022
hmlnarik
hmlnarik reviewed Apr 19, 2022
View reviewed changes
Comment thread model/map-jpa/src/main/resources/META-INF/groups/jpa-groups-changelog-1.xml Outdated
Copy link
Copy Markdown
Contributor

@hmlnarik hmlnarik Apr 19, 2022

Choose a reason for hiding this comment

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

This should be changed as well. Perhaps worth checking all VARCHAR(36) fields

Copy link
Copy Markdown
Contributor Author

@sguilhen sguilhen Apr 19, 2022

Choose a reason for hiding this comment

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

I thought about that but in my opinion this one should be left as is. Reason being that the parentId is referring to a parent group, and since all groups are managed by the same provider (in this case JPA), we shouldn't have a case where a parent group is stored in a different storage, and therefore it should be able to rely on the particular format adopted by that storage. In this case, it can rely on the parent being a group whose id is a UUID, no?

Copy link
Copy Markdown
Contributor

@hmlnarik hmlnarik Apr 19, 2022

Choose a reason for hiding this comment

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

This assumption is not correct - all groups are not generally managed by the same provider. Custom group providers (e.g. LDAP) are allowed, and it is possible that a group from one source would be parent to another.

Copy link
Copy Markdown
Contributor Author

@sguilhen sguilhen Apr 20, 2022

Choose a reason for hiding this comment

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

Resized the parent column as well.

@sguilhen sguilhen requested a review from hmlnarik April 20, 2022 17:42
@sguilhen
Copy link
Copy Markdown
Contributor Author

sguilhen commented Apr 21, 2022

Introduced the kc_key data type to bring consistency to all columns that reference entities that might be external to the JPA storage.

Example table (kc_role) generated using the new type on postgres:

keycloak=# \d kc_role
                                                                   Table "public.kc_role"
    Column     |          Type           | Collation | Nullable |                                          Default                                           
---------------+-------------------------+-----------+----------+--------------------------------------------------------------------------------------------
 id            | uuid                    |           | not null | 
 version       | integer                 |           | not null | 0
 metadata      | jsonb                   |           |          | 
 entityversion | integer                 |           |          | generated always as ((metadata ->> 'entityVersion'::text)::integer) stored
 realmid       | character varying(4000) |           |          | generated always as (((metadata ->> 'fRealmId'::text))::character varying(4000)) stored
 clientid      | character varying(4000) |           |          | generated always as (((metadata ->> 'fClientId'::text))::character varying(4000)) stored
 name          | character varying(255)  |           |          | generated always as (((metadata ->> 'fName'::text))::character varying(255)) stored
 description   | character varying(255)  |           |          | generated always as (((metadata ->> 'fDescription'::text))::character varying(255)) stored

Now same table on cockroachdb:

root@:26257/keycloak> \d kc_role
   column_name  |   data_type   | is_nullable | column_default |           generation_expression           |                           indices                            | is_hidden
----------------+---------------+-------------+----------------+-------------------------------------------+--------------------------------------------------------------+------------
  id            | UUID          |    false    | NULL           |                                           | {kc_role_pkey,role_entityVersion,role_realmId_clientid_name} |   false
  version       | INT8          |    false    | 0:::INT8       |                                           | {kc_role_pkey}                                               |   false
  metadata      | JSONB         |    true     | NULL           |                                           | {kc_role_pkey}                                               |   false
  entityversion | INT8          |    true     | NULL           | (metadata->>'entityVersion')::INT8        | {kc_role_pkey,role_entityVersion}                            |   false
  realmid       | VARCHAR(4000) |    true     | NULL           | (metadata->>'fRealmId')::VARCHAR(4000)    | {kc_role_pkey,role_realmId_clientid_name}                    |   false
  clientid      | VARCHAR(4000) |    true     | NULL           | (metadata->>'fClientId')::VARCHAR(4000)   | {kc_role_pkey,role_realmId_clientid_name}                    |   false
  name          | VARCHAR(255)  |    true     | NULL           | (metadata->>'fName')::VARCHAR(255)        | {kc_role_pkey,role_realmId_clientid_name}                    |   false
  description   | VARCHAR(255)  |    true     | NULL           | (metadata->>'fDescription')::VARCHAR(255) | {kc_role_pkey}                                               |   false

Also adjusted the protocol column in kc_client to VARCHAR(255), which aligns it with other text columns.

@sguilhen
Copy link
Copy Markdown
Contributor Author

sguilhen commented Apr 21, 2022

If #11229 is merged first I will need to update the schema to use the new type. Conversely, if this gets merged first I will have to rework #11229 to use the new type.

hmlnarik
hmlnarik approved these changes Apr 21, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Thanks for the PR!

@hmlnarik hmlnarik merged commit f48d468 into keycloak:main Apr 21, 2022
@hmlnarik hmlnarik mentioned this pull request Apr 21, 2022
Merged
@sguilhen sguilhen deleted the 11329-fkey-size branch April 22, 2022 17:42
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

area/storage Indicates an issue that touches storage (change in data layout or data manipulation)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JPA Map Storage: Increase size of all foreign keys in the areas schemas to 4000

2 participants

@sguilhen @hmlnarik