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

Conflict with notifications relation #636

Open
alexandru-calinoiu opened this issue May 23, 2019 · 9 comments
Open

Conflict with notifications relation #636

alexandru-calinoiu opened this issue May 23, 2019 · 9 comments
Milestone

Comments

@alexandru-calinoiu
Copy link

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
Copy link
Author

alexandru-calinoiu commented May 23, 2019

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
Copy link
Collaborator

Good catch!

@solnic
Copy link
Member

solnic commented May 24, 2019

This can be easily fixed by using Relation#__notifications__ instead

@gbrlcustodio
Copy link

gbrlcustodio commented Dec 19, 2019

Hey y'all

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

@solnic
Copy link
Member

solnic commented Dec 19, 2019

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

@mrship
Copy link

mrship commented Jun 8, 2021

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

@solnic solnic transferred this issue from rom-rb/rom-sql Jun 9, 2021
@solnic
Copy link
Member

solnic commented Jun 9, 2021

@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 solnic added this to the 6.0.0 milestone Jun 9, 2021
@FLemon
Copy link

FLemon commented Jul 27, 2022

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

@solnic
Copy link
Member

solnic commented Jul 29, 2022

Yes I am

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

No branches or pull requests

6 participants