-
Notifications
You must be signed in to change notification settings - Fork 43
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
Extend the Profile List database calls to include selectors #3786
Conversation
@blkt 👀 |
8bf5f0a
to
58a29a1
Compare
@blkt thank you for the help on tidying up the models! I did some follow-up cleanup of the code and I think it's ready for review now. I'm not sure if you can give the final ack given that you are the coauthor on the PR now. |
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.
LGTM! 🎉
sqlc.yaml
Outdated
go_type: | ||
import: "github.com/google/uuid" | ||
type: "UUID" |
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.
question: why is this override necessary now while it wasn't before?
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.
it wasn't, sorry.
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.
(longer reply: This was a copy-paste from sqlc docs which I didn't remove by accident)
database/query/profiles.sql
Outdated
WITH helper AS( | ||
SELECT pr.id as profid, | ||
ARRAY_AGG(ROW(ps.id, ps.profile_id, ps.entity, ps.selector, ps.comment)::profile_selector) FILTER (WHERE ps.id IS NOT NULL) AS selectors | ||
FROM profiles pr | ||
LEFT JOIN profile_selectors ps | ||
ON pr.id = ps.profile_id | ||
WHERE pr.project_id = $1 | ||
GROUP BY pr.id | ||
) | ||
SELECT | ||
sqlc.embed(profiles), | ||
sqlc.embed(profiles_with_entity_profiles), | ||
helper.selectors::profile_selector[] AS profiles_with_selectors | ||
FROM profiles | ||
JOIN profiles_with_entity_profiles ON profiles.id = profiles_with_entity_profiles.profid | ||
JOIN helper ON profiles.id = helper.profid | ||
WHERE profiles.project_id = $1; |
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.
suggestion (non-blocking): I think you could get rid of the FILTER
clause by moving the LEFT JOIN
on the outer query. I expect it to work the same given the result is a slice.
WITH helper AS( | |
SELECT pr.id as profid, | |
ARRAY_AGG(ROW(ps.id, ps.profile_id, ps.entity, ps.selector, ps.comment)::profile_selector) FILTER (WHERE ps.id IS NOT NULL) AS selectors | |
FROM profiles pr | |
LEFT JOIN profile_selectors ps | |
ON pr.id = ps.profile_id | |
WHERE pr.project_id = $1 | |
GROUP BY pr.id | |
) | |
SELECT | |
sqlc.embed(profiles), | |
sqlc.embed(profiles_with_entity_profiles), | |
helper.selectors::profile_selector[] AS profiles_with_selectors | |
FROM profiles | |
JOIN profiles_with_entity_profiles ON profiles.id = profiles_with_entity_profiles.profid | |
JOIN helper ON profiles.id = helper.profid | |
WHERE profiles.project_id = $1; | |
WITH helper AS( | |
SELECT pr.id as profid, | |
ARRAY_AGG(ROW(ps.id, ps.profile_id, ps.entity, ps.selector, ps.comment)::profile_selector) AS selectors | |
FROM profiles pr | |
JOIN profile_selectors ps | |
ON pr.id = ps.profile_id | |
WHERE pr.project_id = $1 | |
GROUP BY pr.id | |
) | |
SELECT | |
sqlc.embed(profiles), | |
sqlc.embed(profiles_with_entity_profiles), | |
helper.selectors::profile_selector[] AS profiles_with_selectors | |
FROM profiles | |
JOIN profiles_with_entity_profiles ON profiles.id = profiles_with_entity_profiles.profid | |
LEFT JOIN helper ON profiles.id = helper.profid | |
WHERE profiles.project_id = $1; |
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.
suggestion taken!
database/query/profiles.sql
Outdated
WITH helper AS( | ||
SELECT pr.id as profid, | ||
ARRAY_AGG(ROW(ps.id, ps.profile_id, ps.entity, ps.selector, ps.comment)::profile_selector) FILTER (WHERE ps.id IS NOT NULL) AS selectors | ||
FROM profiles pr | ||
LEFT JOIN profile_selectors ps | ||
ON pr.id = ps.profile_id | ||
WHERE pr.project_id = $1 | ||
GROUP BY pr.id | ||
) | ||
SELECT sqlc.embed(profiles), | ||
sqlc.embed(profiles_with_entity_profiles), | ||
helper.selectors::profile_selector[] AS profiles_with_selectors | ||
FROM profiles | ||
JOIN profiles_with_entity_profiles ON profiles.id = profiles_with_entity_profiles.profid | ||
JOIN helper ON profiles.id = helper.profid |
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.
suggestion (non-blocking): I think you could get rid of the FILTER
clause by moving the LEFT JOIN
on the outer query. I expect it to work the same given the result is a slice.
WITH helper AS( | |
SELECT pr.id as profid, | |
ARRAY_AGG(ROW(ps.id, ps.profile_id, ps.entity, ps.selector, ps.comment)::profile_selector) FILTER (WHERE ps.id IS NOT NULL) AS selectors | |
FROM profiles pr | |
LEFT JOIN profile_selectors ps | |
ON pr.id = ps.profile_id | |
WHERE pr.project_id = $1 | |
GROUP BY pr.id | |
) | |
SELECT sqlc.embed(profiles), | |
sqlc.embed(profiles_with_entity_profiles), | |
helper.selectors::profile_selector[] AS profiles_with_selectors | |
FROM profiles | |
JOIN profiles_with_entity_profiles ON profiles.id = profiles_with_entity_profiles.profid | |
JOIN helper ON profiles.id = helper.profid | |
WITH helper AS( | |
SELECT pr.id as profid, | |
ARRAY_AGG(ROW(ps.id, ps.profile_id, ps.entity, ps.selector, ps.comment)::profile_selector) AS selectors | |
FROM profiles pr | |
JOIN profile_selectors ps | |
ON pr.id = ps.profile_id | |
WHERE pr.project_id = $1 | |
GROUP BY pr.id | |
) | |
SELECT sqlc.embed(profiles), | |
sqlc.embed(profiles_with_entity_profiles), | |
helper.selectors::profile_selector[] AS profiles_with_selectors | |
FROM profiles | |
JOIN profiles_with_entity_profiles ON profiles.id = profiles_with_entity_profiles.profid | |
LEFT JOIN helper ON profiles.id = helper.profid |
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.
suggestion taken!
Since the profile selectors need to be included when listing profiles, we need to extend the two profile list calls to list selectors as well. Because the selectors are stored in a different table, but we still need to retrieve them in the same Go struct defined in models, we define a new SQL type in a migration that represents the selectors and tell sqlc to use it in sqlc.yaml. This new Go struct needs a Scan method as well to convert raw SQL rows into structs as well. Fixes: mindersec#3721 Co-authored-by: Michelangelo Mori <mmori@stacklok.com>
mindersec#3786 (comment) Co-authored-by: Michelangelo Mori <mmori@stacklok.com>
Summary
Extend the Profile List database calls to include selectors
Since the profile selectors need to be included when listing profiles, we need to extend the two profile list calls to list selectors as well.
Because the selectors are stored in a different table, but we still need to retrieve them in the same Go struct defined in models, we define a new SQL type in a migration that represents the selectors and tell sqlc to use it in sqlc.yaml.
This new Go struct needs a Scan method as well to convert raw SQL rows into structs as well.
Co-authored-by: Michelangelo Mori mmori@stacklok.com
Fixes: #3721
Change Type
Testing
There are unit tests and I tested the code as part of a larger branch, too.
Review Checklist: