Feedback on Relation Inputs Plugin

[litewarp]

Would love some feedback on efforts to port the nested-mutations-plugin form v4 to v5.

https://github.com/litewarp/graphile-relation-inputs-plugin

Thanks again for all your hard work.

• am interested in building this feature myself
• am interested in collaborating on building this feature
• am willing to help testing this feature before it's released
• am willing to write a test-driven test suite for this feature (before it exists)
• am a Graphile sponsor ❤️
• have an active support or consultancy contract with Graphile

[benjie]

https://github.com/litewarp/graphile-relation-inputs-plugin/blob/974192646d442321a59575e0890c8751698e07f5/src/field-inputs-plugin.ts#L384

This is dangerous; instead store a reference:

const oldPlan = field.plan

then overwrite the plan explicitly:

field.plan = EXPORTABLE(...)
return field;

[benjie]

https://github.com/litewarp/graphile-relation-inputs-plugin/blob/974192646d442321a59575e0890c8751698e07f5/src/init-create-plugin.ts#L110-L111

This is dangerous; types/codecs/etc may appear in different orders, just picking the first and continuing can lead to an unstable schema. If there's a conflict, you should explicitly raise that fact (as an error).

[benjie]

The main issue that I see is that this does not perform the mutations inside a transaction, so you can get partial success. The only way to achieve the transaction approach currently is to do all of the mutations in a single step (e.g. via withPgClient()) - you can't use the pgUpdateSingle/etc steps within a transaction. We may or may not support doing so in future, via "subroutines" (I'd like to, but I don't know if or when I'll be able to do it).

[litewarp]

Thanks for the feedback.

This is interesting because I assumed that using the steps would let grafast optimize things.

I had considered ejecting from the steps and handling in a single sql statement (basically what the v4 plugin does), but thought it would be preferable to do it through the grafast system.

Back to the drawing board!

[benjie]

In general, Grafast won't optimise steps that have sideEffects because it's hard to predict what impact those side effects might have. The payloads could be optimized, but the lack of transaction is the main concern here.

[litewarp]

That's fair. And makes sense. It seems like a shame to abandon (eject from?) grafast to accomplish these embedded mutations though.

Lack of these embedded mutations has been a blocker for us, so I'm trying to get something together.
Is it fair to say that there would be no downside to writing a traditional "resolver" here? Or is there a benefit to creating a step to handle them?

[benjie]

If you use a true resolver the schema becomes impure and certain optimisations cannot be used; but there should be no need: pass the parent arg, fieldArgs, and context() into a sideEffect plan and you’ve got the majority of a resolver right there (all you’re missing is resolveInfo).