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

Updated JsonPathV2 version to latest. #1674

Merged
merged 1 commit into from
Sep 11, 2016
Merged

Updated JsonPathV2 version to latest. #1674

merged 1 commit into from
Sep 11, 2016

Conversation

Skarlso
Copy link
Contributor

@Skarlso Skarlso commented Sep 8, 2016

No description provided.

@dsander
Copy link
Collaborator

dsander commented Sep 8, 2016

Awesome! Is it save now that you are using instance_eval or could a user still "break out" of the binding?

@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 8, 2016

It is a lot safer than a plain Eval with no binding what was before this! A plain Eval with no binding has the current context, which is Everything! Instance_eval has only current instance context which is basically JsonPath at that point or the @_current_node instance variable which has limited access to things.

It's very close to a simple block being passed around. But I wouldn't say it's airtight now, yet. :-)

@cantino
Copy link
Member

cantino commented Sep 8, 2016

instance_eval is just as dangerous if it's allowed.

class A; end
A.new.instance_eval "eval('`pwd`')"
# => "/Users/andrewcantino/workspace/...\n"

@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 8, 2016

Yes, like I said it's not airtight. Also I was referring to Ruby's scope. So instance_eval doesn't have the same scope as a plain no binding Eval.

@cantino
Copy link
Member

cantino commented Sep 8, 2016

Yea, I think it's fine as long as it's still off by default in Huginn. :)

What else has changed between 0.0.3 and 0.0.7?

@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 8, 2016

Functionally nothing. :) I'm trying to make the code a bit easier to read and understand. Which isn't too easy. :D

I'm going to write a list of changes in a bit.

@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 8, 2016

So, the changes:

  • Replace Eval
  • Refactors to decrees cyclomatic complexity and nested if depths
  • Some refactors to pull out common regexes into one when in a case
  • There was a bug where it was not parsing elements with @ signs in them.
  • Exploded the huge case / when inner workings into smaller chunked def's so I can later explode them even more, and than easily replace parts of it better.
  • Added a few tests here and there, for issues people were having.
  • And, rubocop fixes applied to the whole codebase.

EDIT: And ofc, eval is still disabled by default. You have to explicitly enable it if you want to use it. :)
EDIT2: So, just to be clear. Eval is still a dangerous thing. What scope means is, that it has the singleton object as a scope, whatever self points to. So if it would define a new method, for example, that would only be defined on self. class_eval, instance_eval, only change the lexical scope of eval. Meaning to add a method to a class with eval, I would use class_eval, or eval with a class binding. Instance only changes the current instance and not every instance. Which is cool.

But eval is still able to execute arbitrary code. And eventually, I want to completely remove the use of eval. But it's proving to be very difficult. :)

@cantino
Copy link
Member

cantino commented Sep 10, 2016

@Skarlso should we merge this, or do you want to update it to fix that >= error in one of your commits?

@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 10, 2016

I'm going to fix it today. :) Wait until than please. :) Thanks.

@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 10, 2016

@cantino Fixed and pushed a new version. Thanks!

@cantino cantino merged commit b815e83 into huginn:master Sep 11, 2016
@cantino
Copy link
Member

cantino commented Sep 11, 2016

Thanks @Skarlso!

@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 11, 2016

Eyy, thanks @cantino! I'm trying to debug stuff right now. And I'm writing a ton of new tests. Hopefully I'll flush something out. :-)

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.

3 participants