Adds "SelectFoo" functions for each DB type.#2259
Conversation
In #2178 we moved to explicit `SELECT` statements using a set of `const` fields for each type to support db migrations and forward compatibility. This commit removes the temptation to interpolate queries by providing convenience `Select` functions for each type allowing the caller to provide the WHERE clause and arguments. Resolves #2214.
jsha
left a comment
jsha
left a comment
There was a problem hiding this comment.
In general, looks great. I think this is a big improvement and will hopefully wipe out a whole category of potential bugs.
sa/model.go
Outdated
| // feature flag is enabled and includes "notAfter" and "isExpired" fields | ||
| CertificateStatusFieldsv2 string = CertificateStatusFields + ", notAfter, isExpired" | ||
| ) | ||
| func SelectRegistration(so dbSelectOne, q string, args ...interface{}) (*regModelv1, error) { |
There was a problem hiding this comment.
To satisfy go vet (which we don't run automatically), each of these exported functions should have a comment explaining what they do and why.
There was a problem hiding this comment.
@jsha are you sure that go vet requires this? I'm seeing no output from go vet sa/model.go without the comments in place.
Happy to add them anyway!
There was a problem hiding this comment.
Sorry, I meant golint. I always confuse those two.
There was a problem hiding this comment.
Aha, I don't think I knew golint even existed! I'll have to start using that more.
sa/model.go
Outdated
| pendingAuthzFields string = "id, identifier, registrationID, status, expires, combinations, LockCol" | ||
| authzFields string = "id, identifier, registrationID, status, expires, combinations" | ||
| sctFields string = "id, sctVersion, logID, timestamp, extensions, signature, certificateSerial, LockCol" | ||
| type dbSelectOne func(interface{}, string, ...interface{}) error |
There was a problem hiding this comment.
I think it might be more natural to define a type dbSelector interface { Select(...) }. That way we can just pass a dbMap into this. Otherwise, I look at a call like sa.SelectCertificates(dbMap.Select, ...) and wonder: What else could I pass instead of .Select? Could I pass .Exec? What effect would it have?
There was a problem hiding this comment.
BTW, replying to the your commit message:
Per feedback from @jsha this commit replaces these function types with
interfaces defining the required functions. This should be safer
overall.
My main interest here was readability more than safety, but I guess this does help prevent accidents like that.
There was a problem hiding this comment.
Readability is probably the better argument overall since I don't think the return types would line up and it would be caught by the go compiler.
| // CertificateFields and CertificateStatusFields are also used by cert-checker and ocsp-updater | ||
| CertificateFields string = "registrationID, serial, digest, der, issued, expires" | ||
| CertificateStatusFields string = "serial, subscriberApproved, status, ocspLastUpdated, revokedDate, revokedReason, lastExpirationNagSent, ocspResponse, LockCol" | ||
| const regFields = "id, jwk, jwk_sha256, contact, agreement, initialIP, createdAt, LockCol" |
There was a problem hiding this comment.
I think these consts are only used in one place and can be put directly into the query string, right?
There was a problem hiding this comment.
The unexported const fields I left are used by more than one select method. If I move them into the functions there will be duplication and more than one place to update if we change the field list.
E.g. authzFields is used by both SelectAuthz, and SelectAuthzs.
Prior to this commit the `SelectFoo()` functions accepted a function type as the first argument, and invoked it on behalf of the caller with the constructed query. This might allow passing the wrong function to `SelectFoo` (e.g. `Exec` instead of `SelectOne`). Per feedback from @jsha this commit replaces these function types with interfaces defining the required functions. This should be safer overall.
|
@jsha should be ready for re-review once CI is 💚 |
rolandshoemaker
left a comment
rolandshoemaker
left a comment
There was a problem hiding this comment.
One style nit, otherwise LGTM.
sa/model.go
Outdated
| const regFieldsv2 = regFields + ", status" | ||
|
|
||
| // Select all fields of one registration model | ||
| func SelectRegistration(s dbOneSelector, q string, args ...interface{}) (*regModelv1, error) { |
There was a problem hiding this comment.
It looks like only SelectCertificate, SelectCertificates, SelectCertificateStatuses, and SelectCertificateStatusesv2 need to be exported, is there a reason you've exported all of the Select* methods?
There was a problem hiding this comment.
No reason. I'll switch the others unexported until there's a reason to do otherwise. Good catch.
This commit unexports all of the `SelectFoo` functions except `SelectCertificate`, `SelectCertificates`, `SelectCertificateStatuses`, and `SelectCertificateStatusesv2`. The comments for the `SelectFoo` functions were changed to start with the function name so that `golint` was satisfied.
3 participants
In #2178 we moved to explicit
SELECTstatements using a set ofconstfields for each type to support db migrations and forward compatibility.This commit removes the temptation to interpolate queries by providing convenience
Selectfunctions for each type allowing the caller to provide the WHERE clause and arguments.Resolves #2214.