rjr icon indicating copy to clipboard operation
rjr copied to clipboard

Send RAW method params to handler

Open WiRight opened this issue 5 years ago • 6 comments

Send raw params of method to handler

It makes for methods like this

{
  "jsonrpc": "2.0",
  "method": "user.login",
  "id": 1,
  "params": {
    "login": "user",
    "password": "pass"
  }
}

Params in this case -> object positional parameters

WiRight avatar Dec 25 '19 14:12 WiRight

@WiRight thanks for the PR. This technically works but removes an elegant feature from this library, specifically the 'explosion' of the positional parameters. eg now the handler will be forced to be defined as taking one parameter, whether it be array or object:


def handler(params)
end

vs

def handler(param1, param2)
end

The later is clearer to understand and I feel more in line w/ the 'ruby-esque' way of making code simple and intuitive.

Ruby also supports 'named' params which can be 'exploded' using the 'double splat' operator. See this article for reference.

What I would suggest here is to detect the type of the 'rjr_method_args' member variable (hash or array) and use the corresponding 'splat' operator appropriately, eg

if rjr_method_args.kind_of?(Array)
  instance_exec(*rjr_method_args, &rjr_handler)
else # if rjr_method_args.kind_of?(Hash)
  instance_exec(**rjr_method_args, &rjr_handler)
end

Then you can define your handler like so:

def handler(login, password)
end

movitto avatar Dec 26 '19 13:12 movitto

@movitto It looks good! I am new in ruby, and dont know some features of this lang.

Thx, i'll try to fix my code!

WiRight avatar Dec 26 '19 15:12 WiRight

@movitto It looks good! I am new in ruby, and dont know some features of this lang.

Thx, i'll try to fix my code!

@WiRight You're doing a great job so far!

movitto avatar Dec 26 '19 15:12 movitto

@movitto Hey, I corrected what was suggested. And now in callback method you may wrote

node.dispatcher.handle('hello') do |login: '', password: ''|
  "Hello #{login} with #{password}"
end

Unfortunately, for now I can't write a tests, because Message::Request expects array... I afraid that i can make some trouble if i change this class... =)

WiRight avatar Dec 28 '19 19:12 WiRight

@WiRight a few things, could you rebase off the latest master and merge the patches into one? It seems like there is a bit of a back and forth with the changes in the different patches in this PR (you revert changes you made in the first patches in subsequent patches)

Also we cannot convert the args to symbols in the way you do it as this presents an attack vector / vulnerability. Symbols are frozen strings, meaning they are never garbage collected, meaning an attacker could send alot of random messages with random parameters to cause a memory leak and crash the process. See: https://cwiki.apache.org/confluence/display/DTACLOUD/Preventing+memory+leaks+in+Ruby

movitto avatar Jan 07 '20 00:01 movitto

Okay, i'll check it and rewrite!

WiRight avatar Feb 06 '20 16:02 WiRight