graphile / crystal

🔮 Graphile's Crystal Monorepo; home to Grafast, PostGraphile, pg-introspection, pg-sql2 and much more!
https://graphile.org/
Other
12.62k stars 571 forks source link

Fix issues with planning errors and hasSideEffects=true #2132

Closed benjie closed 3 months ago

benjie commented 3 months ago

Description

Fixes https://github.com/benjie/ouch-my-finger/pull/13

Setting $step.hasSideEffects = true on a step is only allowed just after the step is created (or during its creation) because we track the fact of a side effect having happened in the layer plan so that all future steps are implicitly "dependent" on the side effect.

However, this check was too strict - it's okay for other steps to have been created, so long as $step ultimately depends on them. We just want to make it so that future steps (those that $step does not depend on) will implicitly depend on $step succeeding. This has been fixed.

Related; when a planning error occurs we "roll back" the plan diagram to a previous state (throwing away older steps). However; when we did this, we did not clear the fact that we were tracking a latest side effect step on the LayerPlan, which resulted in attempting to reference said step at runtime even though it had been removed. This has also been resolved.

Performance impact

None known.

Security impact

None known.

Checklist

changeset-bot[bot] commented 3 months ago

🦋 Changeset detected

Latest commit: 8076500354a3e2bc2de1b6c4e947bd710cc5bddc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages | Name | Type | | -------------------------- | ----- | | @dataplan/pg | Patch | | grafast | Patch | | graphile-build-pg | Patch | | graphile-utils | Patch | | pgl | Patch | | postgraphile | Patch | | @localrepo/grafast-bench | Patch | | @dataplan/json | Patch | | @grafserv/persisted | Patch | | grafserv | Patch | | ruru-components | Patch | | @localrepo/grafast-website | Patch | | graphile-build | Patch | | graphile-export | Patch | | graphile | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR