Skip to content

Convert if plugin to use jinja expressions instead of python eval.#1551

Merged
gazpachoking merged 1 commit intonextfrom
jinja_if
Dec 12, 2016
Merged

Convert if plugin to use jinja expressions instead of python eval.#1551
gazpachoking merged 1 commit intonextfrom
jinja_if

Conversation

@gazpachoking
Copy link
Copy Markdown
Member

@gazpachoking gazpachoking commented Dec 12, 2016

Motivation for changes:

Python eval is not safe for user input. Jinja expressions are much more appropriate.

Detailed changes:

  • if plugin statements are now evaluated as jinja expressions
  • As jinja expressions have mostly the same syntax as the limited form of python we intended with the if plugin, most current configs should still work.
  • We can now also use jinja filters and tests in if statements.
  • implemented the expression evaluation via flexget.utils.evaluate_expression which allows for lazy fields to stay lazy for evaluation. (which the fully default jinja method wouldn't do by itself)

Our tests seem to just pass, not sure if there are cases where config changes could be needed.

@gazpachoking
Copy link
Copy Markdown
Member Author

gazpachoking commented Dec 12, 2016

I'd sortof like to change the if plugin config to bring the expressions to the value side of a dict, so that we can validate it with a schema. We could add a custom schema 'format' for these expressions to catch syntax errors during validation. Not exactly sure what the nicest way to do that is, and if it's worth the extra validation ability though.
perhaps:

if:
- if: year >= now.year
  action: accept

It sorta doubles up the 'if's, but it might also open room for an 'else':

if:
- if: year >= now.year
  action: accept
- elif: year < now.year - 2:
  action: reject
- else:
  action:
    imdb:
       min_score: 6.0

Actually, I got stuck on the else bit. Is the implicit 'None' there horrible? I sorta think it is.

@liiight
Copy link
Copy Markdown
Member

liiight commented Dec 12, 2016

I think the implicit None is fine, if plugin was always programmatic in nature, this fits in well.
schema format is a good idea but since a lot of fields in flexget use jinja not sure if it's relevant only to this plugin.

@paranoidi
Copy link
Copy Markdown
Member

Awesome! I always told you eval is evil! :)

@gazpachoking
Copy link
Copy Markdown
Member Author

I think we'll just go with the engine change for now. Config format can be revisited later.

@gazpachoking gazpachoking merged commit c8e14d9 into next Dec 12, 2016
@gazpachoking gazpachoking deleted the jinja_if branch December 23, 2016 17:43
@liiight liiight mentioned this pull request Dec 25, 2016
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