Have the generator use existing structs as inputs/outputs for generated functions

[DavidS-ovm]

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
}

[DavidS-ovm]

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.

[andrewmbenton]

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?

[DavidS-ovm]

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.

[andrewmbenton]

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.

[DavidS-ovm]

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.