[aerogear-dev] UPS bug - am I crazy?

classic Classic list List threaded Threaded
8 messages Options
qmx
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[aerogear-dev] UPS bug - am I crazy?

qmx
Administrator
Howdy!

I was doing the migration work and found out something funny:

For Installations, we have `installation.setDeviceType()`, but during
Installation `persist()`, the DeviceTokenValidator is called, which in
turn looks for a Variant.

So after fiddling with some orm settings suddenly a bunch of tests
started to break, which led me to this snippet[1]:

// disabled
Installation android3 = new Installation();
android3.setAlias("[hidden email]");
android3.setDeviceToken("543234234890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890");
android3.setDeviceType("Android Tablet");
android3.setEnabled(false);

installationDao.create(android3);

See, our tests are creating an installation without associating it to a
variant first, then it blows up here[2]:

// DeviceTokenValidator#isValid()
if (installation.getVariant() == null || installation.getVariant().getType() == null || deviceToken == null) {
    return false;
}

This is smelling like a bug to me - kinda like the validator wasn't
actually running during tests. Am I crazy?

[1]:https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/jpa/src/test/java/org/jboss/aerogear/unifiedpush/jpa/InstallationDaoTest.java#L133-L140
[2]:https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/api/src/main/java/org/jboss/aerogear/unifiedpush/api/validation/DeviceTokenValidator.java#L60

--
qmx
_______________________________________________
aerogear-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/aerogear-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [aerogear-dev] UPS bug - am I crazy?

Erik Jan de Wit
Hi,

Don’t know if it’s really a bug, but in order to validate that a device token is valid we need to know what variant type it belongs to. It makes no sense for a installation to be persisted without a Variant, because then you cannot send any messages to that device. So if you try to persist a installation without a variant the device token is not valid seems to be to be a valid statement.

This was introduced when we added cordova, there it’s easy to make a mistake and have the wrong variantID / secret in your settings. Resulting in a iOS device registering under an android variant. With the deviceToken validation this can no longer happen.

Does this make sense?

Cheers,
        Erik Jan

On 13 Feb,2015, at 2:02 , Douglas Campos <[hidden email]> wrote:

> Howdy!
>
> I was doing the migration work and found out something funny:
>
> For Installations, we have `installation.setDeviceType()`, but during
> Installation `persist()`, the DeviceTokenValidator is called, which in
> turn looks for a Variant.
>
> So after fiddling with some orm settings suddenly a bunch of tests
> started to break, which led me to this snippet[1]:
>
> // disabled
> Installation android3 = new Installation();
> android3.setAlias("[hidden email]");
> android3.setDeviceToken("543234234890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890");
> android3.setDeviceType("Android Tablet");
> android3.setEnabled(false);
>
> installationDao.create(android3);
>
> See, our tests are creating an installation without associating it to a
> variant first, then it blows up here[2]:
>
> // DeviceTokenValidator#isValid()
> if (installation.getVariant() == null || installation.getVariant().getType() == null || deviceToken == null) {
>    return false;
> }
>
> This is smelling like a bug to me - kinda like the validator wasn't
> actually running during tests. Am I crazy?
>
> [1]:https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/jpa/src/test/java/org/jboss/aerogear/unifiedpush/jpa/InstallationDaoTest.java#L133-L140
> [2]:https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/api/src/main/java/org/jboss/aerogear/unifiedpush/api/validation/DeviceTokenValidator.java#L60
>
> --
> qmx
> _______________________________________________
> aerogear-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/aerogear-dev


_______________________________________________
aerogear-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/aerogear-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [aerogear-dev] UPS bug - am I crazy?

Matthias Wessendorf
Hi,

I agree w/ Erik - however, I think the test has a bug, if the installations are not directly associated with a Variant. Should be easy to fix.

BTW. qmx, you mentioned the "setDeviceType()", but that is really just plain text metadata, like here:

The validator is triggered by the actual variant type (installation.getVariant().getType())

-Matthias



On Fri, Feb 13, 2015 at 8:17 AM, Erik Jan de Wit <[hidden email]> wrote:
Hi,

