From 7bc301e184afb95d8acd77294e2c23a6989ebc34 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 23 Apr 2025 10:12:31 +0200 Subject: [PATCH] Make `Web::PushSubscription#user` and `Web::PushSubscription#access_token` relationships non-optional (#34498) Co-authored-by: Emelia Smith --- app/models/web/push_subscription.rb | 38 +++---------------- .../web/notification_serializer.rb | 2 +- ..._not_null_to_web_push_subscription_user.rb | 7 ++++ ..._not_null_to_web_push_subscription_user.rb | 19 ++++++++++ ...l_to_web_push_subscription_access_token.rb | 7 ++++ ...l_to_web_push_subscription_access_token.rb | 19 ++++++++++ db/schema.rb | 6 +-- .../web_push_subscription_fabricator.rb | 2 + .../api/v1/push/subscriptions_spec.rb | 3 +- 9 files changed, 66 insertions(+), 37 deletions(-) create mode 100644 db/migrate/20250422083912_add_not_null_to_web_push_subscription_user.rb create mode 100644 db/migrate/20250422084214_validate_add_not_null_to_web_push_subscription_user.rb create mode 100644 db/migrate/20250422085027_add_not_null_to_web_push_subscription_access_token.rb create mode 100644 db/migrate/20250422085303_validate_add_not_null_to_web_push_subscription_access_token.rb diff --git a/app/models/web/push_subscription.rb b/app/models/web/push_subscription.rb index 12d843cd09..25140598a5 100644 --- a/app/models/web/push_subscription.rb +++ b/app/models/web/push_subscription.rb @@ -12,13 +12,13 @@ # standard :boolean default(FALSE), not null # created_at :datetime not null # updated_at :datetime not null -# access_token_id :bigint(8) -# user_id :bigint(8) +# access_token_id :bigint(8) not null +# user_id :bigint(8) not null # class Web::PushSubscription < ApplicationRecord - belongs_to :user, optional: true - belongs_to :access_token, class_name: 'Doorkeeper::AccessToken', optional: true + belongs_to :user + belongs_to :access_token, class_name: 'Doorkeeper::AccessToken' has_one :session_activation, foreign_key: 'web_push_subscription_id', inverse_of: :web_push_subscription, dependent: nil @@ -28,7 +28,7 @@ class Web::PushSubscription < ApplicationRecord validates_with WebPushKeyValidator - delegate :locale, to: :associated_user + delegate :locale, to: :user generates_token_for :unsubscribe, expires_in: Web::PushNotificationWorker::TTL @@ -36,24 +36,8 @@ class Web::PushSubscription < ApplicationRecord policy_allows_notification?(notification) && alert_enabled_for_notification_type?(notification) end - def associated_user - return @associated_user if defined?(@associated_user) - - @associated_user = if user_id.nil? - session_activation.user - else - user - end - end - def associated_access_token - return @associated_access_token if defined?(@associated_access_token) - - @associated_access_token = if access_token_id.nil? - find_or_create_access_token.token - else - access_token.token - end + access_token.token end class << self @@ -65,16 +49,6 @@ class Web::PushSubscription < ApplicationRecord private - def find_or_create_access_token - Doorkeeper::AccessToken.find_or_create_for( - application: Doorkeeper::Application.find_by(superapp: true), - resource_owner: user_id || session_activation.user_id, - scopes: Doorkeeper::OAuth::Scopes.from_string('read write follow push'), - expires_in: Doorkeeper.configuration.access_token_expires_in, - use_refresh_token: Doorkeeper.configuration.refresh_token_enabled? - ) - end - def alert_enabled_for_notification_type?(notification) truthy?(data&.dig('alerts', notification.type.to_s)) end diff --git a/app/serializers/web/notification_serializer.rb b/app/serializers/web/notification_serializer.rb index fb2f7248a6..1c2b26bf5b 100644 --- a/app/serializers/web/notification_serializer.rb +++ b/app/serializers/web/notification_serializer.rb @@ -13,7 +13,7 @@ class Web::NotificationSerializer < ActiveModel::Serializer end def preferred_locale - current_push_subscription.associated_user&.locale || I18n.default_locale + current_push_subscription.user&.locale || I18n.default_locale end def notification_id diff --git a/db/migrate/20250422083912_add_not_null_to_web_push_subscription_user.rb b/db/migrate/20250422083912_add_not_null_to_web_push_subscription_user.rb new file mode 100644 index 0000000000..2af08edc40 --- /dev/null +++ b/db/migrate/20250422083912_add_not_null_to_web_push_subscription_user.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddNotNullToWebPushSubscriptionUser < ActiveRecord::Migration[8.0] + def change + add_check_constraint :web_push_subscriptions, 'user_id IS NOT NULL', name: 'web_push_subscriptions_user_id_null', validate: false + end +end diff --git a/db/migrate/20250422084214_validate_add_not_null_to_web_push_subscription_user.rb b/db/migrate/20250422084214_validate_add_not_null_to_web_push_subscription_user.rb new file mode 100644 index 0000000000..af1962a8fb --- /dev/null +++ b/db/migrate/20250422084214_validate_add_not_null_to_web_push_subscription_user.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class ValidateAddNotNullToWebPushSubscriptionUser < ActiveRecord::Migration[8.0] + def up + connection.execute(<<~SQL.squish) + DELETE FROM web_push_subscriptions + WHERE user_id IS NULL + SQL + + validate_check_constraint :web_push_subscriptions, name: 'web_push_subscriptions_user_id_null' + change_column_null :web_push_subscriptions, :user_id, false + remove_check_constraint :web_push_subscriptions, name: 'web_push_subscriptions_user_id_null' + end + + def down + add_check_constraint :web_push_subscriptions, 'user_id IS NOT NULL', name: 'web_push_subscriptions_user_id_null', validate: false + change_column_null :web_push_subscriptions, :user_id, true + end +end diff --git a/db/migrate/20250422085027_add_not_null_to_web_push_subscription_access_token.rb b/db/migrate/20250422085027_add_not_null_to_web_push_subscription_access_token.rb new file mode 100644 index 0000000000..167bc71b72 --- /dev/null +++ b/db/migrate/20250422085027_add_not_null_to_web_push_subscription_access_token.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddNotNullToWebPushSubscriptionAccessToken < ActiveRecord::Migration[8.0] + def change + add_check_constraint :web_push_subscriptions, 'access_token_id IS NOT NULL', name: 'web_push_subscriptions_access_token_id_null', validate: false + end +end diff --git a/db/migrate/20250422085303_validate_add_not_null_to_web_push_subscription_access_token.rb b/db/migrate/20250422085303_validate_add_not_null_to_web_push_subscription_access_token.rb new file mode 100644 index 0000000000..322d376667 --- /dev/null +++ b/db/migrate/20250422085303_validate_add_not_null_to_web_push_subscription_access_token.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class ValidateAddNotNullToWebPushSubscriptionAccessToken < ActiveRecord::Migration[8.0] + def up + connection.execute(<<~SQL.squish) + DELETE FROM web_push_subscriptions + WHERE access_token_id IS NULL + SQL + + validate_check_constraint :web_push_subscriptions, name: 'web_push_subscriptions_access_token_id_null' + change_column_null :web_push_subscriptions, :access_token_id, false + remove_check_constraint :web_push_subscriptions, name: 'web_push_subscriptions_access_token_id_null' + end + + def down + add_check_constraint :web_push_subscriptions, 'access_token_id IS NOT NULL', name: 'web_push_subscriptions_access_token_id_null', validate: false + change_column_null :web_push_subscriptions, :access_token_id, true + end +end diff --git a/db/schema.rb b/db/schema.rb index 6b1aa81bd0..0cabf8997c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2025_04_11_095859) do +ActiveRecord::Schema[8.0].define(version: 2025_04_22_085303) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -1237,8 +1237,8 @@ ActiveRecord::Schema[8.0].define(version: 2025_04_11_095859) do t.json "data" t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false - t.bigint "access_token_id" - t.bigint "user_id" + t.bigint "access_token_id", null: false + t.bigint "user_id", null: false t.boolean "standard", default: false, null: false t.index ["access_token_id"], name: "index_web_push_subscriptions_on_access_token_id", where: "(access_token_id IS NOT NULL)" t.index ["user_id"], name: "index_web_push_subscriptions_on_user_id" diff --git a/spec/fabricators/web_push_subscription_fabricator.rb b/spec/fabricators/web_push_subscription_fabricator.rb index 6b4028342c..458aa8296e 100644 --- a/spec/fabricators/web_push_subscription_fabricator.rb +++ b/spec/fabricators/web_push_subscription_fabricator.rb @@ -8,4 +8,6 @@ Fabricator(:web_push_subscription, from: Web::PushSubscription) do Base64.urlsafe_encode64(ecdh_key) end key_auth { Base64.urlsafe_encode64(Random.new.bytes(16)) } + user { Fabricate(:user) } + access_token { |attrs| Fabricate.build(:accessible_access_token, resource_owner_id: attrs[:user].id) } end diff --git a/spec/requests/api/v1/push/subscriptions_spec.rb b/spec/requests/api/v1/push/subscriptions_spec.rb index 69adeb9b6f..359de9d95c 100644 --- a/spec/requests/api/v1/push/subscriptions_spec.rb +++ b/spec/requests/api/v1/push/subscriptions_spec.rb @@ -207,7 +207,8 @@ RSpec.describe 'API V1 Push Subscriptions' do Fabricate( :web_push_subscription, endpoint: create_payload[:subscription][:endpoint], - access_token_id: token.id + access_token: token, + user: user ) end end