rom icon indicating copy to clipboard operation
rom copied to clipboard

Conflict with notifications relation

Open alexandru-calinoiu opened this issue 5 years ago • 9 comments

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

  • Affects my production application: yes
  • Ruby version: 2.6.2
  • OS: Linux

alexandru-calinoiu avatar May 23 '19 18:05 alexandru-calinoiu

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?

alexandru-calinoiu avatar May 23 '19 18:05 alexandru-calinoiu

Good catch!

v-kolesnikov avatar May 23 '19 20:05 v-kolesnikov

This can be easily fixed by using Relation#__notifications__ instead

solnic avatar May 24 '19 11:05 solnic

Hey y'all

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

gbrlcustodio avatar Dec 19 '19 14:12 gbrlcustodio

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

solnic avatar Dec 19 '19 16:12 solnic

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

mrship avatar Jun 08 '21 10:06 mrship

@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

solnic avatar Jun 09 '21 07:06 solnic

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

FLemon avatar Jul 27 '22 10:07 FLemon

Yes I am

solnic avatar Jul 29 '22 06:07 solnic