Use encoding/csv to when parsing selector record #4515
Conversation
internal/db/profile_selector_scan.go
Outdated
| cr := csv.NewReader(strings.NewReader(str)) | ||
| parts, err := cr.Read() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to scan SelectorInfo: %v", err) | ||
| } |
There was a problem hiding this comment.
Do we need to update the reverse (store) code as well?
Do we have some unit tests to cover the round-trip as well as the serialized format?
There was a problem hiding this comment.
there is no serialization, really. The selectors are used as-is, but they are retrieved by aggregating the selectors table with the profiles table in https://github.com/stacklok/minder/blob/6f89a1de73ec7e42b1730951b645bbfb6c1387a0/database/query/profiles.sql#L68 which uses a SQL type defined at https://github.com/stacklok/minder/blob/6f89a1de73ec7e42b1730951b645bbfb6c1387a0/database/migrations/000072_profile_selectors_type.up.sql#L17 which we force through sqlc (https://github.com/stacklok/minder/blob/6f89a1de73ec7e42b1730951b645bbfb6c1387a0/sqlc.yaml#L31)
The code I modified is a Scan() to convert the Go type for the selectors defined in models.go to the sql type.
There was a problem hiding this comment.
Aha! I'm slightly nervous about whether the Postgres ARRAY_AGG produces csv output, but this makes sense for now.
There was a problem hiding this comment.
turns out that there is pq.Array which sounds like something I want to use. I'll take a look into using it - thanks for bringing up the concern!
There was a problem hiding this comment.
OK, I confused myself. pq.Array would be useful for parsing the whole array, but here we are parsing the ROW, not the array. I would still go with the csv approach, but we might want to consider using LazyQuotes. I'll do some more tests before we merge this PR.
There was a problem hiding this comment.
Yeah, I think using LazyQuotes is what we want to be more permissive about quoting esp with comments - the CEL content itself should be validated before storing in DB by calling CEL's validation
`encoding/csv` allows us to handle selectors with commas in them which our naive parser embarassingly didn't such as: ``` repository.properties['github/primary_language'] in ['TypeScript', 'Go' ``` Fixes: mindersec#4514
jhrozek
force-pushed
the
selector_csv
branch
from
d6536ac to
161cf63
Compare
Summary
encoding/csvallows us to handle selectors with commas in them whichour naive parser embarassingly didn't such as:
Fixes: #4514
Change Type
Testing
unit test plus using that selector
Review Checklist: