opendoor / pggen

A database first code generator focused on postgres
MIT License
59 stars 9 forks source link

Is it possible to set return type to slice of pointers #185

Closed miniscruff closed 2 years ago

miniscruff commented 2 years ago

I have a query shim that is effectively:

[[query]]
    name = "GetAllX"
    return_type = "[]*X"
    body = '''
    SELECT * FROM xs;
    '''

However when the function is generated it looks like:

GetAllX(ctx context.Context, ([]X, error)

If possible I would prefer it to be:

GetAllX(ctx context.Context, ([]*X, error)

Ideally all the ListX operations would return []*X as well, maybe an option not sure.

Note: Willing to try my hand at contributing if that would help.

ethanpailes commented 2 years ago

Currently, there is no support for this, so you'll need to write a little wrapper to box all the results, but I wouldn't be opposed to adding a configuration flag to ask for boxed results. If you want to start working on that, the place to add your config options would be here. You could can then update the templates used for code generation to box or not box results based on that value.

What is your reason for wanting a slice of boxed values? If it is just to match an interface the easiest thing may be to just have pggen spit out a little routine to iterate the result list and then box all the values, then have it call that routine on the result set right before returning. If you have performance reasons for wanting to have boxed results you may not find this satisfying though.

miniscruff commented 2 years ago

What is your reason for wanting a slice of boxed values? If it is just to match an interface the easiest thing may be to just have pggen spit out a little routine to iterate the result list and then box all the values, then have it call that routine on the result set right before returning. If you have performance reasons for wanting to have boxed results you may not find this satisfying though.

I mostly work using pointers so it is a little bit of preference but also I am using a graph ql generator that expects pointers so it would be really convenient to pass the results of pggen directly to the graphql results. I basically created a bunch of methods to turn []X into []*X here https://github.com/treedefense/projectchip/blob/b18fcad07650fdf80c78a6f50e93784a758ac3c8/backend/resolvers/utils.go#L25 ( will turn it into a generic function but will still need it ). Uses here for example: https://github.com/treedefense/projectchip/blob/b18fcad07650fdf80c78a6f50e93784a758ac3c8/backend/resolvers/matches.go#L22

I did take a look at adding the feature myself but was not sure the best method for doing so. A config option to swap to pointers would be nice.

ethanpailes commented 2 years ago

Gotcha, in that case, just having pggen generate basically exactly those wrapper functions seems like the thing to do. If you want I can give you some more in-depth advice on how to go about implementing it.

miniscruff commented 2 years ago

Yea sure, just some small hints and placement should be fine for me, I have taken a look before so I should be able to handle it.

ethanpailes commented 2 years ago

I would start by adding a new configuration option to QueryConfig, and TableConfig. It should be a boolean that indicates that the generated code should return boxed values. Something like box_results might be good.

Then to actually generate the code, you are going to want to modify the gen/gen_{query,table}.go files for each of those objects. In particular for the gen_query.go file, I would update the multi result branch of the template. You'll need to do this in three places because of the fact that we generate a transaction wrapper with an identical interface to the normal database handle and then have each of them dispatch down to an implementation.

Then after all that setup you can just tweak the line that appends the result to box it before adding to the list.

You'll then need to do this for the <table>List routine as well, though if you want you could leave that for another PR or just not implement it.

A less ambitious version of this would be to just spit out a version of the conversion function that you linked when the flag is set in the query and table templates. Then people can just wrap the result in that function if they want.

miniscruff commented 2 years ago

I am going to try and get the table side to work as well if that works for you.

ethanpailes commented 2 years ago

Yeah that sounds great

ethanpailes commented 2 years ago

@miniscruff I've just cut v0.3.4 so your changes can be picked up without having to resort to depending on a git hash.