Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send RAW method params to handler #25

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

WiRight
Copy link

@WiRight WiRight commented Dec 25, 2019

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

@movitto
Copy link
Owner

movitto commented Dec 26, 2019

@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

@WiRight
Copy link
Author

WiRight commented Dec 26, 2019

@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!

@movitto
Copy link
Owner

movitto commented Dec 26, 2019

@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!

@WiRight
Copy link
Author

WiRight commented Dec 28, 2019

@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... =)

@movitto
Copy link
Owner

movitto commented Jan 7, 2020

@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

@WiRight
Copy link
Author

WiRight commented Feb 6, 2020

Okay, i'll check it and rewrite!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants