search

rom-rb / rom

Data mapping and persistence toolkit for Ruby
https://rom-rb.org
MIT License
2.07k stars 161 forks source link

Conflict with notifications relation #636

Open alexandru-calinoiu opened 5 years ago

alexandru-calinoiu commented 5 years ago

Describe the bug

I have an notifications table in my database, after adding a Notifications relation I started receiving very strange errors.

Upon investigation it turns out that instrumentation will try to log to my relation instead of Dry::Monitor::Notifications

To Reproduce

Create a project, add instrumentation to your rom-configuration

    rom_config.plugin :sql, relations: :instrumentation do |plugin_config|
      plugin_config.notifications = notifications
    end

Create a relation / table named notifications

Expected behavior

Should work

Your environment

alexandru-calinoiu commented 5 years ago

It makes sens since here

https://github.com/rom-rb/rom-sql/blob/46d8d115c326afa8ac3a713b220e292d29615e0b/lib/rom/plugins/relation/sql/instrumentation.rb#L33

It's trying to access notifications attribute on a relation, and it probably get's overwritten by the notifications relation instead of using the config one.

I wonder why is this the case, can relations have different notification plugins apart from the one set in config?

v-kolesnikov commented 5 years ago

Good catch!

solnic commented 5 years ago

This can be easily fixed by using Relation#__notifications__ instead

gbrlcustodio commented 4 years ago

Hey y'all

I didn't get it. Is there a way to get rid of this by only changing my code structure?

solnic commented 4 years ago

@gbrlcustodio you can register your notifications relation under a different name for the time being

mrship commented 3 years ago

I got bitten by this today; would be nice not to have to rename my relation.

solnic commented 3 years ago

@mrship I'll make sure this is fixed in rom 6.0.0. This is now in rom-rb/rom because it needs to be fixed here and then a follow-up adjustments must be made in rom-sql

FLemon commented 1 year ago

Hi, @solnic are you still planning to fix this issue in rom 6?

solnic commented 1 year ago

Yes I am