v5.7.1 - Cache-Invalidation Hooks, Handler Isolation, Transaction Rollback

CHANGELOG.md

@@ -1,5 +1,85 @@
 ## parse-stack-next Changelog
 
+### 5.7.1
+
+#### Cache-invalidation webhooks no longer break every application hook for the same trigger
+
+- **FIXED**: `Parse::Cache::Invalidation` raised `NoMethodError: undefined
+  method 'guard'` on every `_User`, `_Role`, and `_Session` trigger it
+  registered, and the failure was not contained. A webhook handler block is
+  bound to the payload before it runs, so the bare `guard` / `bump_subject` /
+  `subject_id` calls in the handler bodies resolved against
+  `Parse::Webhooks::Payload`, which defines none of them. `guard`'s own
+  `rescue` never ran, because it lives inside the body that was never entered,
+  so the error escaped into the route dispatcher. The dispatcher folds a
+  trigger's handlers with `Array#map`, which abandons the collection on the
+  first raise, and these triggers install during `Parse.setup` and therefore
+  sit ahead of every application handler for the same trigger. One
+  unresolvable method name silently prevented an application's own
+  `after_save "_User"` hook from running at all, so any work downstream of it
+  stopped without a visible cause. The handlers now call the module on an
+  explicit receiver captured in a local, which is independent of whatever
+  `self` the dispatcher binds. Applications that set
+  `cache_invalidation_hooks: false` to work around this can remove it.
+- **FIXED**: The invalidation tests dispatched handlers with a plain
+  `Proc#call`, which leaves `self` bound to the module the block closed over,
+  so every one of them passed against handlers that could not run in
+  production. They now dispatch through the real handler invocation and fail
+  against the broken code.
+
+#### Whether one failing `after_*` handler stops the rest is now a setting
+
+- **NEW**: `Parse::Webhooks.abort_after_callbacks_on_error` decides whether an
+  exception raised by one `after_*` handler prevents the remaining handlers for
+  that trigger from running. It defaults to `true`, which is the existing
+  behavior, so nothing changes on upgrade. Set it to `false` to isolate
+  handlers from each other: each one runs regardless of what an earlier one
+  raised, and the failure is reported with a warning and a
+  `parse.webhooks.handler_error` notification instead of being swallowed
+  silently. This was previously not a decision at all but a side effect of
+  folding handlers with `Array#map`, which abandons the collection on the
+  first raise. Registration order is not fully under an application's control,
+  since the SDK's own cache-invalidation triggers install during `Parse.setup`
+  and therefore sit ahead of handlers registered by application files loaded
+  later, so a single raising handler could silently prevent an application's
+  own hooks from running with nothing logged to say they had been skipped.
+- **BEHAVIOR**: The setting governs only whether later handlers still run.
+  Neither mode reverts anything: an `after_*` trigger fires once the write has
+  already committed, so there is no version of this setting that can undo a
+  save. `before_*` dispatch is untouched, and only the accumulating,
+  non-rejectable triggers (`after_save`, `after_delete`, `after_logout`) can
+  hold more than one handler in the first place. A rejectable `before_*`
+  trigger must deny if any handler denies, so its raise must continue to
+  abort.
+
+#### Failed transactions restore the object they rolled back
+
+- **FIXED**: A failed `Parse::Object.transaction` left the in-memory object
+  holding its modified values. The rollback snapshotted `Parse::Object#attributes`
+  and restored it, but that method returns a schema map of field name to type
+  symbol rather than values, so nothing was ever restored. Property values
+  live in `@<field>` instance variables; those are now what the rollback
+  captures and restores, including values nested inside arrays and hashes,
+  relation operation queues, and properties whose ivar did not exist before
+  the transaction. State is captured when the object first enters the
+  transaction rather than at `batch.add`, so the documented pattern of
+  mutating an object and adding it afterwards rolls back correctly.
+- **FIXED**: A failed transaction also left the object with broken change
+  tracking. Restoring the schema map defined `@attributes`, which is the
+  instance variable `ActiveModel::Dirty` keys its behavior on: once defined it
+  builds an `AttributeMutationTracker` over that hash instead of the
+  `ForcedMutationTracker` a `Parse::Object` needs, and `clear_changes!` then
+  called `forgetting_assignment` on the `[key, value]` pairs `Hash#map`
+  yields. Both call sites rescue and warn, so every rollback quietly
+  downgraded the object rather than failing. The rollback no longer defines
+  `@attributes`.
+- **FIXED**: The mongo-direct role-graph integration test gated on
+  `ANALYTICS_DATABASE_URI`, a production variable name the test stack never
+  sets, so both of its traversal assertions skipped on every run while the
+  per-file reporter still reported the file as passing. It now reads
+  `PARSE_TEST_MONGO_URI` like every other mongo-direct integration file and
+  still honors `ANALYTICS_DATABASE_URI` as an override.
+
 ### 5.7.0
 
 #### Cache keys move into a reserved, app-scoped keyspace
