Improve the way library deals with objects in WHERE queries (WIP)

[panta82]

Changes:

• Allow setting NULL-s via object. Query builder will now use "IS" operator instead of the (incorrect) "=".
• Allow undefined properties in objects you provide to WHERE queries. They are now ignored.

My use case for this PR:

    async function getUserByEmailOrNull(email: string, ignoreDeleted?: boolean) {
      return db.sql<TBL.users.SQL, User>`
            SELECT *
            FROM ${'users'}
            WHERE ${{
              email,
              deleted_at: ignoreDeleted ? null : undefined,
            }}`.run()
      );
    }

NOTE: I've updated this PR multiple times, and commit history is a bit messy. If there is interest to merge it, I'll gladly rebase the code.

[javier-garcia-meteologica]

I've also run into problems with objects that have some keys mapped to undefined. Apart from WHERE clauses, this problem occurs also with insert/upsert values, which are handled by vals() and cols().

But I see I potential bug with this patch. This patch removes undefined keys from cols() and vals() using the same criteria. The resulting sql code is consistent only if both functions are called with the same object.

The problem I'm thinking about is with bulk insert or upsert statements. For these statements there is an array of objects, each of which, represents a different item to insert in the DB. cols() is called only once, with the first of those objects. However vals() is called with each one of those objects. So there is a potential mismatch if the first object has an undefined key while the second object doesn't and viceversa.

zapatos/src/db/shortcuts.ts

Lines 241 to 244 in e2cbc70

	completedValues = Array.isArray(values) ? completeKeysWithDefaultValue(values, Default) : [values],
	firstRow = completedValues[0],
	insertColsSQL = cols(firstRow),
	insertValuesSQL = mapWithSeparator(completedValues, sql`, `, v => sql`(${vals(v)})`),

Perhaps this can be solved by patching completeKeysWithDefaultValue to trim undefined properties from objects.

I'm thinking about something like this

/**
 * Removes undefined keys from an object.
 *
 * @param obj The object to trim
 */
export const trimObj = <T extends object>(obj: T): T => {
	const definedEntries = Object.entries(obj).filter(([, value]) => value !== undefined);
	return Object.fromEntries(definedEntries) as T;
};

export const completeKeysWithDefaultValue = <T extends object>(objs: readonly T[], defaultValue: any): T[] => {
	const trimmedObjs: readonly T[] = objs.map(obj => trimObj(obj));
	const unionKeys: T = Object.assign({}, ...trimmedObjs);
	for (const k in unionKeys) unionKeys[k] = defaultValue;
	return trimmedObjs.map(o => ({ ...unionKeys, ...o }));
};

[javier-garcia-meteologica]

This block could be simplified if the columns are filtered here. Then there is no need for firstField

           columnNames = <Column[]>Object.keys(expression.value)
             .filter(key => (<any>expression.value)[key] !== undefined)
             .sort(),

Or with trimObj defined above

           columnNames = <Column[]>Object.keys(trimObj(expression.value)).sort(),

[javier-garcia-meteologica]

This block could be simplified if the columns are filtered here. Then there is no need for firstField

const columnNames = <Column[]>Object.keys(expression)
  .filter(key => (<any>expression)[key] !== undefined)
  .sort();

Or with trimObj defined above

const columnNames = <Column[]>Object.keys(trimObj(expression)).sort();

[panta82]

@javier-garcia-meteologica
You are right about the problem with cols() and vals(). There were other issues that I missed.

The problem is, since this library doesn't have a test suite, I was basically making this blindly, with kind of limited testing (using local NPM install tricks). I've since done the following:

• Changed this PR to WIP, so it doesn't get merged without some more testing
• Create a new branch for testing things out (https://github.com/panta82/zapatos/tree/pantas)
• Published it to npm as a separate package (https://www.npmjs.com/package/@pantas-net/zapatos)
• My current WIP project uses this cloned package, so this is how I am testing out these ideas in practice.

Based on some actual usage experience, I've given up on trying to make this work with cols() and vals(). Since they are called completely separately, there is no good obvious way to ensure that the kind of arguments you give each are properly aligned. I just ended up dealing with non-obvious errors. I have different ideas for cols() and vals(), but I'll test them out and, if they turn out good, I'll submit a separate PR.

The WHERE handling remains, it' working out nicely (with a couple of bug fixes).

I've updated the PR to include the fixed code. I'll keep using my cloned package to test this further, and update this PR (or create a new one) with the improvements I find usable.

[javier-garcia-meteologica]

Related to #46, #97

A month ago I added the patch to ignore undefined keys in the branch javier-garcia-meteologica/zapatos#ready, the commit is named Ignore undefined keys. On top of cols() and vals(), it also patches completeKeysWithDefaultValue().

I haven't run into any bugs, so I guess that was all that had to be changed.

Previously I had been using the workaround suggested in #46, but the problem was that typescript didn't warn me when I used an undefined key.

res = await db.select('cats', {
  ...(breed ?  { breed } : {}),
    age, // Mistake! age is undefined but typescript doesn't warn me
  ...(name ?  { name } : {}),
  otherCharacteristic: true
}).run(pgPool);

Using the patch mentioned above, it becomes easier to use. There is no need to worry about undefined values because they will be treated as if the key didn't exist, which is the same criteria that typescript uses to type inputs.

res = await db.select('cats', {
  breed,
  name,
  age,
  otherCharacteristic: true
}).run(pgPool);

Another solution is the one suggested in #97, which uses exactOptionalPropertyTypes to make a distinction between absent keys and keys with undefined values.

[panta82]

@javier-garcia-meteologica Primary reason why I gave up on cols and vals use case is that there is nothing connecting two calls, so there is no good way to guarantee the key lists will match. In my use case, I have a system where I generate key lists for some of my classes. I did something like this (simplified):

function addUser(user: User) {
  db.sql`INSERT INTO ${'users'} (${db.cols(User.KEYS)}) VALUES (${db.vals(user)})`
}

If some of the keys in user were undefined, it would break during runtime. Sure, everything would work if I used user in both places. But there is nothing in the API to remind me or guide me to do that. It's a hidden footgun.

Another gotcha in your implementation is that, if all keys turn out undefined, this will generate a broken query. You should probably add some guards, see the history of my PR.

[javier-garcia-meteologica]

Let me convince you why your original idea of ignoring undefined keys is as consistent as the current state of zapatos.

[E]verything would work if I used user in both places. But there is nothing in the API to remind me or guide me to do that. It's a hidden footgun

That's an argument against cols() and vals() being two separate functions, regardless of whether undefined keys are ignored. The kind of inconsistencies that you are pointing to, will always exist as long as you don't provide a single function that combines the functionality of both cols() and vals().

Zapatos already expects the same object to be passed to both cols() and vals() in order to be consistent. Given these conditions, applying the same deterministic transformation to the inputs of both functions is consistent too.

Another gotcha in your implementation is that, if all keys turn out undefined, this will generate a broken query

In the case of where object conditions, it compiles just fine, the same as if I used an empty object with the original jawj/zapatos

// With javier-garcia-meteologica/zapatos#ready
zp.sql`SELECT * FROM ${'table_foo'} WHERE ${{ foo: undefined }}`.compile()
// { text: 'SELECT * FROM "table_foo" WHERE TRUE', values: [] }

// With original jawj/zapatos
zp.sql`SELECT * FROM ${'table_foo'} WHERE ${{}}`.compile()
// { text: 'SELECT * FROM "table_foo" WHERE TRUE', values: [] }

In the case of insert columns and values, it can generate invalid insert statements, just as if you passed an empty object to the original jawj/zapatos. But in this case I think it's an acceptable behavior. If someone really wants to create an insert without providing any value, INSERT INTO ${'table_foo'} DEFAULT VALUES; should be used instead.

// With javier-garcia-meteologica/zapatos#ready
const input = { foo: undefined };
zp.sql`INSERT INTO ${'table_foo'} (${zp.cols(input)}) VALUES (${zp.vals(input)})`.compile()
// { text: 'INSERT INTO "table_foo" () VALUES ()', values: [] }

// With original jawj/zapatos
const input = {};
zp.sql`INSERT INTO ${'table_foo'} (${zp.cols(input)}) VALUES (${zp.vals(input)})`.compile()
// { text: 'INSERT INTO "table_foo" () VALUES ()', values: [] }

// With original jawj/zapatos
const input = { foo: undefined };
zp.sql`INSERT INTO ${'table_foo'} (${zp.cols(input)}) VALUES (${zp.vals(input)})`.compile()
// { text: 'INSERT INTO "table_foo" ("foo") VALUES ($1)', values: [undefined] }