doc: add dynamic filtering examples

[JakobJingleheimer]

Added examples for filtering and combining conditions in SQL queries.

Resolves #1126

[karlhorky]

1. I guess the condition is not required to illustrate this method of accessing a property in JSONB, and distracts from the example
2. Maybe you can try inlining the SQL in the interpolation, rather than creating another variable - this would probably also make it easier to follow

[JakobJingleheimer]

I kept it separated because that is likely how it would be written IRL, and because this library is so finicky with string templates and what utils are needed when (which is literally the problem necessitating this example).

Perhaps a better way to write it would be

const field = someCondition
  ? 'notifications'
  : '2FA'

await sql`
  select * from users
    where ${sql`${sql(field)}->>'email' = TRUE`};
`

[karlhorky]

What my point is: the ternary is not needed for dynamic field access and introduces some business logic in the example which is only a distraction

But maybe keeping 2 variables would be illustrative, to show that both can be dynamic:

const field = 'notifications';
const subField = 'email';

await sql`
  select * from users
    where ${sql`${sql(field)}->>${sql(subField)} = TRUE`};
`

[JakobJingleheimer]

I think that sql(subField) is actually wrong (I think it needs to be sql`subField`), but otherwise, sure, that looks fine to me :)

[karlhorky]

Yeah, try it out to confirm that it works - I thought that would be the syntax but maybe I'm wrong.

[karlhorky]

Improve the clarity of the reduce mechanics

Suggested change

	where ${conditions.reduce((clause, condition) => sql`${clause} AND ${condition}`)};
	where ${conditions.reduce((accumulatedCondition, condition) => sql`${accumulatedCondition} AND ${condition}`)};

[JakobJingleheimer]

I think this just makes it wordier

[karlhorky]

Long variable names are good to improve comprehension and expressiveness - see as an reference some of the variables that Facebook/Meta uses in various React codebases