Don’t know if it’s really a bug, but in order to validate that a device token is valid we need to know what variant type it belongs to. It makes no sense for a installation to be persisted without a Variant, because then you cannot send any messages to that device. So if you try to persist a installation without a variant the device token is not valid seems to be to be a valid statement.

This was introduced when we added cordova, there it’s easy to make a mistake and have the wrong variantID / secret in your settings. Resulting in a iOS device registering under an android variant. With the deviceToken validation this can no longer happen.

Does this make sense?

Cheers,
        Erik Jan

On 13 Feb,2015, at 2:02 , Douglas Campos <[hidden email]> wrote:

> Howdy!
>
> I was doing the migration work and found out something funny:
>
> For Installations, we have `installation.setDeviceType()`, but during
> Installation `persist()`, the DeviceTokenValidator is called, which in
> turn looks for a Variant.
>
> So after fiddling with some orm settings suddenly a bunch of tests
> started to break, which led me to this snippet[1]:
>
> // disabled
> Installation android3 = new Installation();
> android3.setAlias("[hidden email]");
> android3.setDeviceToken("543234234890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890");
> android3.setDeviceType("Android Tablet");
> android3.setEnabled(false);
>
> installationDao.create(android3);
>
> See, our tests are creating an installation without associating it to a
> variant first, then it blows up here[2]:
>
> // DeviceTokenValidator#isValid()
> if (installation.getVariant() == null || installation.getVariant().getType() == null || deviceToken == null) {
>    return false;
> }
>
> This is smelling like a bug to me - kinda like the validator wasn't
> actually running during tests. Am I crazy?
>
> [1]:https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/jpa/src/test/java/org/jboss/aerogear/unifiedpush/jpa/InstallationDaoTest.java#L133-L140
> [2]:https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/api/src/main/java/org/jboss/aerogear/unifiedpush/api/validation/DeviceTokenValidator.java#L60
>
> --
> qmx
> _______________________________________________
> aerogear-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/aerogear-dev


_______________________________________________
aerogear-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/aerogear-dev



--

_______________________________________________
aerogear-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/aerogear-dev
qmx
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [aerogear-dev] UPS bug - am I crazy?

qmx
Administrator
On Fri, Feb 13, 2015 at 12:01:07PM +0100, Matthias Wessendorf wrote:
> Hi,
>
> I agree w/ Erik - however, I think the test has a bug, if the installations
> are not directly associated with a Variant. Should be easy to fix.
That's my point - this test should never pass - hence the "bug"
suspicion.

