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

Single Table Inheritance apparently not working properly with Redis store #147

Closed
haslo opened this issue Apr 15, 2015 · 8 comments
Closed

Comments

@haslo
Copy link

haslo commented Apr 15, 2015

We have a similar error to #136 (and maybe #127). We're not using ActiveRecord but rather Redis for storage. The bug was not reproducible with ActiveRecord storage in sqlite3 when I tried that.

  • There's the to_binary bug from Rpush selectively sends notifications. Why? #136 showing up for some of our notifications to Gcm. I found that it happens only when I actually have an APNs app in the storage - trying Rpush.push will result in the APNs path being executed and to_binary being called.
  • Similarly, there was trouble when I tried to do Rpush.apns_feedback with any apps that aren't APNs apps in the storage. They'll be iterated through and will have problems with environment being nil.

I think that both bugs come down to this reproducible test case (I've made a new Rails app to reproduce it, with minimal gems):

https://github.com/haslo/rpush_redis_bug

Essentially, every app is in the list of all apps (including all GCM apps being in the list of APNs apps and vice versa), and every notification is in the list of all notifications (thus iterating over all GCM notifications will return plenty of APNs notifications etc.).

@antulik
Copy link

antulik commented Jul 21, 2015

@haslo any work arounds until this is fixed?

@ileitch
Copy link
Member

ileitch commented Jul 21, 2015

I have a fix for this here: rpush/modis@3d7f0b3

However, it changes the internal structure Rpush uses to support STI, so I still need to add support for migrations to Modis..... (I'm going to hell for this! 😢 😭)

@haslo
Copy link
Author

haslo commented Jul 21, 2015

@antulik I made rpush/modis#2 as a somewhat hacky workaround. It doesn't change the storage structure, but has a bit of a performance impact, particularly with wide STI trees.

@antulik
Copy link

antulik commented Jul 22, 2015

@haslo thx, will give it a shot 👍

@avolkovi
Copy link

avolkovi commented Feb 7, 2017

any updates on this? It's been a year and a half and the patch in modis still isn't released... we ended up working around this by patching Rpush.apns_feedback to use Rpush::Apns::App.all.select { |app| app.is_a?(Rpush::Client::Redis::Apns::App) } instead of Rpush::Apns::App.all

@antulik
Copy link

antulik commented Feb 7, 2017

@avolkovi it's been a while, seems like i've given apps prefixed names like "ios_app" or "android_app" and search only by name instead of a class.

@haslo
Copy link
Author

haslo commented Feb 9, 2017

Should be fixed now, with rpush/modis@3d7f0b3 - I did close my PR, but not the issue, and nobody else did either...

@ileitch, you wanna close this issue?

@aried3r
Copy link
Member

aried3r commented Feb 22, 2018

Seems this was resolved, closing.

@aried3r aried3r closed this as completed Feb 22, 2018
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.

5 participants