-
Notifications
You must be signed in to change notification settings - Fork 925
Have the generator use existing structs as inputs/outputs for generated functions #2625
-
in the continued quest to crisp up my APIs, I was wondering what the thoughts on here are regarding specifying custom structs for inputs/outputs of the generated query functions. Specifically in my case I have existing structs generated from the protobuf definition of my API that in some cases would be very convenient to directly (pre-)fill from the database layer:
// current implementation func (h *ApiKeyServiceHandler) GetAPIKey(ctx context.Context, req *connect.Request[sdp.GetAPIKeyRequest]) (*connect.Response[sdp.GetAPIKeyResponse], error) { // skipped authorization code databaseKey, err := h.Queries.GetApiKeyByID(ctx, models.GetApiKeyByIDParams{ ExternalID: id, AccountName: claims.AccountName, }) // skipped error handling resp := connect.NewResponse(&sdp.GetAPIKeyResponse{ Key: databaseKey.ToSDP(), }) return resp, nil } // ApiKey is the generated model from sqlc func (k *ApiKey) ToSDP() *sdp.APIKey { result := &sdp.APIKey{ Metadata: &sdp.APIKeyMetadata{ Uuid: k.ExternalID[:], Created: &k.CreatedAt, LastUsed: k.LastUsed, Key: k.Key, Scopes: k.Scopes, Status: sdp.KeyStatus(k.Status), }, Properties: &sdp.APIKeyProperties{ Name: k.Name, }, } if k.Error.Valid { result.Metadata.Error = k.Error.String } return result }
-- name: GetApiKeyByID :one SELECT * FROM api_keys WHERE account_name = @account_name and external_id = @external_id;
instead it would be really nice to be able to say
-- name: GetApiKeyByID :one :go:result:*sdp.APIKey SELECT * FROM api_keys WHERE account_name = @account_name and external_id = @external_id;
and boil the handler down to
// current implementation func (h *ApiKeyServiceHandler) GetAPIKey(ctx context.Context, req *connect.Request[sdp.GetAPIKeyRequest]) (*connect.Response[sdp.GetAPIKeyResponse], error) { // skipped authorization code databaseKey, err := h.Queries.GetApiKeyByID(ctx, models.GetApiKeyByIDParams{ ExternalID: id, AccountName: claims.AccountName, }) // skipped error handling resp := connect.NewResponse(&sdp.GetAPIKeyResponse{ Key: databaseKey // pass object through directly }) return resp, nil } // no ApiKey model generated, no manual copying required
Of course all the property names and mappings would have to align and all properties would need to have compatible enough types, which could be checked by generating intermediate temp variables in the generated functions like this:
func (q *Queries) GetApiKeyByID(ctx context.Context, arg GetApiKeyByIDParams) (*sdp.APIKey, error) { row := q.db.QueryRowContext(ctx, getApiKeyByID, arg.AccountName, arg.ExternalID) var externalId uuid.Uuid var accountName string // etc err := row.Scan( &externalID, &accountName, // etc ) var i sdp.APIKey i.ExternalID = externalId i.AccountName = accountName // etc return &i, err }
Beta Was this translation helpful? Give feedback.
All reactions
Replies: 1 comment 4 replies
-
I just realized that my primary use-case would most likely not work for reasons outside of sqlc: using protobuf structs would fail hard in practice because protobuf in general doesn't allow shallow copies (see e.g. https://stackoverflow.com/a/67796278/4918 for an explanation) and would break horribly for not-nullable fields or add complexity dealing with nil-able pointers to those structs even when there would be a guaranteed result.
Beta Was this translation helpful? Give feedback.
All reactions
-
Why wouldn't this work in a case where a database field is not-nullable? It seems like the opposite would be true, i.e. if you have a nullable field then you'd typically get back e.g. sql.NullString
and it's not obvious how to convert that into a proto message field with type string
. Or maybe I'm misunderstanding completely?
Beta Was this translation helpful? Give feedback.
All reactions
-
ach, apologies. I left out the most important bit: timestamppb
(the protobuf timestamp implementation) are in this class of non-copyable structs. Thus, having a non-pointer (i.e. what I would want for a mandatory timestamp, like created_at
) timestamppb
as field in a struct leads to the warnings (and hence the bugs) described in what I linked above.
Beta Was this translation helpful? Give feedback.
All reactions
-
I think I am following now, except for the "using protobuf structs would fail hard" part (emphasis added obv). While it may not be ideal to have a pointer type for e.g. created_at
it seems not too big an issue if you get the benefit of reusing your proto structs. But I still may not be understanding the nuance of this, so please correct me if necessary.
Beta Was this translation helpful? Give feedback.
All reactions
-
You're right, I'm being overly negative here. The structs generated by protoc will always have pointers for this, so as long as row.Scan()
can deal with that it's gonna work.
Beta Was this translation helpful? Give feedback.