@@ -205,7 +285,7 @@
   Server's self-access rules, and role-only checks cannot claim a concrete
   member's pointer or `_User` self permission. CLP cache entries are isolated
   by Parse application so identically named classes cannot leak policy across
-  clients. These helpers are advisory—the eventual Parse Server request is
+  clients. These helpers are advisory: the eventual Parse Server request is
   still authoritative.
 
 #### Test infrastructure

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    parse-stack-next (5.7.0)
+    parse-stack-next (5.7.1)
       activemodel (>= 6.1, < 9)
       activesupport (>= 6.1, < 9)
       connection_pool (>= 2.2, < 4)

lib/parse/cache/invalidation.rb

@@ -52,70 +52,122 @@ def install!(cache)
           registered
         end
 
+        # Run a role-trigger invalidation.
+        #
+        # Public because the registered webhook block calls it on an explicit
+        # receiver. See {handle_identity_trigger} for why that matters.
+        #
+        # @!visibility private
+        # @param cache [Parse::Cache::Redis] the keyspace-configured cache.
+        # @return [void]
+        def handle_role_trigger(cache)
+          guard do
+            # A role write does not say which users are affected: membership
+            # and hierarchy changes arrive as relation deltas on `users` and
+            # `roles`, and the cached value is a flattened transitive closure,
+            # so a parent-role change reaches the members of every child.
+            # Clearing the whole plane is both correct and cheap under a
+            # scoped SCAN. Parse Server does the same, for the same reason.
+            cache.roles.clear
+            # Stamp the epoch so a *foreign* role entry written before this
+            # moment is rejected on read. Parse Server does not clear its own
+            # role cache on a `_Role` delete, so without this the next read
+            # would take its stale entry back and our clear would shorten
+            # revocation by nothing.
+            cache.roles.touch_epoch
+          end
+        end
+
+        # Run an identity-trigger invalidation.
+        #
+        # Public, and invoked on an explicit receiver, because a webhook
+        # handler block does NOT run with `self` bound to the module that
+        # created it. {Parse::Webhooks.invoke_handler} binds the block to the
+        # payload (`payload.define_singleton_method(name, &block)`), and the
+        # historical `payload.instance_exec(payload, &block)` did the same.
+        # A block body calling a bare `guard` / `bump_subject` / `subject_id`
+        # therefore resolved against {Parse::Webhooks::Payload}, which defines
+        # none of them, and raised `NoMethodError` on every `_User`, `_Role`,
+        # and `_Session` trigger.
+        #
+        # That failure was not contained. `guard`'s `rescue` lives inside
+        # `guard`'s own body, which was never entered, so the error escaped
+        # into `call_route`'s `registry.map`. `Array#map` abandons the whole
+        # collection on the first raise, and these triggers register at
+        # `Parse.setup` and therefore sit AHEAD of any application handler for
+        # the same trigger. One unresolvable method name silently prevented
+        # every application `after_save "_User"` hook from running at all.
+        #
+        # Capturing the module in a local and calling it explicitly is what
+        # makes the handler independent of whatever `self` the dispatcher
+        # binds.
+        #
+        # @!visibility private
+        # @param cache [Parse::Cache::Redis] the keyspace-configured cache.
+        # @param type [Symbol] the trigger being handled.
+        # @param payload [Parse::Webhooks::Payload] the incoming payload.
+        # @return [void]
+        def handle_identity_trigger(cache, type, payload)
+          guard do
+            case type
+            when :after_logout
+              # The only trigger Parse Server permits on `_Session`. The
+              # object's own sessionToken is scrubbed from the payload, but
+              # the token is captured from the requesting user before
+              # scrubbing, and for a logout that user *is* the session being
+              # ended. A master-key logout carries no user, so fall back to
+              # the generation bump.
+              #
+              # Pass the RAW token, not a pre-hashed digest.
+              # `Parse::Cache::SubCache#invalidate` hashes its `key`
+              # argument internally for the `:idn` family (see
+              # `SubCache#logical_key`), the same way `#get` / `#set` do.
+              # That is what makes a `set(raw_token, ...)` /
+              # `get(raw_token)` pair round-trip. Hashing here first and
+              # handing SubCache an already-hashed value made it hash the
+              # digest a second time, landing on a key nothing had ever
+              # written to, so logout silently failed to evict the entry.
+              token = payload.respond_to?(:session_token) ? payload.session_token : nil
+              if token && !token.to_s.empty?
+                cache.identity.invalidate(token.to_s)
+              else
+                bump_subject(cache, subject_id(payload))
+              end
+            else
+              # A `_User` write gives a user id, but identity entries are
+              # keyed by session token and no reverse map exists. Bumping a
+              # per-user generation invalidates every one of that user's
+              # entries in O(1), including tokens this process has never
+              # resolved, and without Parse Server's master-key `_Session`
+              # query.
+              bump_subject(cache, subject_id(payload))
+            end
+          end
+        end
+
         private
 
         def install_role_triggers!(cache)