>
> BTW. qmx, you mentioned the "setDeviceType()", but that is really just
> plain text metadata, like here:
> https://github.com/aerogear/aerogear-push-helloworld/blob/master/ios/HelloWorld/AGAppDelegate.m#L106
>
>
> The validator is triggered by the actual variant type (installation
> .getVariant().getType())
>
> -Matthias
>
>
>
> On Fri, Feb 13, 2015 at 8:17 AM, Erik Jan de Wit <[hidden email]> wrote:
>
> > Hi,
> >
> > Don’t know if it’s really a bug, but in order to validate that a device
> > token is valid we need to know what variant type it belongs to. It makes no
> > sense for a installation to be persisted without a Variant, because then
> > you cannot send any messages to that device. So if you try to persist a
> > installation without a variant the device token is not valid seems to be to
> > be a valid statement.
> >
> > This was introduced when we added cordova, there it’s easy to make a
> > mistake and have the wrong variantID / secret in your settings. Resulting
> > in a iOS device registering under an android variant. With the deviceToken
> > validation this can no longer happen.
> >
> > Does this make sense?
> >
> > Cheers,
> >         Erik Jan
> >
> > On 13 Feb,2015, at 2:02 , Douglas Campos <[hidden email]> wrote:
> >
> > > Howdy!
> > >
> > > I was doing the migration work and found out something funny:
> > >
> > > For Installations, we have `installation.setDeviceType()`, but during
> > > Installation `persist()`, the DeviceTokenValidator is called, which in
> > > turn looks for a Variant.
> > >
> > > So after fiddling with some orm settings suddenly a bunch of tests
> > > started to break, which led me to this snippet[1]:
> > >
> > > // disabled
> > > Installation android3 = new Installation();
> > > android3.setAlias("[hidden email]");
> > >
> > android3.setDeviceToken("543234234890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890");
> > > android3.setDeviceType("Android Tablet");
> > > android3.setEnabled(false);
> > >
> > > installationDao.create(android3);
> > >
> > > See, our tests are creating an installation without associating it to a
> > > variant first, then it blows up here[2]:
> > >
> > > // DeviceTokenValidator#isValid()
> > > if (installation.getVariant() == null ||
> > installation.getVariant().getType() == null || deviceToken == null) {
> > >    return false;
> > > }
> > >
> > > This is smelling like a bug to me - kinda like the validator wasn't
> > > actually running during tests. Am I crazy?
> > >
> > > [1]:
> > https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/jpa/src/test/java/org/jboss/aerogear/unifiedpush/jpa/InstallationDaoTest.java#L133-L140
> > > [2]:
> > https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/api/src/main/java/org/jboss/aerogear/unifiedpush/api/validation/DeviceTokenValidator.java#L60
> > >
> > > --
> > > qmx
> > > _______________________________________________
> > > aerogear-dev mailing list
> > > [hidden email]
> > > https://lists.jboss.org/mailman/listinfo/aerogear-dev
> >
> >
> > _______________________________________________
> > aerogear-dev mailing list
> > [hidden email]
> > https://lists.jboss.org/mailman/listinfo/aerogear-dev
> >
>
>
>
> --
> Matthias Wessendorf
>
> blog: http://matthiaswessendorf.wordpress.com/
> sessions: http://www.slideshare.net/mwessendorf
> twitter: http://twitter.com/mwessendorf

> _______________________________________________
> aerogear-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/aerogear-dev


--
qmx
_______________________________________________
aerogear-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/aerogear-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [aerogear-dev] UPS bug - am I crazy?

Matthias Wessendorf
yes, that's a bug in the test

On Friday, February 13, 2015, Douglas Campos <[hidden email]> wrote:
On Fri, Feb 13, 2015 at 12:01:07PM +0100, Matthias Wessendorf wrote:
> Hi,
>
> I agree w/ Erik - however, I think the test has a bug, if the installations
> are not directly associated with a Variant. Should be easy to fix.
That's my point - this test should never pass - hence the "bug"
suspicion.

>
> BTW. qmx, you mentioned the "setDeviceType()", but that is really just
> plain text metadata, like here:
> https://github.com/aerogear/aerogear-push-helloworld/blob/master/ios/HelloWorld/AGAppDelegate.m#L106
>
>
> The validator is triggered by the actual variant type (installation
> .getVariant().getType())
>
> -Matthias
>
>
>
> On Fri, Feb 13, 2015 at 8:17 AM, Erik Jan de Wit <<a href="javascript:;" onclick="_e(event, &#39;cvml&#39;, &#39;edewit@redhat.com&#39;)">edewit@...> wrote:
>
> > Hi,
> >
> > Don’t know if it’s really a bug, but in order to validate that a device
> > token is valid we need to know what variant type it belongs to. It makes no
> > sense for a installation to be persisted without a Variant, because then
> > you cannot send any messages to that device. So if you try to persist a
> > installation without a variant the device token is not valid seems to be to
> > be a valid statement.
> >
> > This was introduced when we added cordova, there it’s easy to make a
> > mistake and have the wrong variantID / secret in your settings. Resulting
> > in a iOS device registering under an android variant. With the deviceToken
> > validation this can no longer happen.
> >
> > Does this make sense?
> >
> > Cheers,
> >         Erik Jan
> >
> > On 13 Feb,2015, at 2:02 , Douglas Campos <<a href="javascript:;" onclick="_e(event, &#39;cvml&#39;, &#39;qmx@qmx.me&#39;)">qmx@...> wrote:
> >
> > > Howdy!
> > >
> > > I was doing the migration work and found out something funny:
> > >
> > > For Installations, we have `installation.setDeviceType()`, but during
> > > Installation `persist()`, the DeviceTokenValidator is called, which in
> > > turn looks for a Variant.
> > >
> > > So after fiddling with some orm settings suddenly a bunch of tests
> > > started to break, which led me to this snippet[1]:
> > >
> > > // disabled
> > > Installation android3 = new Installation();
> > > android3.setAlias("<a href="javascript:;" onclick="_e(event, &#39;cvml&#39;, &#39;foo@bar.org&#39;)">foo@...");
> > >
> > android3.setDeviceToken("543234234890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890");
> > > android3.setDeviceType("Android Tablet");
> > > android3.setEnabled(false);
> > >
> > > installationDao.create(android3);
> > >
> > > See, our tests are creating an installation without associating it to a
> > > variant first, then it blows up here[2]:
> > >
> > > // DeviceTokenValidator#isValid()
> > > if (installation.getVariant() == null ||
> > installation.getVariant().getType() == null || deviceToken == null) {
> > >    return false;
> > > }
> > >
> > > This is smelling like a bug to me - kinda like the validator wasn't
> > > actually running during tests. Am I crazy?
> > >
> > > [1]:
> > https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/jpa/src/test/java/org/jboss/aerogear/unifiedpush/jpa/InstallationDaoTest.java#L133-L140
> > > [2]:
> > https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/api/src/main/java/org/jboss/aerogear/unifiedpush/api/validation/DeviceTokenValidator.java#L60
> > >
> > > --
> > > qmx
> > > _______________________________________________
> > > aerogear-dev mailing list
> > > <a href="javascript:;" onclick="_e(event, &#39;cvml&#39;, &#39;aerogear-dev@lists.jboss.org&#39;)">aerogear-dev@...
> > > https://lists.jboss.org/mailman/listinfo/aerogear-dev
> >
> >
> > _______________________________________________
> > aerogear-dev mailing list
> > <a href="javascript:;" onclick="_e(event, &#39;cvml&#39;, &#39;aerogear-dev@lists.jboss.org&#39;)">aerogear-dev@...
> > https://lists.jboss.org/mailman/listinfo/aerogear-dev
> >
>
>
>
> --
> Matthias Wessendorf
>
> blog: http://matthiaswessendorf.wordpress.com/
> sessions: http://www.slideshare.net/mwessendorf
> twitter: http://twitter.com/mwessendorf

> _______________________________________________
> aerogear-dev mailing list
> <a href="javascript:;" onclick="_e(event, &#39;cvml&#39;, &#39;aerogear-dev@lists.jboss.org&#39;)">aerogear-dev@...
> https://lists.jboss.org/mailman/listinfo/aerogear-dev


--
qmx
_______________________________________________
aerogear-dev mailing list
<a href="javascript:;" onclick="_e(event, &#39;cvml&#39;, &#39;aerogear-dev@lists.jboss.org&#39;)">aerogear-dev@...
https://lists.jboss.org/mailman/listinfo/aerogear-dev


--
Sent from Gmail Mobile

_______________________________________________
aerogear-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/aerogear-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [aerogear-dev] UPS bug - am I crazy?

Matthias Wessendorf
do we have a JIRA for this ? 

On Fri, Feb 13, 2015 at 1:17 PM, Matthias Wessendorf <[hidden email]> wrote:
yes, that's a bug in the test


On Friday, February 13, 2015, Douglas Campos <[hidden email]> wrote:
On Fri, Feb 13, 2015 at 12:01:07PM +0100, Matthias Wessendorf wrote:
> Hi,
>
> I agree w/ Erik - however, I think the test has a bug, if the installations
> are not directly associated with a Variant. Should be easy to fix.
That's my point - this test should never pass - hence the "bug"
suspicion.

>
> BTW. qmx, you mentioned the "setDeviceType()", but that is really just
> plain text metadata, like here:
> https://github.com/aerogear/aerogear-push-helloworld/blob/master/ios/HelloWorld/AGAppDelegate.m#L106
>
>
> The validator is triggered by the actual variant type (installation
> .getVariant().getType())
>
> -Matthias
>
>
>
> On Fri, Feb 13, 2015 at 8:17 AM, Erik Jan de Wit <[hidden email]> wrote:
>
> > Hi,
> >
> > Don’t know if it’s really a bug, but in order to validate that a device
> > token is valid we need to know what variant type it belongs to. It makes no
> > sense for a installation to be persisted without a Variant, because then
> > you cannot send any messages to that device. So if you try to persist a
> > installation without a variant the device token is not valid seems to be to
> > be a valid statement.
> >
> > This was introduced when we added cordova, there it’s easy to make a
> > mistake and have the wrong variantID / secret in your settings. Resulting
> > in a iOS device registering under an android variant. With the deviceToken
> > validation this can no longer happen.
> >
> > Does this make sense?
> >
> > Cheers,
> >         Erik Jan
> >
> > On 13 Feb,2015, at 2:02 , Douglas Campos <[hidden email]> wrote:
> >
> > > Howdy!
> > >
> > > I was doing the migration work and found out something funny:
> > >
> > > For Installations, we have `installation.setDeviceType()`, but during
> > > Installation `persist()`, the DeviceTokenValidator is called, which in
> > > turn looks for a Variant.
> > >
> > > So after fiddling with some orm settings suddenly a bunch of tests
> > > started to break, which led me to this snippet[1]:
> > >
> > > // disabled
> > > Installation android3 = new Installation();
> > > android3.setAlias("[hidden email]");
> > >
> > android3.setDeviceToken("543234234890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890");
> > > android3.setDeviceType("Android Tablet");
> > > android3.setEnabled(false);
> > >
> > > installationDao.create(android3);
> > >
> > > See, our tests are creating an installation without associating it to a
> > > variant first, then it blows up here[2]:
> > >
> > > // DeviceTokenValidator#isValid()
> > > if (installation.getVariant() == null ||
> > installation.getVariant().getType() == null || deviceToken == null) {
> > >    return false;
> > > }
> > >
> > > This is smelling like a bug to me - kinda like the validator wasn't
> > > actually running during tests. Am I crazy?
> > >
> > > [1]:
> > https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/jpa/src/test/java/org/jboss/aerogear/unifiedpush/jpa/InstallationDaoTest.java#L133-L140
> > > [2]:
> > https://github.com/aerogear/aerogear-unifiedpush-server/blob/master/model/api/src/main/java/org/jboss/aerogear/unifiedpush/api/validation/DeviceTokenValidator.java#L60
> > >
> > > --
> > > qmx
> > > _______________________________________________
> > > aerogear-dev mailing list
> > > [hidden email]
> > > https://lists.jboss.org/mailman/listinfo/aerogear-dev
> >
> >
> > _______________________________________________
> > aerogear-dev mailing list
> > [hidden email]
> > https://lists.jboss.org/mailman/listinfo/aerogear-dev
> >
>
>
>
> --
> Matthias Wessendorf
>
> blog: http://matthiaswessendorf.wordpress.com/
> sessions: http://www.slideshare.net/mwessendorf
> twitter: http://twitter.com/mwessendorf

> _______________________________________________
> aerogear-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/aerogear-dev


--
qmx
_______________________________________________
aerogear-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/aerogear-dev


--
Sent from Gmail Mobile



--

_______________________________________________
aerogear-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/aerogear-dev
qmx
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [aerogear-dev] UPS bug - am I crazy?

qmx
Administrator
On Tue, Feb 24, 2015, at 09:09 AM, Matthias Wessendorf wrote:
> do we have a JIRA for this ? 
Not needed, already fixed it on the migration PR

--
qmx

_______________________________________________
aerogear-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/aerogear-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [aerogear-dev] UPS bug - am I crazy?

Matthias Wessendorf
ah! cool

On Tue, Feb 24, 2015 at 1:22 PM, Douglas Campos <[hidden email]> wrote:
On Tue, Feb 24, 2015, at 09:09 AM, Matthias Wessendorf wrote:
> do we have a JIRA for this ? 
Not needed, already fixed it on the migration PR

--
qmx

_______________________________________________
aerogear-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/aerogear-dev



--

_______________________________________________
aerogear-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/aerogear-dev
Loading...