Skip to content
This repository was archived by the owner on Jan 4, 2023. It is now read-only.

FIX: custom fields not merging correctly #15

Closed
wants to merge 1 commit into from
Closed

FIX: custom fields not merging correctly #15

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions services/discourse_push_notifications/pusher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ def self.push(user, payload)

def self.subscriptions(user)
user.custom_fields[DiscoursePushNotifications::PLUGIN_NAME] ||= {}
# this might be an array due to merging, so resovle the merge.
if user.custom_fields[DiscoursePushNotifications::PLUGIN_NAME].kind_of?(Array)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this is abit odd. Is there a bug with custom fields or are we incorrectly assigning it as an array?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it works most of the time, but I'm seeing that it's sometimes come back as an array from some kind of incorrect merge in custom fields:

D, [2017-11-06T20:18:47.693407 #3043] DEBUG -- :   User Load (1.1ms)  SELECT  "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1
D, [2017-11-06T20:18:47.722621 #3043] DEBUG -- :    (4.7ms)  SELECT "user_custom_fields"."name", "user_custom_fields"."value" FROM "user_custom_fields" WHERE "user_custom_fields"."user_id" = 1
I, [2017-11-06T20:18:47.729134 #3043]  INFO -- : Completed 500 Internal Server Error in 64ms (ActiveRecord: 10.1ms)


F, [2017-11-06T20:18:47.765406 #3043] FATAL -- : 
TypeError - no implicit conversion of String into Integer:
  plugins/discourse-push-notifications/services/discourse_push_notifications/pusher.rb:59:in `subscriptions'
  plugins/discourse-push-notifications/services/discourse_push_notifications/pusher.rb:68:in `subscribe'
  plugins/discourse-push-notifications/plugin.rb:66:in `subscribe'
  actionpack (5.1.4) lib/action_controller/metal/basic_implicit_render.rb:4:in `send_action'
  actionpack (5.1.4) lib/abstract_controller/base.rb:186:in `process_action'
  actionpack (5.1.4) lib/action_controller/metal/rendering.rb:30:in `process_action'
  actionpack (5.1.4) lib/abstract_controller/callbacks.rb:20:in `block in process_action'
  activesupport (5.1.4) lib/active_support/callbacks.rb:131:in `run_callbacks'
  actionpack (5.1.4) lib/abstract_controller/callbacks.rb:19:in `process_action'
  actionpack (5.1.4) lib/action_controller/metal/rescue.rb:20:in `process_action'
  actionpack (5.1.4) lib/action_controller/metal/instrumentation.rb:32:in `block in process_action'
  activesupport (5.1.4) lib/active_support/notifications.rb:166:in `block in instrument'
  activesupport (5.1.4) lib/active_support/notifications/instrumenter.rb:21:in `instrument'
  activesupport (5.1.4) lib/active_support/notifications.rb:166:in `instrument'
  actionpack (5.1.4) lib/action_controller/metal/instrumentation.rb:30:in `process_action'
  actionpack (5.1.4) lib/action_controller/metal/params_wrapper.rb:252:in `process_action'
  activerecord (5.1.4) lib/active_record/railties/controller_runtime.rb:22:in `process_action'
  actionpack (5.1.4) lib/abstract_controller/base.rb:124:in `process'
  actionview (5.1.4) lib/action_view/rendering.rb:30:in `process'
  rack-mini-profiler (0.10.5) lib/mini_profiler/profiling_methods.rb:102:in `block in profile_method'
  actionpack (5.1.4) lib/action_controller/metal.rb:189:in `dispatch'
  actionpack (5.1.4) lib/action_controller/metal.rb:253:in `dispatch'
  actionpack (5.1.4) lib/action_dispatch/routing/route_set.rb:49:in `dispatch'
  actionpack (5.1.4) lib/action_dispatch/routing/route_set.rb:31:in `serve'
  actionpack (5.1.4) lib/action_dispatch/journey/router.rb:50:in `block in serve'
  actionpack (5.1.4) lib/action_dispatch/journey/router.rb:33:in `serve'
  actionpack (5.1.4) lib/action_dispatch/routing/route_set.rb:834:in `call'
  railties (5.1.4) lib/rails/engine.rb:522:in `call'
  railties (5.1.4) lib/rails/railtie.rb:185:in `method_missing'
  actionpack (5.1.4) lib/action_dispatch/routing/mapper.rb:17:in `block in <class:Constraints>'
  actionpack (5.1.4) lib/action_dispatch/routing/mapper.rb:46:in `serve'
  actionpack (5.1.4) lib/action_dispatch/journey/router.rb:50:in `block in serve'
  actionpack (5.1.4) lib/action_dispatch/journey/router.rb:33:in `serve'
  actionpack (5.1.4) lib/action_dispatch/routing/route_set.rb:834:in `call'
  rack-protection (2.0.0) lib/rack/protection/frame_options.rb:31:in `call'
  omniauth (1.6.1) lib/omniauth/strategy.rb:189:in `call!'
  omniauth (1.6.1) lib/omniauth/strategy.rb:167:in `call'
  omniauth (1.6.1) lib/omniauth/strategy.rb:189:in `call!'
  omniauth (1.6.1) lib/omniauth/strategy.rb:167:in `call'
  omniauth (1.6.1) lib/omniauth/strategy.rb:189:in `call!'
  omniauth (1.6.1) lib/omniauth/strategy.rb:167:in `call'
  omniauth (1.6.1) lib/omniauth/strategy.rb:189:in `call!'
  omniauth (1.6.1) lib/omniauth/strategy.rb:167:in `call'
  omniauth (1.6.1) lib/omniauth/strategy.rb:189:in `call!'
  omniauth (1.6.1) lib/omniauth/strategy.rb:167:in `call'
  omniauth (1.6.1) lib/omniauth/strategy.rb:189:in `call!'
  omniauth (1.6.1) lib/omniauth/strategy.rb:167:in `call'
  omniauth (1.6.1) lib/omniauth/strategy.rb:189:in `call!'
  omniauth (1.6.1) lib/omniauth/strategy.rb:167:in `call'
  omniauth (1.6.1) lib/omniauth/builder.rb:63:in `call'
  rack (2.0.3) lib/rack/conditional_get.rb:38:in `call'
  rack (2.0.3) lib/rack/head.rb:12:in `call'
  rack (2.0.3) lib/rack/session/abstract/id.rb:232:in `context'
  rack (2.0.3) lib/rack/session/abstract/id.rb:226:in `call'
  actionpack (5.1.4) lib/action_dispatch/middleware/cookies.rb:613:in `call'
  activerecord (5.1.4) lib/active_record/migration.rb:556:in `call'
  actionpack (5.1.4) lib/action_dispatch/middleware/callbacks.rb:26:in `block in call'
  activesupport (5.1.4) lib/active_support/callbacks.rb:97:in `run_callbacks'
  actionpack (5.1.4) lib/action_dispatch/middleware/callbacks.rb:24:in `call'
  actionpack (5.1.4) lib/action_dispatch/middleware/executor.rb:12:in `call'
  better_errors (2.1.1) lib/better_errors/middleware.rb:84:in `protected_app_call'
  better_errors (2.1.1) lib/better_errors/middleware.rb:79:in `better_errors_call'
  better_errors (2.1.1) lib/better_errors/middleware.rb:57:in `call'
  actionpack (5.1.4) lib/action_dispatch/middleware/debug_exceptions.rb:59:in `call'
  actionpack (5.1.4) lib/action_dispatch/middleware/show_exceptions.rb:31:in `call'
  logster (1.2.8) lib/logster/middleware/reporter.rb:31:in `call'
  railties (5.1.4) lib/rails/rack/logger.rb:36:in `call_app'
  railties (5.1.4) lib/rails/rack/logger.rb:26:in `call'
  config/initializers/100-quiet_logger.rb:16:in `call'
  config/initializers/100-silence_logger.rb:29:in `call'
  actionpack (5.1.4) lib/action_dispatch/middleware/remote_ip.rb:79:in `call'
  actionpack (5.1.4) lib/action_dispatch/middleware/request_id.rb:25:in `call'
  rack (2.0.3) lib/rack/method_override.rb:22:in `call'
  rack (2.0.3) lib/rack/runtime.rb:22:in `call'
  actionpack (5.1.4) lib/action_dispatch/middleware/executor.rb:12:in `call'
  actionpack (5.1.4) lib/action_dispatch/middleware/static.rb:125:in `call'
  rack (2.0.3) lib/rack/sendfile.rb:111:in `call'
  lib/middleware/missing_avatars.rb:21:in `call'
  lib/middleware/turbo_dev.rb:34:in `call'
  rack-mini-profiler (0.10.5) lib/mini_profiler/profiler.rb:282:in `call'
  message_bus (2.0.8) lib/message_bus/rack/middleware.rb:63:in `call'
  railties (5.1.4) lib/rails/engine.rb:522:in `call'
  railties (5.1.4) lib/rails/railtie.rb:185:in `method_missing'
  rack (2.0.3) lib/rack/urlmap.rb:68:in `block in call'
  rack (2.0.3) lib/rack/urlmap.rb:53:in `call'
  puma (3.9.1) lib/puma/configuration.rb:224:in `call'
  puma (3.9.1) lib/puma/server.rb:602:in `handle_request'
  puma (3.9.1) lib/puma/server.rb:435:in `process_client'
  puma (3.9.1) lib/puma/server.rb:299:in `block in run'
  puma (3.9.1) lib/puma/thread_pool.rb:120:in `block in spawn_thread'

If it helps, I debugged one of the wayward subscription merges (these are all test keys and subscriptions, so nothing is sensitive here) from https://github.com/discourse/discourse-push-notifications/blob/master/services/discourse_push_notifications/pusher.rb#L59

user.custom_fields[DiscoursePushNotifications::PLUGIN_NAME] somehow gets set to:

[{"subscriptions"=>{"fkIj6s4VzTQ:APA91bF5J2hx5PvhVTI2tpFM5hGEi01EwB1plG6ZWkVL_zjFJSOkJv8EV4GUsb7pP5OLkCXmWjjzjVA78uDud6wKhW22HKTXL9uNtJznp9bMYI4KNoZDQ_m4TzS9MM8dJWShZDDi7sVH"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/fkIj6s4VzTQ:APA91bF5J2hx5PvhVTI2tpFM5hGEi01EwB1plG6ZWkVL_zjFJSOkJv8EV4GUsb7pP5OLkCXmWjjzjVA78uDud6wKhW22HKTXL9uNtJznp9bMYI4KNoZDQ_m4TzS9MM8dJWShZDDi7sVH\",\"keys\":{\"p256dh\":\"BPvhh2nnJqNec0Wr-yRzwbc_7HAETlxvTpb_VbwcVXvlFhmlTG1khsTEq16Ee1G5wYFeZj9tDznCAmJMd37Z6vg=\",\"auth\":\"oHWgCAV7sRb2BY3KZGy13Q==\"}}", "fHsgktbPGyY:APA91bGUF6QtO3GyzsjmMyMCRptgYBC6EJtGRAyKtxAgVZsb25wJPlTBLm7EirfcifX3HE3NbmDZ-NS8ouONYNRDLAYoYpvgQv9X0c8DyBTAjOI4lnQLLFB75U9pWnso0R_290aKYxPa"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/fHsgktbPGyY:APA91bGUF6QtO3GyzsjmMyMCRptgYBC6EJtGRAyKtxAgVZsb25wJPlTBLm7EirfcifX3HE3NbmDZ-NS8ouONYNRDLAYoYpvgQv9X0c8DyBTAjOI4lnQLLFB75U9pWnso0R_290aKYxPa\",\"keys\":{\"p256dh\":\"BIIgZFpAdmmU0xSLpNUdiHW8vUuK47HGwiJD3XunYYUPcFjQXfNlFQeTZ-Z5XNEGOR7b1LtoNQVFMg4daOSF6VU=\",\"auth\":\"6KohGqmRujYUU_hIk49ZGQ==\"}}", "dreWp4gWwHA:APA91bFWrOdbjEVmw5jJO4P7LUS56zozxXAfyD8iIjp_j9WUNu6-yro5y0cIRmJPBK2ow3yhGVdtb-SoVbvqIp04-aa3pzoyVe6jRY4v5O5RbPPGDXGK-GQHT0vx8PphHmt8571orr6F"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/dreWp4gWwHA:APA91bFWrOdbjEVmw5jJO4P7LUS56zozxXAfyD8iIjp_j9WUNu6-yro5y0cIRmJPBK2ow3yhGVdtb-SoVbvqIp04-aa3pzoyVe6jRY4v5O5RbPPGDXGK-GQHT0vx8PphHmt8571orr6F\",\"keys\":{\"p256dh\":\"BJF7-yoeeaPw8yWpruZuY-RfG4yfObhJ1-BQR0T_Kw5EiCX6HGshoVLR4X8XTvxdBLY-JbNik0rgSj7cOM8BU0o=\",\"auth\":\"6X0RDPKh-eWuXH5JJusnnQ==\"}}", "ef7TORwSxJo:APA91bFVB07CvkpKlFfnTbImywFRwi3GsskbEmqjwucycuF4QKOWURzXwmkmymj08spRp4uYQ5qRkaYkd0eKuuw2K2bZwfJw9kI-HCyc7bUUFgGtoNLNNNKuWsY_oALFUi9-VFnFTyoe"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/ef7TORwSxJo:APA91bFVB07CvkpKlFfnTbImywFRwi3GsskbEmqjwucycuF4QKOWURzXwmkmymj08spRp4uYQ5qRkaYkd0eKuuw2K2bZwfJw9kI-HCyc7bUUFgGtoNLNNNKuWsY_oALFUi9-VFnFTyoe\",\"keys\":{\"p256dh\":\"BNKTy11XuChzeXQ4KhjFYGNXf6GV_h1MaUQdZIdhZyOIxxFzA4qvbElV-lmrXUIHA-DFj2BQFh5HxEzAqCuFSos=\",\"auth\":\"DZ44uRI3_qE3YUsBwfk0mg==\"}}", "c_kgxZsGnhw:APA91bEXaMSDUX22Ce1_bM9zW_NmM9eBo_N4ex4NgXvw5AYUOBAyTzK_wii3JGk3Mq2KemxHhgIyLrG_2Te1RVEFLMqb56R7gKVWtR81Op8r8Bmgg8mkKyglRA-q07JsDDmh1IFcrNpe"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/c_kgxZsGnhw:APA91bEXaMSDUX22Ce1_bM9zW_NmM9eBo_N4ex4NgXvw5AYUOBAyTzK_wii3JGk3Mq2KemxHhgIyLrG_2Te1RVEFLMqb56R7gKVWtR81Op8r8Bmgg8mkKyglRA-q07JsDDmh1IFcrNpe\",\"keys\":{\"p256dh\":\"BAQPEETuf-L5_GSO9DR3Z2vpKfmmqdz19gJo6PfihnHbhRXZkqOphQ-ggRCSaKSdlcoQgGh0TspX3tHkSLk3Z-8=\",\"auth\":\"2vz_fxwKAieZeNi9m20ReA==\"}}"}}, {"subscriptions"=>{"fkIj6s4VzTQ:APA91bF5J2hx5PvhVTI2tpFM5hGEi01EwB1plG6ZWkVL_zjFJSOkJv8EV4GUsb7pP5OLkCXmWjjzjVA78uDud6wKhW22HKTXL9uNtJznp9bMYI4KNoZDQ_m4TzS9MM8dJWShZDDi7sVH"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/fkIj6s4VzTQ:APA91bF5J2hx5PvhVTI2tpFM5hGEi01EwB1plG6ZWkVL_zjFJSOkJv8EV4GUsb7pP5OLkCXmWjjzjVA78uDud6wKhW22HKTXL9uNtJznp9bMYI4KNoZDQ_m4TzS9MM8dJWShZDDi7sVH\",\"keys\":{\"p256dh\":\"BPvhh2nnJqNec0Wr-yRzwbc_7HAETlxvTpb_VbwcVXvlFhmlTG1khsTEq16Ee1G5wYFeZj9tDznCAmJMd37Z6vg=\",\"auth\":\"oHWgCAV7sRb2BY3KZGy13Q==\"}}", "fHsgktbPGyY:APA91bGUF6QtO3GyzsjmMyMCRptgYBC6EJtGRAyKtxAgVZsb25wJPlTBLm7EirfcifX3HE3NbmDZ-NS8ouONYNRDLAYoYpvgQv9X0c8DyBTAjOI4lnQLLFB75U9pWnso0R_290aKYxPa"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/fHsgktbPGyY:APA91bGUF6QtO3GyzsjmMyMCRptgYBC6EJtGRAyKtxAgVZsb25wJPlTBLm7EirfcifX3HE3NbmDZ-NS8ouONYNRDLAYoYpvgQv9X0c8DyBTAjOI4lnQLLFB75U9pWnso0R_290aKYxPa\",\"keys\":{\"p256dh\":\"BIIgZFpAdmmU0xSLpNUdiHW8vUuK47HGwiJD3XunYYUPcFjQXfNlFQeTZ-Z5XNEGOR7b1LtoNQVFMg4daOSF6VU=\",\"auth\":\"6KohGqmRujYUU_hIk49ZGQ==\"}}", "dreWp4gWwHA:APA91bFWrOdbjEVmw5jJO4P7LUS56zozxXAfyD8iIjp_j9WUNu6-yro5y0cIRmJPBK2ow3yhGVdtb-SoVbvqIp04-aa3pzoyVe6jRY4v5O5RbPPGDXGK-GQHT0vx8PphHmt8571orr6F"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/dreWp4gWwHA:APA91bFWrOdbjEVmw5jJO4P7LUS56zozxXAfyD8iIjp_j9WUNu6-yro5y0cIRmJPBK2ow3yhGVdtb-SoVbvqIp04-aa3pzoyVe6jRY4v5O5RbPPGDXGK-GQHT0vx8PphHmt8571orr6F\",\"keys\":{\"p256dh\":\"BJF7-yoeeaPw8yWpruZuY-RfG4yfObhJ1-BQR0T_Kw5EiCX6HGshoVLR4X8XTvxdBLY-JbNik0rgSj7cOM8BU0o=\",\"auth\":\"6X0RDPKh-eWuXH5JJusnnQ==\"}}", "ef7TORwSxJo:APA91bFVB07CvkpKlFfnTbImywFRwi3GsskbEmqjwucycuF4QKOWURzXwmkmymj08spRp4uYQ5qRkaYkd0eKuuw2K2bZwfJw9kI-HCyc7bUUFgGtoNLNNNKuWsY_oALFUi9-VFnFTyoe"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/ef7TORwSxJo:APA91bFVB07CvkpKlFfnTbImywFRwi3GsskbEmqjwucycuF4QKOWURzXwmkmymj08spRp4uYQ5qRkaYkd0eKuuw2K2bZwfJw9kI-HCyc7bUUFgGtoNLNNNKuWsY_oALFUi9-VFnFTyoe\",\"keys\":{\"p256dh\":\"BNKTy11XuChzeXQ4KhjFYGNXf6GV_h1MaUQdZIdhZyOIxxFzA4qvbElV-lmrXUIHA-DFj2BQFh5HxEzAqCuFSos=\",\"auth\":\"DZ44uRI3_qE3YUsBwfk0mg==\"}}", "c_kgxZsGnhw:APA91bEXaMSDUX22Ce1_bM9zW_NmM9eBo_N4ex4NgXvw5AYUOBAyTzK_wii3JGk3Mq2KemxHhgIyLrG_2Te1RVEFLMqb56R7gKVWtR81Op8r8Bmgg8mkKyglRA-q07JsDDmh1IFcrNpe"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/c_kgxZsGnhw:APA91bEXaMSDUX22Ce1_bM9zW_NmM9eBo_N4ex4NgXvw5AYUOBAyTzK_wii3JGk3Mq2KemxHhgIyLrG_2Te1RVEFLMqb56R7gKVWtR81Op8r8Bmgg8mkKyglRA-q07JsDDmh1IFcrNpe\",\"keys\":{\"p256dh\":\"BAQPEETuf-L5_GSO9DR3Z2vpKfmmqdz19gJo6PfihnHbhRXZkqOphQ-ggRCSaKSdlcoQgGh0TspX3tHkSLk3Z-8=\",\"auth\":\"2vz_fxwKAieZeNi9m20ReA==\"}}", "eQKp7ZbhMQY:APA91bEFLtSn0p_T9FvHKIC46MIcYki2danWCBgkQdVMCGOol8NOcXHkFLzrUw--OFd-gyBX-JqRdNZ0lvNTyVOJoKoz7WS5Z08WdD-INRuR9Frzg_1O-wIE8GAnWr6ke72GqdamvVnE"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/eQKp7ZbhMQY:APA91bEFLtSn0p_T9FvHKIC46MIcYki2danWCBgkQdVMCGOol8NOcXHkFLzrUw--OFd-gyBX-JqRdNZ0lvNTyVOJoKoz7WS5Z08WdD-INRuR9Frzg_1O-wIE8GAnWr6ke72GqdamvVnE\",\"keys\":{\"p256dh\":\"BMiKwrQbBna3LTcDgM0Um3660DwUE8iv63jSkF3dke3k810pKCTfZhjxsKbJZldReVF8HVKo7D5usFbX3hcIFDs=\",\"auth\":\"SxFOhWIbI6E6-h2fwD9PGA==\"}}", "eRztrel4SD4:APA91bEErWGkRWvF0vdy2sBrwa_bmlmuLYEDSGfsopsB_nVfZjRGpIALtumJRYAVs3sjhfJH_Av-h3v-cOLtgzgQ2QV73c6w9bbKeDETZl6fuCJ8iRTDAnAyAFkcbs_KLvIoJwD_p_pC"=>"{\"endpoint\":\"https://fcm.googleapis.com/fcm/send/eRztrel4SD4:APA91bEErWGkRWvF0vdy2sBrwa_bmlmuLYEDSGfsopsB_nVfZjRGpIALtumJRYAVs3sjhfJH_Av-h3v-cOLtgzgQ2QV73c6w9bbKeDETZl6fuCJ8iRTDAnAyAFkcbs_KLvIoJwD_p_pC\",\"keys\":{\"p256dh\":\"BPV0MuqC7tN78Z1DNnNCJMeDc2y5Q9nGGSsPatJhkgxxU27vhZ-1AUHgrXPGZEV0FNIMUBiTkSZ2hbxoZpWJwvQ=\",\"auth\":\"MzlCoHg56P5qxN-I63KJgA==\"}}"}}]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tldr, I think it could possibly be an error in core where custom fields don't deep merge correctly?

merged_subscriptions = {}
user.custom_fields[DiscoursePushNotifications::PLUGIN_NAME].each do |subscription|
merged_subscriptions = merged_subscriptions.merge(subscription[SUBSCRIPTION_KEY])
end
user.custom_fields[DiscoursePushNotifications::PLUGIN_NAME] = merged_subscriptions
end
user.custom_fields[DiscoursePushNotifications::PLUGIN_NAME][SUBSCRIPTION_KEY] ||= {}
user.custom_fields[DiscoursePushNotifications::PLUGIN_NAME][SUBSCRIPTION_KEY]
end
Expand Down