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.

Sign up for GitHub

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

feat: handle datetime in nosql_apply_parameters_to_query #299

Merged
merged 1 commit into from
Jan 29, 2021
Merged

feat: handle datetime in nosql_apply_parameters_to_query #299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion tests/test_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from datetime import datetime, timedelta

import pytest
from aiohttp import web

Expand Down Expand Up @@ -142,9 +144,28 @@ def test_apply_parameter_to_query_do_nothing():
{'code': 'Paris_France', 'domain': 'Test'},
),
({'code': '{{city}}_{{country}}', 'domain': 'Test'}, None, {'domain': 'Test'}),
(
{'column': 'date', 'operator': 'eq', 'value': '{{ t + delta }}'},
{'t': datetime(2020, 12, 31), 'delta': timedelta(days=1)},
{'column': 'date', 'operator': 'eq', 'value': datetime(2021, 1, 1)},
),
(
{'column': 'date', 'operator': 'eq', 'value': '{{ t.strftime("%d/%m/%Y") }}'},
{'t': datetime(2020, 12, 31)},
{'column': 'date', 'operator': 'eq', 'value': '31/12/2020'},
),
(
{'column': 'date', 'operator': 'in', 'value': '{{ allowed_dates }}'},
{'allowed_dates': [datetime(2020, 12, 31), datetime(2021, 1, 1)]},
{
'column': 'date',
'operator': 'in',
'value': [datetime(2020, 12, 31), datetime(2021, 1, 1)],
Comment on lines +158 to +163
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great for checkboxes variables 👍

},
),
],
)
def test_apply_parameter_to_query(query, params, expected):
def test_nosql_apply_parameters_to_query(query, params, expected):
assert nosql_apply_parameters_to_query(query, params) == expected


Expand Down
31 changes: 15 additions & 16 deletions toucan_connectors/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import pyjq
from aiohttp import ClientSession
from jinja2 import Environment, StrictUndefined, Template, meta
from jinja2.nativetypes import NativeEnvironment
from pydantic import Field
from toucan_data_sdk.utils.helpers import slugify

Expand All @@ -16,7 +17,6 @@
RE_PARAM = r'%\(([^(%\()]*)\)s'
RE_JINJA = r'{{([^({{)}]*)}}'

RE_PARAM_ALONE = r'^' + RE_PARAM + '$'
RE_JINJA_ALONE = r'^' + RE_JINJA + '$'

# Identify jinja params with no quotes around or complex condition
Expand Down Expand Up @@ -71,26 +71,25 @@ def _render_query(query, parameters):
return {key: _render_query(value, parameters) for key, value in deepcopy(query).items()}
elif isinstance(query, list):
return [_render_query(elt, parameters) for elt in deepcopy(query)]
elif type(query) is str:
elif isinstance(query, str):
if not _has_parameters(query):
return query
clean_p = deepcopy(parameters)

# Replace param templating with jinja templating:
query = re.sub(RE_PARAM, r'{{ \g<1> }}', query)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So easier ^^

By the way, I think we should progressively retire this parameter syntax in favor of Jinja one, which is simpler to read and write. Also, it's complex for users to have two possibilities that does the same thing.
Creating an issue to suggest that: #303


# Add quotes to string parameters to keep type if not complex
if re.match(RE_PARAM_ALONE, query) or re.match(RE_JINJA_ALONE, query):
clean_p = deepcopy(parameters)
if re.match(RE_JINJA_ALONE, query):
clean_p = _prepare_parameters(clean_p)

# Render jinja then render parameters `%()s`
res = Template(query).render(clean_p) % clean_p

# Remove extra quotes with literal_eval
try:
res = ast.literal_eval(res)
if isinstance(res, str):
return res
else:
return _prepare_result(res)
except (SyntaxError, ValueError):
return res
env = NativeEnvironment()
res = env.from_string(query).render(clean_p)
# NativeEnvironment's render() isn't recursive, so we need to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not instantly clear to me what do you mean by "render is not recursive".
May you explain that with an example? Thx

(I tried this to test nested arrays, but obtained as expected:

>>> env.from_string('{{ [punk, [ipa]] }}').render({'punk': ['pale', 'ale'], 'ipa': 10})
[['pale', 'ale'], [10]]

)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure !
There are actually 2 problems :
The 1st one is that render will apply a ast.literal_eval on the result (if the result is a str), and by doing so we lose the original type :

>>> env.from_string('{{ x }}').render({'x': 1})  # we expect 1
1
>>> env.from_string('{{ x }}').render({'x': '1'})  # we expect '1'
1  # <- meh

to handle this problem, a solution is to put strings inside quotes, this is what _prepare_parameters does.

>>> env.from_string('{{ x }}').render({'x': '"1"'})  # we expect '1'
'1'  # <- it works !

it also works if x is a list and we get one element from this list :

>>> env.from_string('{{ x[0] }}').render({'x': ['"1"', '"2"']})  # we expect '1'
'1'

but here's the 2nd problem :

>>> env.from_string('{{ x }}').render({'x': ['"1"', '"2"']})  # we expect ['1', '2']
['"1"', '"2"']  # <- meh

in this case, since the result is a list and not a str, jinja did not apply the literal_eval recursively. That's why we have to do it by hand, by calling _prepare_result.

# apply recursively the literal_eval by hand for lists and dicts:
if isinstance(res, (list, dict)):
return _prepare_result(res)
return res
else:
return query

Expand Down