Add migration to fix signature keys inconsistency#11113
Conversation
mshaposhnik
changed the title
benoitf
added
status/code-review
sleshchenko
left a comment
sleshchenko
left a comment
There was a problem hiding this comment.
Take a look my inlined comments
|
|
||
|
|
||
| -- remove key pair records which linked to non-existing keys | ||
| DELETE FROM che_sign_key_pair kp |
There was a problem hiding this comment.
Can't it be simplified like the following?
DELETE FROM che_sign_key_pair kp
WHERE NOT EXISTS (
SELECT id
FROM che_sign_key k
WHERE k.id = kp.private_key OR k.id = kp.public_key
)
There was a problem hiding this comment.
Nope. That query will work only if both private_ and public_ key not exists.
There was a problem hiding this comment.
I understand. Thanks.
There is a possible case when key par A exists, and it has public key but not private one. Key pair is going to be removed. But private key won't be removed. As result adding foreign key will fail. Am I right? If yes, please consider adding cleaning private and public keys which are referencing non-existing key pair.
There was a problem hiding this comment.
I don't see reason why foreign key shoud fail? After that query executed there will be correct data with both keys exists. But i agree that if one key is exists and other is not, the existing key can be left in DB forewer. Need to think is it can became a problem.
There was a problem hiding this comment.
I don't see reason why foreign key shoud fail?
You're right. I missed that foreign keys are from pair to keys but not wise versa.
There was a problem hiding this comment.
Added extra unused keys cleanup query
There was a problem hiding this comment.
| CREATE UNIQUE INDEX index_sign_public_private_key_id ON che_sign_key_pair (public_key, private_key); | ||
|
|
||
| -- add keys table constraints | ||
| ALTER TABLE che_sign_key_pair ADD CONSTRAINT fk_sign_public_key_id FOREIGN KEY (public_key) REFERENCES che_sign_key (id); |
There was a problem hiding this comment.
Make sure that there are indexes for fk_sign_public_key_id and fk_sign_private_key_id
There was a problem hiding this comment.
Didn't get the idea. Explain, pls.
There was a problem hiding this comment.
Please make sure that
--indexes
CREATE INDEX index_sign_public_key_id ON che_sign_key_pair (public_key);
CREATE INDEX index_sign_private_key_id ON che_sign_key_pair (private_key);
There was a problem hiding this comment.
What is the reason of adding that? We never do seatch on that fields, and i'ts seems not needed for the FK constraint.
There was a problem hiding this comment.
We never do seatch on that fields
I would say that we don't do such searches explicitly. But I think JPA uses such search to fetch keys for a requested pair.
We have to add indexes on foreign keys, and we do it like here https://github.com/eclipse/che/blob/6c53555fded7621014501ab44d2c4bbca869b4db/wsmaster/che-core-sql-schema/src/main/resources/che-schema/5.8.0/1__add_foreigh_key_indexes.sql#L18-L17
Correct me If I'm wrong.
There was a problem hiding this comment.
Ok, i didn't remember the reason, but seems it's needed for JPA. Will add separate indexes.
| WHERE ( | ||
| SELECT count(*) FROM che_sign_key_pair kp2 | ||
| WHERE kp1.private_key = kp2.private_key | ||
| OR kp1.private_key = kp2.public_key |
There was a problem hiding this comment.
I guess you wanted to write kp1.public_key = kp2.public_key here
|
ci-test |
|
ci-test build report: |
|
ci-test |
|
ci-test build report: |
benoitf
removed
the
status/code-review
5 participants
What does this PR do?
Adds migration that makes sure there is all workspace public & private pairs exists, check that all workspaces have unique key pairs, and adds appropriate constraints to the DB schema.
What issues does this PR fix or reference?
#11087
Release Notes
N/A
Docs PR
N/A