feat(utils): support for NULLS FIRST/LAST in orderByAscDesc

[Deckstar]

…scDesc;

Description

Currently orderByAscDesc works as expected, except for one detail: when ordering by descending, null values show up first. This leads to unwanted results.

In my case, for example, I needed to create a plugin that ordered profiles by "top rated" — meaning the average of their reviews:

const ProfilesTopRated = makeAddPgTableOrderByPlugin(
  'app_public',
  'profiles',
  (build) => {
    const { pgSql: sql } = build;
    const sqlIdentifier = sql.identifier(Symbol('review'));

    const customOrderBy = orderByAscDesc(
      'TOP_RATED',
      (helpers) => {
        const { queryBuilder } = helpers;

        const orderByFrag = sql.fragment`(
          select avg(${sqlIdentifier}.rating)
          from app_public.profile_reviews as ${sqlIdentifier}
          where ${sqlIdentifier}.reviewee_profile_id = ${queryBuilder.getTableAlias()}.id
        )`;

        return orderByFrag;
      },
    );

    return customOrderBy;
  }
);

However, new profiles without any reviews were showing up first (which isn't exactly what comes to mind when thinking "top-rated" 😉 ).

This new feature offers a very quick and easy fix:

const customOrderBy = orderByAscDesc(
      'TOP_RATED',
      (helpers) => {
        const { queryBuilder } = helpers;

        const orderByFrag = sql.fragment`(
          select avg(${sqlIdentifier}.rating)
          from app_public.profile_reviews as ${sqlIdentifier}
          where ${sqlIdentifier}.reviewee_profile_id = ${queryBuilder.getTableAlias()}.id
        )`;

        return orderByFrag;
      },
+      undefined,
+      { createNullsLast: true }
    );

I could then make a GraphQL request for TOP_RATED_NULLS_LAST.

---

The new createNullsLast and createNullsFirst options should be completely optional and have no breaking changes whatsoever. They should also be both completely type-safe and run-time safe.

I have not written Jest tests for this feature as it's quite a short PR. If necessary I can do that. I am however using it in my current PostGraphile build (using the starter project as a base) and it seems to work as intended.

If the feature is acceptable in general, I can also gladly add it to the documentation.

Thanks for reading! 😄

---

No fixes.

Discord discussion: https://discord.com/channels/489127045289476126/498852330754801666/840304986117898240

Performance impact

Unknown (minimal hopefully?)

Security impact

Unknown (none?)

Checklist

• My code matches the project's code style and yarn lint:fix passes.
• I've added tests for the new feature, and yarn test passes.
• I have detailed the new feature in the relevant documentation.
• I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
• If this is a breaking change I've explained why.

[benjie]

I've pondered this a bit and have concluded that I don't like the explosion of enums that this could invoke. Instead I propose that we stick with this method generating just two enum values, but what those values do is controlled by a single option:

nulls: 'first' | 'last' | 'first-iff-ascending' | 'last-iff-ascending' = 'last-iff-ascending'

e.g.

const customOrderBy = orderByAscDesc(
  'RATING_NULLS_LOW',
  (helpers) => {
    const { queryBuilder } = helpers;

    const orderByFrag = sql.fragment`(
      select avg(${sqlIdentifier}.rating)
      from app_public.profile_reviews as ${sqlIdentifier}
      where ${sqlIdentifier}.reviewee_profile_id = ${queryBuilder.getTableAlias()}.id
    )`;

    return orderByFrag;
  },
  { nulls: 'first-iff-ascending' }
);

would generate

• RATING_NULLS_LOW_ASC = order by rating asc nulls first
• RATING_NULLS_LOW_DESC = order by rating desc nulls last

You could then generate the other enums if you like by adding additional calls to orderByAscDesc; e.g. RATING (last-iff-ascending), RATING_NULLS_LAST (last), RATING_NULLS_FIRST (first).

---

Also, let's change the signature of the function so that it only accepts 3 arguments by using overloads:

interface OrderByAscDescOptions {
  unique?: boolean;
  nulls: 'first' | 'last' | 'first-iff-ascending' | 'last-iff-ascending' = 'last-iff-ascending'
}

export function orderByAscDesc(
  baseName: string,
  columnOrSqlFragment: OrderBySpecIdentity,
  unique?: boolean,
): MakeAddPgTableOrderByPluginOrders;
export function orderByAscDesc(
  baseName: string,
  columnOrSqlFragment: OrderBySpecIdentity,
  options?: OrderByAscDescOptions,
): MakeAddPgTableOrderByPluginOrders;
export function orderByAscDesc(
  baseName: string,
  columnOrSqlFragment: OrderBySpecIdentity,
  optionsOrUnique: boolean | OrderByAscDescOptions = false,
): MakeAddPgTableOrderByPluginOrders {
  const options =
    typeof optionsOrUnique === 'boolean'
      ? { unique: optionsOrUnique }
      : optionsOrUnique ?? {};
  const { unique, nulls } = options;

(We have to accept the overloads because we cannot break the existing function signature.)

---

Aside: for your use case you'd just call RATING_NULLS_LOW RATING, and this'd be the only one you want I think?

[benjie]

(In case you're not familiar, iff means "if and only if".)

[Deckstar]

Hey Benjie, thanks a lot for taking the time to review my PR.

I like your solution! I'll try to implement it later today and then I'll update the PR.

Two questions:

• since all three functions return the same type (MakeAddPgTableOrderByPluginOrders), is function overloading necessary here? For cleaner code, maybe it would be easier to just call the last variable

uniqueOrOptions: boolean | OrderByAscDescOptions = false,

and then write only one function?

• Also, in the options object case: I guess if the user doesn't specify unique in the object (e.g. if they just use { nulls: 'last' }, then we need to specify the default as false, right? i.e.:

- const { unique, nulls } = options;
+ const { unique = false, nulls } = options;

(I'm assuming that passing in unique as undefined might break something).

[Deckstar]

Also, in response to this:

Aside: for your use case you'd just call RATING_NULLS_LOW RATING, and this'd be the only one you want I think?

In the plugin above, it is true that I'd probably only use the DESC with nulls last option (I don't foresee a need for querying for "lowest rated of those who were rated". Except maybe in an admin page for moderators, who might be wondering who they should ban or something? 😄 ). Regardless, I still thought that it was a useful enough piece of functionality to make it customizeable by the developer in orderByAscDesc.

For instance, I can foresee a use-case where I might want to query for profiles with the closest/farthest location, while leaving those who haven't set a location to be last (still discoverable, just penalized in search results).

[benjie]

is function overloading necessary here?

No... and I'm honestly not sure why I thought it was. I blame lack of sleep 😴

[...unique, nulls...]

Yes, apply defaults wherever it's sensible. I don't think the = default I added in the interface even works... because interfaces can't have defaults like that? I think I was thinking in GraphQL rather than JS... 💤

[...everything else...]

Agreed.

[Deckstar]

Hahah 😄

In fact I tried to add a default = like how you did it, but got a big fat linter error 😄

Great to hear that we are agreed! Is there anything else I should do before the PR can be mergeable? (E.g. should I write tests and update the documentation myself)? Or do you prefer to handle it yourself?

[benjie]

We should keep the default behaviour unless overridden (sorry for misleading you with the default). Let's allow nulls = undefined.

[benjie]

If not valid; throw? (Remembering to allow for undefined, to allow default behaviour.)

[benjie]

We should try to mirror (or, preferably, invoke) the logic here:

graphile-engine/packages/graphile-build-pg/src/plugins/PgConnectionArgOrderBy.js

Lines 157 to 164 in b715f6d

	// If the enum specifies null ordering, use that
	// Otherwise, use the orderByNullsLast option if present
	const nullsFirst =
	specNullsFirst != null
	? specNullsFirst
	: orderByNullsLast != null
	? !orderByNullsLast
	: undefined;

Namely there's a graphileBuildOptions.orderByNullsLast setting that orders with nulls last always. So either factor that into the default:

const { unique = false, nulls = orderByNullsLast ? "last" : "last-iff-ascending" } = options

Or, preferably, allow nulls to be undefined and in that case go with the default (current) behaviour.

[benjie]

(E.g. should I write tests and update the documentation myself)? Or do you prefer to handle it yourself?

I'd appreciate help with those. Generally I leave them until the functionality is agreed upon though, because there's nothing more frustrating than throwing away tests.

[Deckstar]

Made some updates 😄 Let me know if it's fine and then I'll get started on tests / documentation.

[benjie]

Looking good 👍

[benjie]

Please pull all version changes back out of the git history (this should mean CHANGELOG, lerna.json, package.json, etc are all unmodified).

[Deckstar]

Done 😄

[benjie]

This is ready for tests - thanks @Deckstar!

[benjie]

Looks good; just needs tests! 👍

[Deckstar]

Awesome! Thanks! 😄 I'll try to write some over the next few days.

[benjie]

These tests cover it nicely 👍