"Magically" converted types from simpleTypeConvert

[Bob Morley]

We have just started to make use of ProductionRun in our project and we were having a problem in the create because the ProductionRunServices (around line 274) makes a call to "createWorkEffort" using a "quantityToProduce" that is a BigDecimal.  The field on the WorkEffort entity is actually a Double, so in our solution it was causing a "bad type error" and failing.

I jumped to trunk to try to figure out why it was working in stock Ofbiz.  After some digging, it turns out that the conversion enhancements now (appear to) silently do this conversion, resulting in a context that contains that field as a Double.

This happens because the ServiceDispatcher's runSync method calls "checkAuth" which (if the user can call the service) calls ModelService.makeValid which ultimately calls ObjectType.simpleTypeConvert that handles the BigDecimal -> Double conversion.

Is this the intended behavior?  To be honest, it seems to me that the complaint that the parameter was not the proper data type was more desirable.  I understand that when we are handling a postback, values need to be converted from String to their desired types; but if I am making a call in our back-end to another service I would think I should be forced to adhere to that service definition's signature.

[Scott Gray-2]

Firstly and without verifying, the automatic conversion sounds wrong and yes I'm quite sure it intentional never used to be that way.

Secondly, I'm not sure why quantityToProduce is a floating point instead of a fixed point data type.  I'm surprised and can't imagine why I skipped these when I was doing the double -> BigDecimal conversion of everything.

Regards
Scott

On 14/04/2010, at 6:50 AM, Bob Morley wrote:

> We have just started to make use of ProductionRun in our project and we were
> having a problem in the create because the ProductionRunServices (around
> line 274) makes a call to "createWorkEffort" using a "quantityToProduce"
> that is a BigDecimal.  The field on the WorkEffort entity is actually a
> Double, so in our solution it was causing a "bad type error" and failing.
>
> I jumped to trunk to try to figure out why it was working in stock Ofbiz.
> After some digging, it turns out that the conversion enhancements now
> (appear to) silently do this conversion, resulting in a context that
> contains that field as a Double.
>
> This happens because the ServiceDispatcher's runSync method calls
> "checkAuth" which (if the user can call the service) calls
> ModelService.makeValid which ultimately calls ObjectType.simpleTypeConvert
> that handles the BigDecimal -> Double conversion.
>
> Is this the intended behavior?  To be honest, it seems to me that the
> complaint that the parameter was not the proper data type was more
> desirable.  I understand that when we are handling a postback, values need
> to be converted from String to their desired types; but if I am making a
> call in our back-end to another service I would think I should be forced to
> adhere to that service definition's signature.
> --
> View this message in context: http://n4.nabble.com/Magically-converted-types-from-simpleTypeConvert-tp1838891p1838891.html
> Sent from the OFBiz - Dev mailing list archive at Nabble.com.

[Bob Morley]

Scott Gray-2 wrote

Firstly and without verifying, the automatic conversion sounds wrong and yes I'm quite sure it intentional never used to be that way.

Secondly, I'm not sure why quantityToProduce is a floating point instead of a fixed point data type.  I'm surprised and can't imagine why I skipped these when I was doing the double -> BigDecimal conversion of everything.

Ahhhh HA!  Ok I did more digging and it appears that the call to ModelService.makeValid would always do these type of conversions "magically".  I suspect the main intent here was to take a context that may or may not exactly match a service definition (parms & parm types) and select the desirable parameters into a new context that can be used for invocation.  There is no difference between the old and new in terms of this method being called.

However, there appears to be some problem code, here it is from ServiceDispatcher ...

        if (UtilValidate.isNotEmpty(origService.permissionServiceName)) {
            ...
            if (hasPermission.booleanValue()) {
                context.putAll(permResp);
                context = origService.makeValid(context, ModelService.IN_PARAM);

Effectively what happens is if a ModelService has a permissionServiceName, it will invoke that permission and if it is successful it adds the permission response to the context and converts it to match the service that will be invoked.  I suspect this gives us the ability to have the service permission provide arguments to the underlying service.

Someone on our team overrode the "createWorkEffort" and removed the permissionServiceName.  So this code would not execute, we had the context with the Double in it, and service dispatcher produced the standard violation.

My suggestion is we change this code to make use of ServiceUtil.setServiceFields, something like this ...

Map<String, Object> permRespContext = ServiceUtil.setServiceFields(dctx, serviceName, permResp);
context.putAll(permRespContext);

Effectively, we would take the permission response and grab the things from it that match our service signature and then add those to the context (and ultimately return it).  This would ensure that only replaces are added without impacting anything else.  The second part to this would be either changing WorkEffort to use BigDecimal OR change the service implementation to pass in a Double when invoking those services.

Make sense?

[Scott Gray-2]

On 14/04/2010, at 10:23 AM, Bob Morley wrote:

>
>
> Scott Gray-2 wrote:
>>
>> Firstly and without verifying, the automatic conversion sounds wrong and
>> yes I'm quite sure it intentional never used to be that way.
>>
>> Secondly, I'm not sure why quantityToProduce is a floating point instead
>> of a fixed point data type.  I'm surprised and can't imagine why I skipped
>> these when I was doing the double -> BigDecimal conversion of everything.
>>
>
> Ahhhh HA!  Ok I did more digging and it appears that the call to
> ModelService.makeValid would always do these type of conversions
> "magically".  I suspect the main intent here was to take a context that may
> or may not exactly match a service definition (parms & parm types) and
> select the desirable parameters into a new context that can be used for
> invocation.  There is no difference between the old and new in terms of this
> method being called.
>
> However, there appears to be some problem code, here it is from
> ServiceDispatcher ...
>
>        if (UtilValidate.isNotEmpty(origService.permissionServiceName)) {
>            ...
>            if (hasPermission.booleanValue()) {
>                context.putAll(permResp);
>                context = origService.makeValid(context,
> ModelService.IN_PARAM);
>
> Effectively what happens is if a ModelService has a permissionServiceName,
> it will invoke that permission and if it is successful it adds the
> permission response to the context and converts it to match the service that
> will be invoked.  I suspect this gives us the ability to have the service
> permission provide arguments to the underlying service.
>
> Someone on our team overrode the "createWorkEffort" and removed the
> permissionServiceName.  So this code would not execute, we had the context
> with the Double in it, and service dispatcher produced the standard
> violation.
>
> My suggestion is we change this code to make use of
> ServiceUtil.setServiceFields, something like this ...
>
> Map<String, Object> permRespContext = ServiceUtil.setServiceFields(dctx,
> serviceName, permResp);
> context.putAll(permRespContext);
>
> Effectively, we would take the permission response and grab the things from
> it that match our service signature and then add those to the context (and
> ultimately return it).  This would ensure that only replaces are added
> without impacting anything else.  

That sounds fine, we definitely shouldn't be modifying context variables that have nothing to do with the permission call.  My only concern would be that switching from makeValid to setServiceFields could alter some sort of behavior that a previous developer deemed necessary (although I can't imagine what the reason would be).  But yes I think we should definitely change it from dealing with context to dealing with permResp instead and I think using setServiceFields should be fine.

> The second part to this would be either
> changing WorkEffort to use BigDecimal OR change the service implementation
> to pass in a Double when invoking those services.

I think we should switch it to BigDecimal.

Regards
Scott

[Bob Morley]

I have captures the pertinent bits from this thread and created two new JIRAs ...

OFBIZ-3699 - ServiceDispatcher.checkAuth modifies the context if the invocation service has a permissionServiceName
OFBIZ-3700 - Convert WorkEffort (and related entities) quantities from Double -> BigDecimal

Not sure when I will get a chance to work on these tickets, but we can move the conversation to them if more discussion is warranted.