feat: add modifiedSchema on preExecution hooks

[Eomm]

Opening this PR to get some feedback.

I'm working on this issue: mercurius-js/auth#43
The target of that issue is to modify the output schema for inspection queries like so

app.graphql.addHook('preExecution', async function filterHook (schema, document, context) {

  // removes those objects that the user has not access to
  const filteredSchema = auth.filterDirectives(schema, authSchema)

  return {
    schema: filteredSchema
  }
})

The inspection query is a standard query that is being executed on the app.graphql.schema object.

To let the user (and in this case the mercurius/auth plugin) change the schema where the query is executed and affect the output results, the preExecution hook may manage a schema replacing.

Note that I did not use app.graphql.transformSchema because it would replace the initial/original schema: this is unwanted: the target here is to replace the schema only for a single request execution.

Doing so, every request may change the schema accordingly to its data (such as client's role&permissions).

This PR lacks documentation, but I would like to know if you are OK with this solution before add it.
Feel free to suggest some additional tests too

Thanks

[mcollina]

To let the user (and in this case the mercurius/auth plugin) change the schema where the query is executed and affect the output results, the preExecution hook may manage a schema replacing.

This seems a really bad idea. What about concurrency issues?

[Eomm]

What about concurrency issues?

I don't expect those issues since the "temporary-schema" is used on the request's scope and it must not affect the global schema that remains the default.

I can arrange a test for it as well

[Eomm]

This seems a really bad idea.

I did not find a way to implement the desired logic using the actual API, such as the onResolution hook, mainly because there is not a reference to the (auth) resolver to execute to decide if a Graph FIELD or OBJECT should be returned or not.

I agree that this could be a dangerous feature to use with caution - the risk is a broken API.
Could we protect the user from misusing this feature?

[mcollina]

Can you add docs?

[mcollina]

lgtm

[mcollina]

Please add a few test with jit enabled.

[mcollina]

This should be documented. I think the same problem might happen for modifiedDocument too - we might end up fixing a bug.

[Eomm]

Fixed

I think the same problem might happen for modifiedDocument

I think the modifiedDocument is worth being jit'ed it is a consistent update.
For example, a developer is renaming a field from old to new

[mcollina]

are we using modifiedQuery anywhere?

[Eomm]

It is used on the gateway implementation

[mcollina]

Maybe make it a getter?

[Eomm]

It was unused.
Removed

The same name is used on the preGatewayExecutionHandler

[mcollina]

Suggested change

	if (!modifiedSchema) {
	if (!modifiedSchema || !modifiedDocument) {

[Eomm]

If we change the schema but not the document the jit is executed against the "origin" schema.
Is it useful creating the jit when the document will be executed using the modifiedSchema?

[mcollina]

I don't understand.

I think the modifiedDocument is worth being jit'ed it is a consistent update.
For example, a developer is renaming a field from old to new

What makes me itchy in jiting this is that the hook takes the context as an argument, therefore those changes could be based on user credentials. Compiling those in is a potential security hazard.

What we really need is another field to defer this to the user. Having a cacheable or jit property would put the developer in charge of what is happening.

[Eomm]

I get the point, I will try to arrange a security test for this case.

The current PR's code blocks the jit execution when the schema is modified by the hook.
The suggested code enables the jit when the user changes the modifiedSchema property that seems unwanted.

What we really need is another field to defer this to the user. Having a cacheable or jit property would put the developer in charge of what is happening.

Clear, I would add it in a follow-up PR because there are a few cases that must be discussed or would you like to upgrade this PR instead?

The doubts concern:

• should we provide the cache counter to the hook function using the context?
• does the cacheable property have priority over the jit option?

[mcollina]

should we provide the cache counter to the hook function using the context?

Unsure about this

does the cacheable property have priority over the jit option?

Yes

[mcollina]

lgtm

[jonnydgreen]

Looks good! As well as the getter added on the modifiedQuery could we also update the typings and the typing tests? https://github.com/mercurius-js/mercurius/blob/master/index.d.ts#L788

[jonnydgreen]

LGTM