Re: svn commit: r1177789 - /ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java

[Scott Gray-2]

What bug?

On 1/10/2011, at 8:25 AM, [hidden email] wrote:

> Author: jleroux
> Date: Fri Sep 30 19:25:16 2011
> New Revision: 1177789
>
> URL: http://svn.apache.org/viewvc?rev=1177789&view=rev
> Log:
> Fix a bug reported by Martin Sanchez Vivo on user ML
>
> Modified:
>    ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>
> Modified: ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
> URL: http://svn.apache.org/viewvc/ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java?rev=1177789&r1=1177788&r2=1177789&view=diff
> ==============================================================================
> --- ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java (original)
> +++ ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java Fri Sep 30 19:25:16 2011
> @@ -111,10 +111,9 @@ public class ServerHitBin {
>     }
>
>     protected static void countHit(String baseId, int type, HttpServletRequest request, long startTime, long runningTime, GenericValue userLogin, boolean isOriginal) {
> -        String delegatorName = (String) request.getSession().getAttribute("delegatorName");
> -        Delegator delegator = null;
> -        if (UtilValidate.isNotEmpty(delegatorName)) {
> -            delegator = DelegatorFactory.getDelegator(delegatorName);
> +        Delegator delegator = (Delegator)request.getAttribute("delegator");
> +        if (delegator == null){
> +            delegator = DelegatorFactory.getDelegator("default");
>         }
>         if (delegator == null) {
>             throw new IllegalArgumentException("In countHit could not find a delegator or delegatorName to work from");
>
>

[Jacques Le Roux]

http://markmail.org/message/4b37lgy7ymsfg43u

I was able to reproduce

Jacques

Scott Gray wrote:

> What bug?
>
> On 1/10/2011, at 8:25 AM, [hidden email] wrote:
>
>> Author: jleroux
>> Date: Fri Sep 30 19:25:16 2011
>> New Revision: 1177789
>>
>> URL: http://svn.apache.org/viewvc?rev=1177789&view=rev
>> Log:
>> Fix a bug reported by Martin Sanchez Vivo on user ML
>>
>> Modified:
>>    ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>
>> Modified: ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java?rev=1177789&r1=1177788&r2=1177789&view=diff
>> ============================================================================== ---
>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java (original) +++
>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java Fri Sep 30 19:25:16 2011 @@ -111,10
>>     +111,9 @@ public class ServerHitBin { }
>>
>>     protected static void countHit(String baseId, int type, HttpServletRequest request, long startTime, long runningTime,
>> GenericValue userLogin, boolean isOriginal) {
>> -        String delegatorName = (String) request.getSession().getAttribute("delegatorName");
>> -        Delegator delegator = null;
>> -        if (UtilValidate.isNotEmpty(delegatorName)) {
>> -            delegator = DelegatorFactory.getDelegator(delegatorName);
>> +        Delegator delegator = (Delegator)request.getAttribute("delegator");
>> +        if (delegator == null){
>> +            delegator = DelegatorFactory.getDelegator("default");
>>         }
>>         if (delegator == null) {
>>             throw new IllegalArgumentException("In countHit could not find a delegator or delegatorName to work from");

[David E. Jones-2]

Also, this introduces a bug: a hard-coded dependence on the "default"
delegator. The delegator name for this should always comes from the
webapp, which is configured in the web.xml file.

-David


Scott Gray wrote:

> What bug?
>
> On 1/10/2011, at 8:25 AM, [hidden email] wrote:
>
>> Author: jleroux
>> Date: Fri Sep 30 19:25:16 2011
>> New Revision: 1177789
>>
>> URL: http://svn.apache.org/viewvc?rev=1177789&view=rev
>> Log:
>> Fix a bug reported by Martin Sanchez Vivo on user ML
>>
>> Modified:
>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>
>>
>> Modified:
>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java?rev=1177789&r1=1177788&r2=1177789&view=diff
>>
>> ==============================================================================
>>
>> ---
>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>> (original)
>> +++
>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>> Fri Sep 30 19:25:16 2011
>> @@ -111,10 +111,9 @@ public class ServerHitBin {
>> }
>>
>> protected static void countHit(String baseId, int type,
>> HttpServletRequest request, long startTime, long runningTime,
>> GenericValue userLogin, boolean isOriginal) {
>> - String delegatorName = (String)
>> request.getSession().getAttribute("delegatorName");
>> - Delegator delegator = null;
>> - if (UtilValidate.isNotEmpty(delegatorName)) {
>> - delegator = DelegatorFactory.getDelegator(delegatorName);
>> + Delegator delegator = (Delegator)request.getAttribute("delegator");
>> + if (delegator == null){
>> + delegator = DelegatorFactory.getDelegator("default");
>> }
>> if (delegator == null) {
>> throw new IllegalArgumentException("In countHit could not find a
>> delegator or delegatorName to work from");
>>
>>
>

[Scott Gray-2]

Two things:
1. By asking "What bug?" I was trying to remind you to add proper details to commit messages.
2. Are you sure this is the correct fix?  I'm pretty sure we moved away from using a default delegator when the tenant stuff was added.

Maybe the deeper question is "Why is delegator name empty? Why is there no delegator instance in the request?".  Not everything is solvable with a quick fix, it is always better to try and understand the cause of the problem before coming up with a solution.

Regards
Scott

On 1/10/2011, at 5:54 PM, Jacques Le Roux wrote:

> http://markmail.org/message/4b37lgy7ymsfg43u
>
> I was able to reproduce
>
> Jacques
>
> Scott Gray wrote:
>> What bug?
>>
>> On 1/10/2011, at 8:25 AM, [hidden email] wrote:
>>
>>> Author: jleroux
>>> Date: Fri Sep 30 19:25:16 2011
>>> New Revision: 1177789
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1177789&view=rev
>>> Log:
>>> Fix a bug reported by Martin Sanchez Vivo on user ML
>>>
>>> Modified:
>>>   ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>>
>>> Modified: ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java?rev=1177789&r1=1177788&r2=1177789&view=diff
>>> ============================================================================== ---
>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java (original) +++
>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java Fri Sep 30 19:25:16 2011 @@ -111,10
>>>    +111,9 @@ public class ServerHitBin { }
>>>
>>>    protected static void countHit(String baseId, int type, HttpServletRequest request, long startTime, long runningTime,
>>> GenericValue userLogin, boolean isOriginal) {
>>> -        String delegatorName = (String) request.getSession().getAttribute("delegatorName");
>>> -        Delegator delegator = null;
>>> -        if (UtilValidate.isNotEmpty(delegatorName)) {
>>> -            delegator = DelegatorFactory.getDelegator(delegatorName);
>>> +        Delegator delegator = (Delegator)request.getAttribute("delegator");
>>> +        if (delegator == null){
>>> +            delegator = DelegatorFactory.getDelegator("default");
>>>        }
>>>        if (delegator == null) {
>>>            throw new IllegalArgumentException("In countHit could not find a delegator or delegatorName to work from");

[Jacques Le Roux]

Scott Gray wrote:
> Two things:
> 1. By asking "What bug?" I was trying to remind you to add proper details to commit messages.

Agreed and done

> 2. Are you sure this is the correct fix?  I'm pretty sure we moved away from using a default delegator when the tenant stuff was
> added.

Actually, I guess this part could be ommited (just belt and suspenders)
>>>> +        if (delegator == null){
>>>> +            delegator = DelegatorFactory.getDelegator("default");
>>>>        }
There is another use in R10.04 (EntityExpr.java[210])
// this will be the common case for now as the delegator isn't available where we want to do this
// we'll cheat a little here and assume the default delegator

So when doing a quick review I thought it could not hurt before throwing an exception.  Maybe in tenant case it could be a problem
indeed and should be rather removed (I doubt the delegator could miss in the  request and in such case it's indeed maybe better to
simply throw an exception)

> Maybe the deeper question is "Why is delegator name empty? Why is there no delegator instance in the request?".  Not everything
> is solvable with a quick fix, it is always better to try and understand the cause of the problem before coming up with a
> solution.

Note the delegator missed in the session not the request, as stated above the 2d block is just belt and suspenders, maybe useless
and wrong indeed

Jacques

> Regards
> Scott
>
> On 1/10/2011, at 5:54 PM, Jacques Le Roux wrote:
>
>> http://markmail.org/message/4b37lgy7ymsfg43u
>>
>> I was able to reproduce
>>
>> Jacques
>>
>> Scott Gray wrote:
>>> What bug?
>>>
>>> On 1/10/2011, at 8:25 AM, [hidden email] wrote:
>>>
>>>> Author: jleroux
>>>> Date: Fri Sep 30 19:25:16 2011
>>>> New Revision: 1177789
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1177789&view=rev
>>>> Log:
>>>> Fix a bug reported by Martin Sanchez Vivo on user ML
>>>>
>>>> Modified:
>>>>   ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>>>
>>>> Modified: ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java?rev=1177789&r1=1177788&r2=1177789&view=diff
>>>> ============================================================================== ---
>>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java (original) +++
>>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java Fri Sep 30 19:25:16 2011 @@ -111,10
>>>>    +111,9 @@ public class ServerHitBin { }
>>>>
>>>>    protected static void countHit(String baseId, int type, HttpServletRequest request, long startTime, long runningTime,
>>>> GenericValue userLogin, boolean isOriginal) {
>>>> -        String delegatorName = (String) request.getSession().getAttribute("delegatorName");
>>>> -        Delegator delegator = null;
>>>> -        if (UtilValidate.isNotEmpty(delegatorName)) {
>>>> -            delegator = DelegatorFactory.getDelegator(delegatorName);
>>>> +        Delegator delegator = (Delegator)request.getAttribute("delegator");
>>>> +        if (delegator == null){
>>>> +            delegator = DelegatorFactory.getDelegator("default");
>>>>        }
>>>>        if (delegator == null) {
>>>>            throw new IllegalArgumentException("In countHit could not find a delegator or delegatorName to work from");

[Jacques Le Roux]

Fixed at r1177923, see also my answer to Scott's, I did no read your message before. I think we would never get to this point,
anyway better safe than sorry, since it was wrong.

Jacques

David E Jones wrote:

> Also, this introduces a bug: a hard-coded dependence on the "default"
> delegator. The delegator name for this should always comes from the
> webapp, which is configured in the web.xml file.
>
> -David
>
>
> Scott Gray wrote:
>> What bug?
>>
>> On 1/10/2011, at 8:25 AM, [hidden email] wrote:
>>
>>> Author: jleroux
>>> Date: Fri Sep 30 19:25:16 2011
>>> New Revision: 1177789
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1177789&view=rev
>>> Log:
>>> Fix a bug reported by Martin Sanchez Vivo on user ML
>>>
>>> Modified:
>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>>
>>>
>>> Modified:
>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>>
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java?rev=1177789&r1=1177788&r2=1177789&view=diff
>>>
>>> ==============================================================================
>>>
>>> ---
>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>> (original)
>>> +++
>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>> Fri Sep 30 19:25:16 2011
>>> @@ -111,10 +111,9 @@ public class ServerHitBin {
>>> }
>>>
>>> protected static void countHit(String baseId, int type,
>>> HttpServletRequest request, long startTime, long runningTime,
>>> GenericValue userLogin, boolean isOriginal) {
>>> - String delegatorName = (String)
>>> request.getSession().getAttribute("delegatorName");
>>> - Delegator delegator = null;
>>> - if (UtilValidate.isNotEmpty(delegatorName)) {
>>> - delegator = DelegatorFactory.getDelegator(delegatorName);
>>> + Delegator delegator = (Delegator)request.getAttribute("delegator");
>>> + if (delegator == null){
>>> + delegator = DelegatorFactory.getDelegator("default");
>>> }
>>> if (delegator == null) {
>>> throw new IllegalArgumentException("In countHit could not find a
>>> delegator or delegatorName to work from");

[Jacques Le Roux]

Actually better and complete fix at r1177926. I should have backported it when doing r1021344

Jacques

Jacques Le Roux wrote:

> Fixed at r1177923, see also my answer to Scott's, I did no read your message before. I think we would never get to this point,
> anyway better safe than sorry, since it was wrong.
>
> Jacques
>
> David E Jones wrote:
>> Also, this introduces a bug: a hard-coded dependence on the "default"
>> delegator. The delegator name for this should always comes from the
>> webapp, which is configured in the web.xml file.
>>
>> -David
>>
>>
>> Scott Gray wrote:
>>> What bug?
>>>
>>> On 1/10/2011, at 8:25 AM, [hidden email] wrote:
>>>
>>>> Author: jleroux
>>>> Date: Fri Sep 30 19:25:16 2011
>>>> New Revision: 1177789
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1177789&view=rev
>>>> Log:
>>>> Fix a bug reported by Martin Sanchez Vivo on user ML
>>>>
>>>> Modified:
>>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>>>
>>>>
>>>> Modified:
>>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>>>
>>>> URL:
>>>> http://svn.apache.org/viewvc/ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java?rev=1177789&r1=1177788&r2=1177789&view=diff
>>>>
>>>> ==============================================================================
>>>>
>>>> ---
>>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>>> (original)
>>>> +++
>>>> ofbiz/branches/release10.04/framework/webapp/src/org/ofbiz/webapp/stats/ServerHitBin.java
>>>> Fri Sep 30 19:25:16 2011
>>>> @@ -111,10 +111,9 @@ public class ServerHitBin {
>>>> }
>>>>
>>>> protected static void countHit(String baseId, int type,
>>>> HttpServletRequest request, long startTime, long runningTime,
>>>> GenericValue userLogin, boolean isOriginal) {
>>>> - String delegatorName = (String)
>>>> request.getSession().getAttribute("delegatorName");
>>>> - Delegator delegator = null;
>>>> - if (UtilValidate.isNotEmpty(delegatorName)) {
>>>> - delegator = DelegatorFactory.getDelegator(delegatorName);
>>>> + Delegator delegator = (Delegator)request.getAttribute("delegator");
>>>> + if (delegator == null){
>>>> + delegator = DelegatorFactory.getDelegator("default");
>>>> }
>>>> if (delegator == null) {
>>>> throw new IllegalArgumentException("In countHit could not find a
>>>> delegator or delegatorName to work from");