Add missing indices to resolve #1067#1069
Conversation
|
I wasn't sure if you had a naming convention for indices. Please let me know if you would like me to update the names. To test the migrations, I applied them on both postgres and mysql using the supplied docker-compose setup. |
oauth2/fosite_store_sql.go
Outdated
| subject varchar(255) NOT NULL | ||
| )`, | ||
| "4": fmt.Sprintf("ALTER TABLE hydra_oauth2_%s ADD active BOOL NOT NULL DEFAULT TRUE", table), | ||
| "5": fmt.Sprintf("CREATE INDEX hydra_oauth2_%s_request_id_idx ON hydra_oauth2_%s (request_id)", table, table), |
There was a problem hiding this comment.
I think it would make sense to have an unique index. They key should really be unique and it will help with query speed.
There was a problem hiding this comment.
lmao 2 years later and I found that this new unique index actually breaks OpenID Connect compliance...welp 🤷
| }, | ||
| }, | ||
| { | ||
| Id: "6", |
There was a problem hiding this comment.
@aeneasr I had to keep them separate because I only wanted to create an index on the requested_at column on the hydra_oauth2_access table.
I guess I could add a semicolon and add another statement to migration 5?
There was a problem hiding this comment.
Oh yeah, I see, this is because of the function design. Let's keep it the way you have it
|
Please also add a test. Since you didn't change the schema it's probably enough to re-enter the previous data again but with a new ID. |
|
@aeneasr maybe I am being stupid 😅, but I am not sure how adding the test to https://github.com/ory/hydra/blob/master/client/manager_0_sql_migrations_test.go will work? That test seems to be specifically around migrations related to the client manager? |
|
Thanks for the feedback! Will address in the morning 🙌 |
aaslamin
force-pushed
the
1067-add-missing-indices
branch
from
f9cb97a to
2718474
Compare
• `request_id` column in the hydra_oauth2_access & hydra_oauth2_refresh tables • `requested_at` column in the hydra_oauth2_access table Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
aaslamin
force-pushed
the
1067-add-missing-indices
branch
from
2718474 to
7ff3e53
Compare
Update:• Changed index to a unique index on the |
… on the request_id column Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
Update• Fixed broken test - there was a test that was inserting multiple records with the same |
|
Nice! I'll review that tomorrow! |
…the `request_id` column in the hydra_oauth2_access and hydra_oauth2_refresh tables. Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
aaslamin
force-pushed
the
1067-add-missing-indices
branch
from
d505fc2 to
2399d83
Compare
Update:• Added new test coverage to cover the unique constraints placed on the This PR is ready for potentially a final review @aeneasr 🤝 |
|
Thank you! |
Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
See #1069 Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
2 participants
Resolves #1067 by adding indices to:
•
request_idcolumn in the hydra_oauth2_access & hydra_oauth2_refresh tables•
requested_atcolumn in the hydra_oauth2_access tableReview: @aeneasr