But if you have a shorter (non-abbreviated) alternative to clause (which doesn't seem expressive enough to me), open to suggestions. I looked at 10 alternatives to clause, and this is the one that looked the best.

When I read clause:

• I didn't immediately understand how this was being used in your reduce example
• it also made me think "why is it now called clause, without any reference to condition anymore? everything else was called condition? what is the difference between a clause and a condition?"

[JakobJingleheimer]

I used clause because that's its official name in SQL. A quick search for "sql clause" gives 10 near-identical and succinct definitions. From DuckDuckGo's search assistant:

A SQL clause is a part of a SQL statement that specifies a particular condition or action, such as filtering data with the WHERE clause or sorting results with the ORDER BY clause.

That's exactly what this is, and it even cites the specific filter operator used in this example.

[karlhorky]

I think introducing the new terminology makes it worse. My opinion would be to change it, for the reasons I already expressed above

[karlhorky]

We don't need to exhaustively list the things that don't work

Suggested change

	To dynamically combine multiple conditions, use [Array `reduce`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reduce); [Array `join`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/join) does not work because its output is a string (an interpolated value) instead of a `Fragment`:
	To dynamically combine multiple conditions, use [`Array.prototype.reduce()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reduce):

[JakobJingleheimer]

I disagree since this is what almost everyone will try, and it mysteriously fails despite looking perfectly fine in the output from postgres.js's debug. That nonsense cost me 1.5 days of wtf, and you yourself did not realise it was the problem 😜

[karlhorky]

Unfortunately, others may try something else - we won't be able to cover all possible incorrect cases / methods, and it blows up the size of the docs.

[JakobJingleheimer]

They may well try something else. This will catch the vast majority and also, most importantly, explains why join doesn't work and reduce does.

[karlhorky]

I don't think either of us can decide what the majority would be - I disagree with having these notes in the docs

[porsager]

Thanks a lot for the improvements. I think an addition of something like this to the docs is a smell because the real fix is to add a proper solution for joining fragments. My wish for the join method is for it to look like this:

sql` and `.join(...)

The current pr to add join is close, but tests aren't passing and the solution is too verbose sql.join(sql` and `, fragments). I'll follow up there too to get it moving

[JakobJingleheimer]

Mm, I agree. I was thinking of this as an interim solution (and caveat) since sql.join() isn't available yet—I would much prefer sql.join() 🙂

That syntax though initially looks like Yoda 🤪 I think something like this would be much more intuitive:

await sql`
  SELECT * FROM "Example"
    WHERE ${sql.join(
      [
        sql`foo = ${foo}`,
        sql`bar = ${bar}`,
      ],
      'OR', // default to AND?
    )};
`

[porsager]

Haha :P Yeah I can see your point. The trouble is that we need to use a fragment for the joiner if we want to keep the safety. That makes yours and the PRs example into sql.join(..., sql` or `).

Your sample also adds another question. Do we just willy nilly add whitespace on each side of the join string? That would probably help after few, but also be annoying for someone where the whitespace breaks some intended functionality

[porsager]

I have also been thinking of using between instead to prevent the ambiguity around js join and sql join. That would also reverse the yodaness you mention.. But I agree that join is the goto keyword someone would think of so we'd loose that..

sql` and `.between(...)

[JakobJingleheimer]

That makes yours and the PRs example into sql.join(..., sql` or `).

Your sample also adds another question. Do we just willy nilly add whitespace on each side of the join string?

I guarantee you requiring users to include whitespace around the joiner arg will trip up almost, if not every, user 😬

annoying for someone where the whitespace breaks some intended functionality

I'm ~95% sure extra whitespace does not create a problem—AFAIK, SQL collapses it under the hood. You can even do something like "TableName" . "FieldName".

I have also been thinking of using between instead to prevent the ambiguity around js join and sql join.

Six of one, half a dozen of the other: SQL also has BETWEEN 😅 So I think on top of still being ambiguous, it's not what anyone would expect. I think users will be working with either a plain key-value object/dictionary or an array, and build their conditions from that.

I'm currently migrating from pg to this library, and the first thing I tried when dynamically building a WHERE clause was sql() (since it handles an object for INSERT). I hoped it would work for WHERE or WHERE … IN. I think it's not much of a mental stretch, so if sql() could also handle this, that would perhaps be the most straightforward for users:

await sql`
  UPDATE "Example"
    SET ${sql({ foo: 1 })}
    WHERE ${sql({ bar: 2 })};
`

sql() does not handle many cases for filter fields though (ex JSON(B) selectors like "SomeJsonField"->>'someSubField'), so without enhancing sql() to handle those, a dictionary can't be what's passed to WHERE's sql() since keys of an object can't themselves be objects, which is what sql`` produces: (invalid js) { [sql`"SomeJsonField"->>'someSubField'`]: 'qux' }.

Buuut, on the flip-side, sql is already quite overloaded, so maybe adding more onto the pile is actually not a good idea 🤔 And also sql() already accepts a second/...rest argument to pluck fields from the first, so it would probably be a huge headache to implement (if even possible—a user could do something insane like name a column AND, so the imp couldn't look for specific needles to determine what it's being passed).

P.S. Whatever the ultimate solution, please don't make it a promise—that makes debugging such a headache.

[karlhorky]

Seems pretty good, thanks for the docs addition!

I added some small notes above.

[JakobJingleheimer]

I disagree since this is what almost everyone will try, and it mysteriously fails despite looking perfectly fine in the output from postgres.js's debug. That nonsense cost me 1.5 days of wtf, and you yourself did not realise it was the problem 😜

[JakobJingleheimer]

I think this just makes it wordier

[JakobJingleheimer]

I kept it separated because that is likely how it would be written IRL, and because this library is so finicky with string templates and what utils are needed when (which is literally the problem necessitating this example).

Perhaps a better way to write it would be

const field = someCondition
  ? 'notifications'
  : '2FA'

await sql`
  select * from users
    where ${sql`${sql(field)}->>'email' = TRUE`};
`