+          # `invalidator` is a captured LOCAL, not `self`. The registered block
+          # runs with `self` rebound to the payload, so a bare method call in
+          # its body would resolve against `Parse::Webhooks::Payload`. A local
+          # closes over correctly regardless of the receiver the dispatcher
+          # binds.
+          invalidator = self
           TRIGGERS[:role].map do |(type, class_name)|
             Parse::Webhooks.route(type, class_name) do |payload|
-              guard do
-                # A role write does not say which users are affected: membership
-                # and hierarchy changes arrive as relation deltas on `users` and
-                # `roles`, and the cached value is a flattened transitive closure,
-                # so a parent-role change reaches the members of every child.
-                # Clearing the whole plane is both correct and cheap under a
-                # scoped SCAN. Parse Server does the same, for the same reason.
-                cache.roles.clear
-                # Stamp the epoch so a *foreign* role entry written before this
-                # moment is rejected on read. Parse Server does not clear its own
-                # role cache on a `_Role` delete, so without this the next read
-                # would take its stale entry back and our clear would shorten
-                # revocation by nothing.
-                cache.roles.touch_epoch
-              end
+              invalidator.handle_role_trigger(cache)
               true
             end
             [type, class_name]
           end
         end
 
         def install_identity_triggers!(cache)
+          invalidator = self
           TRIGGERS[:identity].map do |(type, class_name)|
             Parse::Webhooks.route(type, class_name) do |payload|
-              guard do
-                case type
-                when :after_logout
-                  # The only trigger Parse Server permits on `_Session`. The
-                  # object's own sessionToken is scrubbed from the payload, but
-                  # the token is captured from the requesting user before
-                  # scrubbing, and for a logout that user *is* the session being
-                  # ended. A master-key logout carries no user, so fall back to
-                  # the generation bump.
-                  #
-                  # Pass the RAW token, not a pre-hashed digest.
-                  # `Parse::Cache::SubCache#invalidate` hashes its `key`
-                  # argument internally for the `:idn` family (see
-                  # `SubCache#logical_key`), the same way `#get` / `#set` do —
-                  # that is what makes a `set(raw_token, ...)` /
-                  # `get(raw_token)` pair round-trip. Hashing here first and
-                  # handing SubCache an already-hashed value made it hash the
-                  # digest a second time, landing on a key nothing had ever
-                  # written to, so logout silently failed to evict the entry.
-                  token = payload.respond_to?(:session_token) ? payload.session_token : nil
-                  if token && !token.to_s.empty?
-                    cache.identity.invalidate(token.to_s)
-                  else
-                    bump_subject(cache, subject_id(payload))
-                  end
-                else
-                  # A `_User` write gives a user id, but identity entries are
-                  # keyed by session token and no reverse map exists. Bumping a
-                  # per-user generation invalidates every one of that user's
-                  # entries in O(1), including tokens this process has never
-                  # resolved, and without Parse Server's master-key `_Session`
-                  # query.
-                  bump_subject(cache, subject_id(payload))
-                end
-              end
+              invalidator.handle_identity_trigger(cache, type, payload)
               true
             end
             [type, class_name]

lib/parse/model/associations/belongs_to.rb

@@ -243,6 +243,10 @@ def belongs_to(key, opts = {})
               instance_variable_set ivar, val
             end
 
+            # Capture after lazy hydration, before a caller can mutate an
+            # object returned by this association getter.
+            send(:_capture_transaction_state!) if respond_to?(:_capture_transaction_state!, true)
+
             # Track association source for N+1 detection when returning an unfetched pointer
             # Uses a registry instead of setting instance variables on the pointer object
             if val.is_a?(Parse::Pointer) && val.pointer? && Parse.warn_on_n_plus_one

lib/parse/model/associations/collection_proxy.rb

@@ -359,6 +359,9 @@ def <<(*list)
 
     # Notifies the delegate that the collection changed.
     def notify_will_change!
+      if @delegate && @delegate.respond_to?(:_capture_transaction_state!, true)
+        @delegate.send(:_capture_transaction_state!)
+      end
       collection_will_change!
       forward "#{@key}_will_change!"
     end

lib/parse/model/associations/has_many.rb

@@ -502,6 +502,10 @@ def has_many(key, scope = nil, **opts)
               val = instance_variable_get ivar
             end
 
+            # Capture before this getter materializes or returns a mutable
+            # proxy that can be changed in place.
+            send(:_capture_transaction_state!) if respond_to?(:_capture_transaction_state!, true)
+
             # if the result is not a collection proxy, then create a new one.
             unless val.is_a?(Parse::PointerCollectionProxy)
               results = []