Extensible DeviceFactory

[GoogleCodeExporter]

It should be possible to design our own DeviceFactories (powered by JPA for 
example) by making the following small modifications in your code (there are 
other ways, but this one involves less code modifications and no design 
changes):

1) Add a static setInstance(DeviceFactory) method to the DeviceFactory class, 
to allow us to plug our own implementation of DeviceFactory.

2) Make the DeviceFactory constructor public.  There is no reason here to make 
it private, and this is preventing us from extending the class.  The same 
comment goes for the PushNotificationManager, whose private constructor is 
preventing us from extending the class.

With these two small changes, we'll be able to build our own custom 
DeviceFactory, register it by calling DeviceFactory.setInstance(myFactory), and 
automatically get our custom-stored devices to the PushNotificationProvider.

Thank you very much for this very helpful library!

SP

Original issue reported on code.google.com by sype...@gmail.com on 19 Oct 2010 at 8:23

[GoogleCodeExporter]

Making the DeviceFactory & PushNotifcationManager constructor public will break 
the singleton pattern.

As per DeviceFactory notes:
 * It implements the singleton pattern so only one class manages the devices,
 * avoiding problems with duplicate or lost devices

I can understand the desire to add JPA support, but it seems like making the 
PushNotifcationManager an abstract class would be more in line with the end 
goal. This will have to be investigated.

Original comment by idbill.p...@gmail.com on 20 Oct 2010 at 6:15

• Changed state: Accepted

[GoogleCodeExporter]

Thanks for reviewing this issue!  I'm not sure I understand though why the 
singleton pattern is so important to preserve here, as this is an anti-pattern 
in regards to dependency injection and makes unit testing far more difficult 
down the road - and is directly preventing users of the library from extending 
the DeviceFactory to provide their own implementations of Device.

Since PushNotificationManager always invokes DeviceFactory.getInstance(), I 
would strongly suggest removing the strict singleton requirement and allow 
users of the library to subclass DeviceFactory and statically set the actual 
instance that is used with DeviceFactory.setInstance(customFactory).  
PushNotificationManager would still always rely on a single instance of 
DeviceFactory (because they call the static getInstance method), alleviating 
the concerns about duplicate or lost devices.

The other way to go would be, as you suggested, to allow the 
PushNotificationManager class to be subclassed.  Again, I'm not sure I 
understand why the singleton pattern has any usefulness for this class (it 
seems to be causing more issues than it is solving, particularly for IoC and 
testing) . The PushNotificationManager would not need to be made abstract 
either, as a subclass could simply override your default implementations of 
addDevice, getDevice and removeDevice methods without it being abstract.  Plus 
if you don't make it abstract but make it subclassable, the library will be 
easier to use out-of-the-box for first-time users, while providing much more 
flexibility for users that cannot rely on a simple memory-mapped and volatile 
list of devices.

Thank you very much for all the efforts you put into this library!

Original comment by sype...@gmail.com on 5 Nov 2010 at 10:15

[GoogleCodeExporter]

> I'm not sure I understand though why the singleton pattern is so important
I don't really understand either, but I didn't write the code either. But there 
is a comment that says:
 * It implements the singleton pattern so only one class manages the devices,
 * avoiding problems with duplicate or lost devices

so, maybe this was made when the PushNotificationManager was not a singleton... 
dunno. 

A year or so ago, the owners of this project were gracious enough to add me to 
the project. I have been trying to fix bugs and add documentation to help 
others, but I have limited experience with Java. I also have limited time atm.

I talked it over with a co-worker and we think you have some good ideas. We 
agree with removing the singleton requirement on the DeviceFactory and making 
it into an abstract class so that it can be overridden.

If you would like to submit code or even get added to the project, then these 
changes could be expedited. 
I would very much welcome the assistance!

Original comment by idbill.p...@gmail.com on 10 Nov 2010 at 5:59

[GoogleCodeExporter]

Hi!  I'd be happy to help with the evolution of this code, as it will be very 
useful within the larger project I work with. So yes, you are welcome to add me 
to the project.  Thank you!

Original comment by sype...@gmail.com on 15 Nov 2010 at 7:58

[GoogleCodeExporter]

We have begun working on enhancing the library to make it usable in a 
dependency-injected and JPA-persisted context.  The required changes involved 
many classes, whose documentation and test cases would need to be updated 
accordingly.  Thus to avoid modifying the trunk with a lot of sudden 
behavior-impacting changes, we've branched the project and committed all 
modifications to that branch instead.  

We believe that the ideal solution to support custom DeviceFactories was to go 
with interfaces.  Going with interfaces instead of abstract classes makes the 
library even easier to use, because it doesn't require users to extend a class 
(which might not be possible in many situations).

Further more, the current project structure probably made sense for the 
originally intended use of the library;  but in a dependency-injected and 
JPA-persisted project, a significant refactoring effort helped make clear 
separations of roles and responsibilities of the various parts of the library.  
Again, this has been committed to the new branch only, for review purposes, so 
not to cause panic on the main source tree :)

Original comment by sype...@gmail.com on 16 Nov 2010 at 2:22

[GoogleCodeExporter]

How is the enhancement going?

Original comment by idbill.p...@gmail.com on 11 Dec 2010 at 7:09

[GoogleCodeExporter]

Hi, other parts of our project required our attention for the past few weeks, 
but we'll be getting back to Javapns integration very shortly.  I'll keep you 
informed of our progress :)  Thank you!

Original comment by sype...@gmail.com on 10 Jan 2011 at 9:54

[GoogleCodeExporter]

Have you seen:
https://github.com/notnoop/java-apns/wiki

Looks like they have some of the features you want.

Original comment by idbill.p...@gmail.com on 26 Apr 2011 at 3:44

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Made a major commit to the 1.7 branch today (see r160 and r161).  As it is now, 
this release pushes notifications perfectly for us, and can be hooked up 
successfully with JPA (although this is still optional).  It also improves 
error reporting, so it is easier to understand what is going wrong in some 
situations (such as bad keystore password, invalid certificate chain, etc.).  
This release also fixes a critical bug in the PushNotificationManager class 
which also affects the trunk (pushing empty payloads instead of the supplied 
ones).  The Feedback Service is also working and deleting devices in the 
database as needed.  All recent changes to the trunk have also been imported 
into this branch.

Original comment by sype...@gmail.com on 31 Aug 2011 at 2:02

• Changed state: Fixed