Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New notifications params are suddenly encoded as string in postgres #291

Closed
tranxuanthang opened this issue May 28, 2023 · 5 comments · Fixed by #313
Closed

New notifications params are suddenly encoded as string in postgres #291

tranxuanthang opened this issue May 28, 2023 · 5 comments · Fixed by #313

Comments

@tranxuanthang
Copy link

tranxuanthang commented May 28, 2023

I've been using noticed for a long time without any issue. But after recovering from a full disk space incident of my server (No space left on device), I found some users reporting 500 status errors that are related to notification. After investigating, I found some faulty notifications that are causing the problem. An example is this notification ID:

irb(main):003:0> Notification.find(2118871).params[:manga_id]
Traceback (most recent call last):
        2: from (irb):3
        1: from (irb):3:in `[]'
TypeError (no implicit conversion of Symbol into Integer)

After checking that notification ID further, it seems that the params of that notification is in text string instead of hash, makes it impossible to call notification.params[:manga_id]:

db_production=# SELECT * FROM notifications WHERE id = 2118871;
   id    | recipient_type | recipient_id |          type          |                                                                                                                                                                                                                                                                                                   params                                                                                                                                                                                                                                                                                                   | read_at |         created_at         |         updated_at
---------+----------------+--------------+------------------------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+---------+----------------------------+----------------------------
 2118871 | User           |        22556 | NewChapterNotification | "{\"chapter_id\":19553,\"chapter_number\":\"33\",\"chapter_name\":\"[REDACTED]\",\"manga_id\":885,\"manga_title\":\"[REDACTED] \",\"manga_cover_url\":\"[REDACTED]\",\"manga_panorama_url\":\"[REDACTED]\",\"_aj_symbol_keys\":[\"chapter_id\",\"chapter_number\",\"chapter_name\",\"manga_id\",\"manga_title\",\"manga_cover_url\",\"manga_panorama_url\"]}" |         | 2023-05-26 13:03:40.764933 | 2023-05-26 13:03:40.764933
(1 row)
irb(main):022:0> Notification.find(2118871).params
=> "{\"chapter_id\":19553,\"chapter_number\":\"33\",\"chapter_name\":\"[REDACTED]\",\"manga_id\":885,\"manga_title\":\"[REDACTED] \",\"manga_cover_url\":\"[REDACTED]\",\"manga_panorama_url\":\"[REDACTED]\",\"_aj_symbol_keys\":[\"chapter_id\",\"chapter_number\",\"chapter_name\",\"manga_id\",\"manga_title\",\"manga_cover_url\",\"manga_panorama_url\"]}"

Most of the notifications before still rendered correctly:

db_production=# SELECT * FROM notifications WHERE id = 2119104;
   id    | recipient_type | recipient_id |          type          |                                                                                                                                                                                                                                                                                     params                                                                                                                                                                                                                                                                                     | read_at |        created_at        |        updated_at
---------+----------------+--------------+------------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+---------+--------------------------+--------------------------
 2119104 | User           |        22556 | NewChapterNotification | {"manga_id": 1112, "chapter_id": 19562, "manga_title": "[REDACTED]", "chapter_name": "[REDACTED]", "chapter_number": "42", "_aj_symbol_keys": ["chapter_id", "chapter_number", "chapter_name", "manga_id", "manga_title", "manga_cover_url", "manga_panorama_url"], "manga_cover_url": "[REDACTED]", "manga_panorama_url": "[REDACTED]"} |         | 2023-05-26 13:50:59.0579 | 2023-05-26 13:50:59.0579
(1 row)
irb(main):023:0> Notification.find(2119104).params
=> {:manga_id=>1112, :chapter_id=>19562, :manga_title=>"[REDACTED]", :chapter_name=>"[REDACTED]", :chapter_number=>"42", :manga_cover_url=>"[REDACTED]", :manga_panorama_url=>"[REDACTED]"}

The params column in the notifications table has jsonb type:

db_production=# \d notifications;
                                            Table "public.notifications"
     Column     |              Type              | Collation | Nullable |                  Default
----------------+--------------------------------+-----------+----------+-------------------------------------------
 id             | bigint                         |           | not null | nextval('notifications_id_seq'::regclass)
 recipient_type | character varying              |           | not null |
 recipient_id   | bigint                         |           | not null |
 type           | character varying              |           | not null |
 params         | jsonb                          |           |          |
 read_at        | timestamp without time zone    |           |          |
 created_at     | timestamp(6) without time zone |           | not null |
 updated_at     | timestamp(6) without time zone |           | not null |
Indexes:
    "notifications_pkey" PRIMARY KEY, btree (id)
    "index_notifications_on_read_at" btree (read_at)
    "index_notifications_on_recipient" btree (recipient_type, recipient_id)

Expected: Notification's params can be accessed as hash (notification.params[:manga_id]).
Actual: Notification's params cannot be accessed as hash

Does anyone have any idea what can cause this? Could this be postgresql's fault?

@jpawlyn
Copy link

jpawlyn commented Aug 29, 2023

Hi. We recently came across the same issue described above. For us it looks like it's caused by:

rescue *DATABASE_ERROR_CLASS_NAMES => _error
warn("Noticed was unable to bootstrap correctly as the database is unavailable.")
Noticed::TextCoder
end

We upgraded our db recently and this caused the above warning to be logged. Our params column is jsonb and would normally be serialized/deserialized with Noticed::Coder. But when no db connection can be established it looks like the serializer defaults to Noticed::TextCoder.

The problem fixes itself I guess when the class is re-loaded (ie on deploy) but the existing params values remain as strings.

A simple and possibly naive fix for this would be to run:

Notification.find_each.select { |no| no.params.is_a?(String) }.each do |no| 
  no.update!(params: ActiveJob::Arguments.send(:deserialize_argument, JSON.parse(no.params)))
end

@excid3
Copy link
Owner

excid3 commented Aug 29, 2023

That makes sense. Maybe caching that value would be a solution here.

Either that, or we could specify the coder in a config so it's always set to the same value and won't accidentally change.

@jpawlyn
Copy link

jpawlyn commented Aug 29, 2023

Sounds good or another possibility, slightly hacky perhaps, is overriding serialize in the application code:

  include Noticed::Model
  serialize :params, Noticed::Coder

@tranxuanthang
Copy link
Author

@jpawlyn Thank you for your sharing! I had not known about serialize, but back then I did something similar to temporarily patch this:

class Notification < ApplicationRecord
  include Noticed::Model
  belongs_to :recipient, polymorphic: true

  def params
    return self[:params] if self[:params].class == Hash
    JSON.parse(self[:params]).transform_keys(&:to_sym)
  end
end

@scottbarrow
Copy link

Did anyone resolve this with a long term fix or discover the root cause?
Thanks for the patch suggestions

@excid3 excid3 linked a pull request Jan 15, 2024 that will close this issue
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants