Non-admin users cannot edit own profile (production)

Wow, we're a bit screwed here. The problem is this: new Long(4) == new Long(4) is false.

Because all of our primary keys in the client classes are Longs, not longs, comparing them with == is doing a reference check to see whether the Long container is equal to the other Long container. Somehow, this works on the dev mode, but it doesn't on production.

In this particular case, the offending line is in UserServiceImpl.getByName, line 57:
        ClientView cv = (curUser.getPrimaryKey() == ud.getPrimaryKey()) ?
                                        ClientView.PRIVATE : ClientView.PUBLIC;

That needs to be
         ClientView cv = (curUser.getPrimaryKey().equals(ud.getPrimaryKey())) ?
                                        ClientView.PRIVATE : ClientView.PUBLIC;

That fixes this particular problem on production, but who knows what else is buggy because of these sort of comparisons. (Basically all permissions are checked using ==.)

Okay, that wasn't nearly as hard as I thought. I grepped and carefully read through every single "==" operation in all of Mugle, and it turns out there were just four comparisons between two boxed numbers, all of them in the user service (which explains why this wasn't a problem for teams or games).

Fixed in trunk r459 -- now use .equals for all comparisons between boxed numbers.

Perhaps we can do a find and replace of all instances of ==, since
most of the comparisons will be based on references like this.

On 26/05/2011, at 4:01 PM, Matt Giuca <email address hidden> wrote:

> Wow, we're a bit screwed here. The problem is this: new Long(4) == new
> Long(4) is false.
>
> Because all of our primary keys in the client classes are Longs, not
> longs, comparing them with == is doing a reference check to see
> whether
> the Long container is equal to the other Long container. Somehow, this
> works on the dev mode, but it doesn't on production.
>
> In this particular case, the offending line is in
> UserServiceImpl.getByName, line 57:
> ClientView cv = (curUser.getPrimaryKey() == ud.getPrimaryKey
> ()) ?
> ClientView.PRIVATE :
> ClientView.PUBLIC;
>
> That needs to be
> ClientView cv = (curUser.getPrimaryKey().equals
> (ud.getPrimaryKey())) ?
> ClientView.PRIVATE :
> ClientView.PUBLIC;
>
> That fixes this particular problem on production, but who knows what
> else is buggy because of these sort of comparisons. (Basically all
> permissions are checked using ==.)
>
> --
> You received this bug notification because you are subscribed to
> MUGLE.
> https://bugs.launchpad.net/bugs/788479
>
> Title:
> Non-admin users cannot edit own profile (production)
>
> Status in Melbourne University Game-based Learning Environment:
> In Progress
>
> Bug description:
> Non-admin users have userCanEdit set to false (can't see the edit
> button or access the edit page) for their own User objects. This is
> not a problem for devteams or games, only users. It only shows up on
> the production server; the testing server (including the gwtc
> compiled
> code) does not exhibit this problem.

Forgot to check for instances of !=. Fixed a few more bad comparisons in trunk r461.

@Scott: Nope, only a handful, it turns out. The majority of them are == null, and some of them are in database queries.

Won't this still be a problem with the database queries? I'm confused

On 26/05/2011, at 4:36 PM, Matt Giuca <email address hidden> wrote:

> @Scott: Nope, only a handful, it turns out. The majority of them are
> ==
> null, and some of them are in database queries.
>
> --
> You received this bug notification because you are subscribed to
> MUGLE.
> https://bugs.launchpad.net/bugs/788479
>
> Title:
> Non-admin users cannot edit own profile (production)
>
> Status in Melbourne University Game-based Learning Environment:
> Fix Committed
>
> Bug description:
> Non-admin users have userCanEdit set to false (can't see the edit
> button or access the edit page) for their own User objects. This is
> not a problem for devteams or games, only users. It only shows up on
> the production server; the testing server (including the gwtc
> compiled
> code) does not exhibit this problem.

No, database queries is a separate language (JDO Query Language); in that language the numbers are primitives, not boxed (for them to be boxed, they would have to have their own database table), so it couldn't possibly be doing a reference equality. It's only in the Java code that it's a problem, since == checks the pointers to the boxes.