[BUG] Noticed seems incompatible with MS SQL server (undefined method `has_key?' for an instance of String) #450
Comments
|
We only test against Postgres, MySQL, and SQLite. Looks like you have a text column for |
|
Yeah, internally the I can actually "fix" this by creating a notifier like this: class GenericNotifier < Noticed::Event
attribute :params, ActiveRecord::Type::SQLServer::Json.new
required_params "title", "message"
end |
|
Good to know! One of the troubles we had in the past was trying to detect the database format at runtime causing issues so I'm not sure what's best here. We can definitely document it in the README, but we could possibly add it to the generator as well. |
|
I am also not sure what's best to do here. I fixed this by defining my ApplicationNotifier like so: class ApplicationNotifier < Noticed::Event
attribute :params, ActiveRecord::Type::SQLServer::Json.new
def self.required_params(*args)
super(*args.map(&:to_s))
end
endThat way I can still define my notifiers like normal: class GenericNotifier < ApplicationNotifier
required_params :title, :message
endMaybe just a note in the docs is enough for this? I can live with this solution for my project at least. |
|
If you open the Rails console, does We might be able to inject that line into ApplicationNotifier in the generators easy enough. This was added in Rails 6.1 and we require 6.1+ for Noticed, so we should be able to use that during generators. |
|
Yeah, it does. That could work actually. 3.3.1 :001 > ActiveRecord::Base.connection_db_config.adapter
=> "sqlserver"If you want to support also the singular class ApplicationNotifier < Noticed::Event
attribute :params, ActiveRecord::Type::SQLServer::Json.new
class << self
def required_params(*args)
super(*args.map(&:to_s))
end
alias_method :required_param, :required_params
end
end |
|
This can be simplified with which preserves the symbols and other objects in the JSON. attribute :params, ActiveRecord::Type::SQLServer::Json.new
serialize :params, coder: Noticed::Coder |
|
Just tried it, works great 😄 |
|
Looking at the code, |
|
That didn't work. Guess I'll just leave it alone. 😜 |
|
Was that by changing the My change is making the type of the params attribute JSON |
This injects SQLServer configuration into ApplicationNotifier as needed when sqlserver adapter is detected. Fixes #450
|
Oh thanks! That's exactly what I did wrong haha |
|
Np :) The following works on my end: class ApplicationNotifier < Noticed::Event
attribute :params, ActiveRecord::Type::Json.new, default: {}
serialize :params, coder: Noticed::Coder
end |
|
That worked, but does break a test with the Ephemeral notifier which uses ActiveModel. |
|
Ah ok, nevermind then. Worth a shot 😄 |
|
Any idea why it would put the 
|
|
That's the thing we using ActiveJob::Arguments to retain symbols, classes, objects with GlobalID, etc. |
|
Try out #451. Setting the type seems to be the missing piece so Rails handles all that nicely. |
Ok, so this is expected?
Doing it right now 😃 |
|
Can confirm it works great! 😃 Thanks for fixing this! 🚀 |
|
You're welcome and thanks for the help on this! I think this'll fix other databases and be more compatible all around. 👍 |
By specifying the type for the params column, we can make sure ActiveRecord knows how to handle this for more databases like sqlserver. Fixes #450
Bug Report
Describe the Bug:
When using Noticed with the MS SQL server adapter, it seems that the params get serialised as a string and Noticed isn't expecting that. This has something to do with ActiveJob it seems, but I'm not 100% sure. When I try to deliver a notification I get the error:
To Reproduce:
tiny_tdsandactiverecord-sqlserver-adaptergems (and noticed of course).GenericNotifier.with(title: "Hello", message: "World").deliver(User.all)Repo that demonstrates the problem: https://github.com/erlingur/noticed-bug
You will need to launch a MS SQL container with Docker like so:
If you have an issue installing
tiny_tdsyou might have to install it like so:gem install tiny_tds -v '2.1.5' -- --with-freetds-include=/opt/homebrew/include --with-freetds-lib=/opt/homebrew/lib(This is what I have to do in my setup at least)
Expected Behavior:
I expect the notification to be delivered to all users
Actual Behavior:
I get an error:
Environment:
Checklist:
The text was updated successfully, but these errors were encountered: