Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unique constraints for images #3254

Merged
merged 11 commits into from
Jul 6, 2023
Merged

Conversation

zephraph
Copy link
Contributor

@zephraph zephraph commented May 30, 2023

Fixes #2895

As @davepacheco pointed out in #2895, CRDB doesn't treat null as unique which ultimately means duplicate entries can be present. To prevent that I've broken down the constraint down into two indices to ensure the unique constraint is enforced.

@david-crespo
Copy link
Contributor

Great news if that's all it takes.

@zephraph zephraph marked this pull request as ready for review May 30, 2023 20:31
Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

Looks great assuming you confirmed the test fails without the index change.

@zephraph
Copy link
Contributor Author

Yeah, the only issue is that when the constraint check fails it's a 500 which is not what we want. Need to add some logic around catching that.

@davepacheco
Copy link
Collaborator

That's usually a case where something wants to be using ErrorHandler::Conflict instead of some other variant. (Sorry if this is unhelpful -- I haven't looked at the code here.)

@zephraph
Copy link
Contributor Author

Yeah, that was my conclusion. Unfortunately there's a slight hiccup in that Conflict is mostly used in cases where we have the name in question. In this particular case we don't actually have the name on hand. We're promoting an image by ID and the uniqueness constraint is failing on name. That may mean we need to do a secondary lookup for the name of the resource before doing the insert?

@zephraph
Copy link
Contributor Author

As an update on this

I paid with @davepacheco and he helped me with some query execution analysis. The query to list a project's images combined with silo images (which is used for situations like instance create) ends up doing a full table scan. We were able to avoid the full table scan by breaking it down into two select queries with a UNION clause combining the results. Diesel exposes capacities for performing a union but I've been struggling to figure out how to slot that in with our current paginated approach.

@zephraph
Copy link
Contributor Author

zephraph commented Jul 6, 2023

I've decided to sidestep the whole issue with the query and just remove the include_silo_images flag when listing project images. It reduces the option space for the UI, but we can always look to add it again later if it's a customer requested feature.

@david-crespo
Copy link
Contributor

So how do you get a list of silo images?

@zephraph
Copy link
Contributor Author

zephraph commented Jul 6, 2023

The images list endpoint already had to modes. /v1/images gives you the silo images. /v1/images?project=:project gives you a project's images. The flag that I removed was /v1/images?project=:project&include_silo_images which allowed for listing both a single projects images alongside all the silo images. The use case there was so that we could have a single image selection interface.

@david-crespo
Copy link
Contributor

Oh right. I forgot about selecting with no param. So the console will just make two requests. No problem.

@zephraph zephraph enabled auto-merge (squash) July 6, 2023 21:25
@zephraph zephraph merged commit c2de480 into main Jul 6, 2023
@zephraph zephraph deleted the fix-image-unique-constraint branch July 6, 2023 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image promote does not check for duplicate names
3 participants