From 4e2480c369a1ac15557cd4917394ff8759b64f89 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 22:23:36 +0000 Subject: [PATCH 1/2] Fix notification update and destroy under Mongoid 9, and their N+1 `NotificationsController#update` and `#destroy` raise on every request: Mongoid::Errors::InvalidQuery: Expression must be a Hash: #...}> `notification_params` returns ActionController::Parameters, which is not a Hash, and Mongoid 9 requires a query expression to be one. Mongoid 8 accepted it. This is a regression from the Mongoid 8 -> 9 upgrade in #892, already on master, so marking notifications as read and dismissing them are both broken right now. Fixed by calling `to_h`, which is safe because the parameters have been through `permit`. I audited the rest of app/ and lib/ for the same shape. Every other case is either ActiveRecord or ActiveModel, which still accept permitted Parameters, or a plain Hash built by hand -- `ReactionsController#reaction_params` is the latter. This was the only Mongoid one. Two N+1s went with it, both of which made the index action untestable under `Bullet.raise`: - `after_initialize :set_defaults` recomputed `post_id` by reaching through `notificateable` on *every* instantiation, including records read back from the database that already had the value stored. Now guarded on `post_id.blank?`, which still backfills documents written before the field existed while costing nothing for the rest. - The index aggregation titles each group through `notificateable`, so the query now eager loads it. Mongoid 9 does handle `includes` for this polymorphic belongs_to; I checked before relying on it. 469 examples, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) --- .../app/controllers/api/v1/notifications_controller.rb | 10 ++++++++-- backend/app/models/notification.rb | 6 +++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/backend/app/controllers/api/v1/notifications_controller.rb b/backend/app/controllers/api/v1/notifications_controller.rb index 0e4e6d6e9..6fb173032 100644 --- a/backend/app/controllers/api/v1/notifications_controller.rb +++ b/backend/app/controllers/api/v1/notifications_controller.rb @@ -2,7 +2,11 @@ module Api module V1 class NotificationsController < ApplicationController def index - notifications = Notification.where(encrypted_notify_user_id: current_user.encrypted_id) + # The aggregation reaches through `notificateable` to title each group, so without + # eager loading this is one query per notification. + notifications = Notification + .where(encrypted_notify_user_id: current_user.encrypted_id) + .includes(:notificateable) authorize_collection :index, notifications @@ -41,7 +45,9 @@ def notification_params parameters[:notificateable_type] = parameters[:notificateable_type].titleize parameters[:encrypted_notify_user_id] = current_user.encrypted_id - parameters + # Mongoid 9 requires a query expression to be a Hash and raises InvalidQuery on + # an ActionController::Parameters, which Mongoid 8 accepted. + parameters.to_h end def authorize_collection(name, collection) diff --git a/backend/app/models/notification.rb b/backend/app/models/notification.rb index d837ece8c..10e96c2c7 100644 --- a/backend/app/models/notification.rb +++ b/backend/app/models/notification.rb @@ -3,7 +3,11 @@ class Notification include Mongoid::Timestamps include Usernameable - after_initialize :set_defaults + # Only derive post_id when it is not already stored. This runs on every instantiation, + # including records loaded from the database, and it reaches through `notificateable`, + # so without the guard every Notification loaded costs an extra query to recompute a + # value it was already holding. + after_initialize :set_defaults, if: -> { post_id.blank? } field :kind, type: String field :encrypted_user_id, type: String, encrypted: {type: :integer} From b86b0934f7dc82db6396f760dfd67bb0b7a23593 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 22:23:42 +0000 Subject: [PATCH 2/2] Cover the notifications controller All three actions, including the grouping that collapses repeated notifications on one subject into a single entry with a count. This is what surfaced both the Mongoid 9 regression and the N+1s fixed in the preceding commit -- update and destroy raised on the first request a spec ever made to them. 469 examples, 0 failures. app/ and lib/ coverage 87.85% -> 88.94%. Co-Authored-By: Claude Opus 5 (1M context) --- .../api/v1/notifications_controller_spec.rb | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 backend/spec/controllers/api/v1/notifications_controller_spec.rb diff --git a/backend/spec/controllers/api/v1/notifications_controller_spec.rb b/backend/spec/controllers/api/v1/notifications_controller_spec.rb new file mode 100644 index 000000000..31cb8ebc7 --- /dev/null +++ b/backend/spec/controllers/api/v1/notifications_controller_spec.rb @@ -0,0 +1,95 @@ +require "rails_helper" + +RSpec.describe Api::V1::NotificationsController do + let(:user) { create(:user) } + let(:notifier) { create(:user) } + let(:notified_post) { create(:post, encrypted_user_id: user.encrypted_id) } + + let!(:notification) do + create(:notification, + kind: "comment", + notificateable: notified_post, + encrypted_user_id: notifier.encrypted_id, + encrypted_notify_user_id: user.encrypted_id) + end + + before { sign_in user } + + describe "index" do + it "returns the signed-in user's notifications grouped by kind and subject" do + get :index + + expect(response).to have_http_status :ok + expect(response_body[:notifications].size).to eq 1 + expect(response_body[:notifications].first[:kind]).to eq "comment" + expect(response_body[:notifications].first[:count]).to eq 1 + end + + it "collapses repeated notifications on the same subject into a count" do + create(:notification, + kind: "comment", + notificateable: notified_post, + encrypted_user_id: create(:user).encrypted_id, + encrypted_notify_user_id: user.encrypted_id) + + get :index + + expect(response_body[:notifications].size).to eq 1 + expect(response_body[:notifications].first[:count]).to eq 2 + end + + it "does not return notifications addressed to somebody else" do + create(:notification, + kind: "reaction", + notificateable: create(:post), + encrypted_notify_user_id: create(:user).encrypted_id) + + get :index + + expect(response_body[:notifications].map { |n| n[:kind] }).to eq ["comment"] + end + end + + describe "update" do + let(:subject_params) do + {notificateable_id: notified_post.id.to_s, notificateable_type: "post"} + end + + it "marks the notifications for that subject as read" do + put :update, params: subject_params + + expect(response).to have_http_status :ok + expect(notification.reload.unread).to be false + end + + it "returns the regrouped notifications" do + put :update, params: subject_params + + expect(response_body[:notifications].first[:unread]).to be false + end + + it "leaves another user's notifications on the same subject alone" do + theirs = create(:notification, + kind: "comment", + notificateable: notified_post, + encrypted_notify_user_id: create(:user).encrypted_id) + + put :update, params: subject_params + + expect(theirs.reload.unread).to be true + end + end + + describe "destroy" do + it "removes the notifications for that subject" do + expect { + delete :destroy, params: { + notificateable_id: notified_post.id.to_s, + notificateable_type: "post" + } + }.to change { Notification.count }.by(-1) + + expect(response).to have_http_status :no_content + end + end +end