feat(QueryBuilder): new methods for managing QB children

[singingwolfboy]

This adds three new public methods to the QueryBuilder class: buildChild, buildNamedChildFrom, and getNamedChild. These new methods will enable new capabilities for PostGraphile plugins. @benjie and I discussed this via Discord a few days ago, so I know he approves of these new methods.

I do have a few qustions:

• Should we also add a buildNamedChild method, which is like buildNamedChildFrom but does not call the select, from, or lock methods? It seems like an obvious extension of this API, but I'm not sure if it's actually useful/necessary. It might be better to wait until we find a use-case before building it.
• Should I add automated tests for this change? I don't see any automated tests for the QueryBuilder class at all.

[benjie]

It might be better to wait until we find a use-case before building it.

Agree; that's why we've allowed space for it with the naming.

Should I add automated tests for this change? I don't see any automated tests for the QueryBuilder class at all.

I'm not a huge fan of unit tests; particularly for units like QueryBuilder that are constantly evolving. I prefer integration tests. QueryBuilder is very heavily tested because literally every GraphQL query that relates to PostgreSQL is using QueryBuilder.

Before we can merge this, we will need it to be used in some tests. A simple way to do this would be to do an integration test where you add a plugin in the tests that uses this functionality, and then run some queries through it to make sure they work. Have a look at these schemas for example:

graphile-engine/packages/postgraphile-core/__tests__/integration/queries.test.js

Lines 61 to 121 in 7007a43

	createPostGraphileSchema(pgClient, ["a", "b", "c"], {
	subscriptions: true,
	appendPlugins: [
	makeExtendSchemaPlugin({
	typeDefs: gql`
	extend type Query {
	extended: Boolean
	}
	`,
	resolvers: {
	Query: {
	extended: () => true,
	},
	},
	}),
	],
	}),
	createPostGraphileSchema(pgClient, ["a", "b", "c"], {
	subscriptions: true,
	classicIds: true,
	}),
	createPostGraphileSchema(pgClient, ["a", "b", "c"], {
	subscriptions: true,
	dynamicJson: true,
	setofFunctionsContainNulls: null,
	}),
	createPostGraphileSchema(pgClient, ["a", "b", "c"], {
	subscriptions: true,
	pgColumnFilter: attr => attr.name !== "headline",
	setofFunctionsContainNulls: false,
	}),
	createPostGraphileSchema(pgClient, ["a", "b", "c"], {
	subscriptions: true,
	viewUniqueKey: "testviewid",
	setofFunctionsContainNulls: true,
	}),
	createPostGraphileSchema(pgClient, ["d"], {}),
	createPostGraphileSchema(pgClient, ["a", "b", "c"], {
	subscriptions: true,
	simpleCollections: "both",
	}),
	createPostGraphileSchema(pgClient, ["a"], {
	subscriptions: true,
	graphileBuildOptions: {
	orderByNullsLast: true,
	},
	}),
	createPostGraphileSchema(pgClient, ["smart_comment_relations"], {}),
	createPostGraphileSchema(pgClient, ["large_bigint"], {}),
	createPostGraphileSchema(pgClient, ["network_types"], {
	graphileBuildOptions: {
	pgUseCustomNetworkScalars: true,
	},
	}),
	serverVersionNum >= 100000
	? createPostGraphileSchema(pgClient, ["pg10"], {
	graphileBuildOptions: {
	pgUseCustomNetworkScalars: true,
	},
	})
	: null,

(I really need to get around to restructuring these tests.)

[benjie]

This is great; please also make sure it's used in some tests and add the TypeScript types for it into the .d.ts file 👍

[benjie]

Functionality-wise I think this is great, but as always naming is the problem! I also think we should accomodate its usage in multi-column situations. I've made some suggestions, let me know what you think.

[benjie]

If we change to buildNamedChildSelecting or similar, I think this can be replaced with something much simpler, such as:

buildSelectRaw() {
  return this._fixedSelectionExpression;
}

[singingwolfboy]

What would this._fixedSelectionExpression be, and how would it get defined? I'm assuming that it's something different from this._isSingleFieldSelector, but I'm not following you on exactly what it should be.

[benjie]

LGTM! 🎉

[benjie]

@singingwolfboy I've refactored this a bit; let me know if you're happy with it and if so we can merge. 👍

[benjie]

(The main change is that I've made the "exclusive select body" into it's own method so other plugins can use it for other things, and so that it doesn't need an unnecessary alias.)

[singingwolfboy]

Looks great to me!

[ab-pm]

I believe I understand the reasoning behind these new methods, but it would be nice if they could be documented somehow with usage instructions for plugin authors - either here on the thread, as comments in the querybuilder source code, or even on the documentation page.

Btw, should the children that are used by the builtin plugins (postgraphile-core) become named children?

[benjie]

It's rare that named children are required. This release isn't officially out yet, so documentation is not ready yet.

[ab-pm]

OK, then maybe I didn't understand their purpose completely. But it's good to know that documentation will come with the release :-)

[benjie]

I've spent all day writing docs for the new release; here's some basic documentation for this feature but I've not done much detail because I don't expect many people will need it.

https://www.graphile.org/postgraphile/make-extend-schema-plugin/#querybuilder-named-children

[ab-pm]

I thought of using a named child query builder instead of a JOIN for interfaces, when selecting fields that are stored in a shared table. So I'd want to add the subquery on-demand in the data generators, and re-use it when it already exists. However, each field uses it's own select(), and I don't know these beforehand, so I cannot use that fixedSelectExpression. I'm not sure why that is a requirement for use of buildNamedChildSelecting? I'd much rather use the original buildNamedChildFrom. Or am I overlooking something and this is the completely wrong tool?

[benjie]

I’m not sure this is the right approach for that. I don’t like the “add it if it doesn’t already exist” approach, really. Still, I’m open to you extending this area of the API if it’s the only way to make it work.

[ab-pm]

Yeah, there might be a better approach that involves more lookahead planning, but I guess making it more easy to reuse querybuilder children is still nice.

[benjie]

We've left space in the API for buildNamedChild deliberately; but I don't want to actually add that functionality until we have a concrete use case for it where it's a good fit.