Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend the Query Result Mask #371

Closed
vitaly-t opened this issue Jul 19, 2017 · 8 comments
Closed

Extend the Query Result Mask #371

vitaly-t opened this issue Jul 19, 2017 · 8 comments
Milestone

Comments

@vitaly-t
Copy link
Owner

vitaly-t commented Jul 19, 2017

queryResult mask needs to be revised to allow multi-result sets, as supported by node-postgres v7.

New mask multi is to be added.

@vitaly-t vitaly-t added this to the v7.0 milestone Jul 19, 2017
@vitaly-t
Copy link
Owner Author

vitaly-t commented Jul 19, 2017

Updated type in branch driver-7.x:

/**
 * @enum {number}
 * @alias queryResult
 * @readonly
 * @description
 * _Query Result Mask._
 *
 * Binary mask that represents the result expected from queries.
 * It is used by generic {@link Database#query query} method, as well as method {@link Database#func func}.
 *
 * The mask is always the last optional parameter, which defaults to `queryResult.any`.
 *
 * Any combination of flags is supported.
 *
 * The type is available from the library's root: `pgp.queryResult`.
 *
 * @see {@link Database#query Database.query}, {@link Database#func Database.func}
 */
const queryResult = {
    /** Expecting a single result-set, with a single row in it. */
    one: 1,

    /** Expecting a single result-set, with one or more rows in it. */
    many: 2,

    /** Expecting a single result-set, with no rows in it. */
    none: 4,

    /** Expecting multiple result-sets. */
    multi: 8,
    
    /** No expectation as to the data returned. */
    any: 15 //= one | many | none | multi
};

This is a major breaking change, as it completely changes the logic for the query-result mask.

@vitaly-t
Copy link
Owner Author

vitaly-t commented Jul 19, 2017

Masks vs Result

All combinations of masks as can be passed into methods query and result, versus what is expected back:

mask/rows 0 1 > 1 multi-result
none null error error error
one error {} error error
many error [{}] [{}...] error
multi [[{}..]..] [[{}..]..] [[{}..]..] [[{}..]..]
any null {} [{}...] [[{}..]..]
none&one null {} error error
none&many null [{}] [{}...] error
none&multi null [[{}]] [[{}...]] [[{}..]..]
none&one&many null {} [{}...] error
none&one&multi null {} error [[{}..]..]
none&many&multi null [{}] [{}...] [[{}..]..]
one&many error {} [{}...] error
one&multi [[]] {} [[{}...]] [[{}..]..]
one&many&multi [[]] {} [{}...] [[{}..]..]
many&multi [[]] [{}] [{}...] [[{}..]..]

Note that any = none & one & many & multi.

Methods vs Result

We only have methods that use result mask combinations that make practical sense, even though you are free to use any combination via methods query and func.

method/rows 0 1 > 1 multi-result
none null error error error
one error {} error error
oneOrNone null {} error error
many error [{}] [{}...] error
manyOrNone null [{}] [{}...] error
any null {} [{}...] [[{}..]..]
rows [] [{}] [{}...] error
map [] [{}] [{}...] error
each [] [{}] [{}...] error
result Result{} Result{} Result{} error
proc null {} error error
multi [[{}..]..] [[{}..]..] [[{}..]..] [[{}..]..]
multiResult [[Result{}..]..] [[Result{}..]..] [[Result{}..]..] [[Result{}..]..]

The only methods not listed here are query and func that take the result mask as a parameter, and expect the data according to the mask.

@vitaly-t
Copy link
Owner Author

Implemented, pending tests + documentation.

@dmfay
Copy link

dmfay commented Jul 21, 2017

I'm not sure about the multi mask results... for 0, 1, >1 I would expect [[]], [[{}]], [[{}...]] respectively. That looks like it could just be copy & paste getting you though, and it's the only thing that jumps out at me.

@vitaly-t
Copy link
Owner Author

@dmfay The reason I did it this way is to have methods that can access the original data sets either as arrays or as Result. That's how methods multi and multiResult work accordingly.

It is of a compromise. I was thinking - how to get all the basics available and not to over-design this thing. We already have so many methods for the mask, even though it is all logical, I'd love keeping it simple.

Anyhow, any suggestions are welcome! 😉

@dmfay
Copy link

dmfay commented Jul 21, 2017

ah, I got you. It is a bit up for debate whether a constraint is on one or all resultsets when you bring multi into the mix.

Looking a bit closer at the methods, I'm more of the 'if a result can ever iterate, it should always iterate' school of thought. I'd never use manyOrNone when rows exists, for example. With the former I have to make an explicit check for null on every result. With the latter I only have to check for zero length when it matters that there are precisely no records, and if I'm performing an iterative task on the results then I don't have to check anything (no iterations = no problem). Likewise with the new any, but I guess it's probably a safe bet that someone somewhere is going to need it for something I'm not thinking of at the moment.

@vitaly-t
Copy link
Owner Author

vitaly-t commented Jul 22, 2017

I agree, the previous method any was way more useful. But if I were to keep it that way, it would be a problem, because method any is expected to return anything, but it couldn't for multi-result sets.

Also method manyOrNone becomes less useful, for sure.

I just want a set of methods that are logical, consistent with their naming and the logical mask.

This is somewhat challenging. May be I should reduce the methods to a bare minimum... somehow.

@dmfay I am open to suggestions 😉

My biggest concern right now - the default mask for method query. It used to be any, but now it would become quite useless to use any as the default.

@dmfay
Copy link

dmfay commented Jul 23, 2017

As far as reducing the method count: out of the first seven (none through rows in the table) I think the essential set is oneOrNone and rows, with any as the odd one out. The rest of that set don't really add a lot of value imo; sure, none and one save you from having to test for null yourself, but at the expense of having a generic exception instead of being able to write your own more actionable error message if you do get an unexpected result back. many and manyOrNone are similar with respect to the more useful rows as I outlined earlier.

I'm not sure query really has a place anymore. rows does what it would do if multiple resultsets weren't a concern, which is in turn covered by multi, and I don't know that they need to be mixed. Especially given @brianc's note:

The change also doesn't impact prepared statements at all because postgres doesn't allow more than 1 prepared statement to be executed at a time.

But fundamentally, I have a hard time envisioning a use case in which I'm emitting a query and don't know up front whether it comprises one or several individual statements. If I did have that kind of situation, I think it'd be reasonable to just use multi or multiResult and work with the nested output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants