apache / openwhisk

Apache OpenWhisk is an open source serverless cloud platform
https://openwhisk.apache.org/
Apache License 2.0
6.54k stars 1.17k forks source link

cli parsing of numbers to floating point considered suprising #1717

Closed stevenoh93 closed 7 years ago

stevenoh93 commented 7 years ago

Environment details:

I'm running wsk trigger create... command on Mac CLI.

Steps to reproduce the issue:

When I passed in -p clientId '12345.231452', the parameter in the trigger would be rounded to 12345.231. But when I passed in -p clientId 'a12345.231452', I would get the expected value: a12345.231452. Seems like OW is converting the parameters as floating points and rounding them?

rabbah commented 7 years ago

I am not seeing this rounding error (I'm also on a mac).

> wsk action create echo tests/dat/actions/echo.js
ok: created action echo
> wsk action invoke echo -p  clientId '12345.231452' -br
{
    "clientId": 12345.231452
}

I also verified that a trigger firing results in the same.

> wsk trigger create t
> wsk rule create r t echo
> wsk trigger fire t -p  clientId '12345.231452'
> wsk activation list
activations
583b5685cfe040188d5169a00f915782                 echo
6c2d8b897e0b412781bde4bd3c4c7b06                    r
04486f28b04c4f99bc5753121cf85238                    t
> wsk activation result 583b5685cfe040188d5169a00f915782
{
    "clientId": 12345.231452
}

Can you provide more detail?

stevenoh93 commented 7 years ago

I passed in the parameters during the creation of the trigger:

wsk trigger create slackevents --feed slack_feed -p clientId "12345.12345" -p clientSecret 'muchsercret'

Is this not the best practice to create triggers?

rabbah commented 7 years ago

got it - so this is a feed, which means there's some action on the backside running and the rounding is likely happening there. we'll need to check the slack package. @houshengbo @jasonpet any ideas?

stevenoh93 commented 7 years ago

I'm not using the Slack package here. slack_feed is actually a custom feed that I wrote, and it contains this:

if (lifecycleEvent == 'CREATE') {
    var body = {
        "clientId": params.clientId.substring(1),
        "clientSecret": params.clientSecret,
        "name": triggerName[2],
    "namespace": triggerName[1]
    };
    ...

I had to add .substring(1) to temporarily fix the rounding issue by pre-pending a character.

rabbah commented 7 years ago

I still can't replicate it.

# not a proper feed but at least checks that client id is not mangled
> cat slack_feed.js 
function main(args) {
    console.log(args.clientId)
    return { clientId: args.clientId }
}
>  wsk action create slack_feed slack_feed.js
ok: created action slack_feed
> wsk trigger create slackevents --feed slack_feed -p clientId "12345.12345" -p clientSecret 'muchsercret' -v
...
"response": {
    "result": {
      "clientId": 12345.12345
    },
    "success": true,
    "status": "success"
  },
...
stevenoh93 commented 7 years ago

Weird.. maybe it only happens when the number is longer? Could you try with this id? This one matches the length of my actual id. 12345678912.123456789012

rabbah commented 7 years ago
> wsk action create echo tests/dat/actions/echo.js
ok: created action echo
> wsk action invoke echo -br -p a '12345678912.123456789012' -v
REQUEST:
[POST]  https://192.168.99.100:443/api/v1/namespaces/_/actions/echo?blocking=true
Req Body
{{"a":1.2345678912123457e+10}
}
...
{
    "a": 1.2345678912123457e+10
}

@dubeejw seems the CLI is parsing the number and changing its notation.

> wsk action invoke echo -br -p a '"12345678912.123456789012"' 
{
    "a": "12345678912.123456789012"
}
> wsk action invoke echo -br -p a '12345678912.123456789012' 
{
    "a": 1.2345678912123457e+10
}
dubee commented 7 years ago

Looks like the Go programming language only supports 64-bit floating point numbers when encoding parameters into JSON.

lzbj commented 7 years ago

It seems someone already encounter the same issue, http://stackoverflow.com/questions/22343083/json-marshaling-with-long-numbers-in-golang-gives-floating-point-number

So we add a flag to let user choose? or By default use decoder as numbers? Any ideas?

mbehrendt commented 7 years ago

would it be possible to just treat everything as a string? any major downsides to that?

lzbj commented 7 years ago

Decode it as float64 as current, we can change the code to use string representation, just FYR @mbehrendt .

@rabbah @dubeejw help correct if I'm error.

rabbah commented 7 years ago

Does the UseNumber approach work for the wsk cli?

lzbj commented 7 years ago

Yes, I'll do some test and submit a pr if I can.

lzbj commented 7 years ago

results.txt

@rabbah ,please see the attached file for my test results, the former part is the result with my code change, the latter one is the original, the activation result decoder also needs change, or it will decode the result in wrong format.

btw, please ignore the 2 commits above.

rabbah commented 7 years ago

Fixed by https://github.com/openwhisk/openwhisk/commit/03e041c416e19283cf1e1f2b58da85687c0c8dd5.