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

Resolve datetime strings to datetime objects as required by model. #379

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@doobeh
Contributor

doobeh commented Jan 10, 2015

I noticed that when you did when searching, the date search terms would just get forwarded to the database without any attempt at checking their validity-- so something like:

/api/computer?q={"filters":[{"name":"purchase_time", "op":"le", "val":"really-bad-date"}]}

Would just get forwarded on to the database, which means that your query values need to match your database's date/time representations. And at least in the case of sqlite, you'll get results returned for that query mentioned above, which I don't believe should be expected.

I noticed that you use dateutil.parser to handle the date-strings resolution using the string_to_dates, so I adjusted the query handling code to use that existing function to attempt to resolve a date when indicated by the model.

If you now pass a bad date in, the use will get an {"message": "Unable to construct query"} which should be more useful in helping them figure out they've got an error.

In addition, because it's now using dateutil parsing on those params, it allows you to run date queries in a number of formats and get the expected results, eg:

 /api/computer?q={"filters":[{"name":"purchase_time", "op":"le", "val":"2015-01-10T10:49:41.493Z"}]}
 /api/computer?q={"filters":[{"name":"purchase_time", "op":"le", "val":"Sat 10th Jan 2015"}]}
 /api/computer?q={"filters":[{"name":"purchase_time", "op":"le", "val":"Sat, 10 Jan 2015 10:49:41 -0300"}]}
@doobeh

This comment has been minimized.

Contributor

doobeh commented Jan 10, 2015

Ahh, seems like the build is failing. I'll take it back to the drawing board and see what's going on.

@doobeh

This comment has been minimized.

Contributor

doobeh commented Jan 12, 2015

I still need to look into eq matches-- The strings_to_dates seems to always return a datetime, even if the column sqlalchemy column is a date— which means the eq search doesn't get a hit. Plus, I need tests to show this is all actually working.

@doobeh

This comment has been minimized.

Contributor

doobeh commented Jan 12, 2015

I think I'm fairly happy with this pull-request now. It'll now allow you to search in any format that dateutil.parser.parse() supports. I had to add a check to the string_to_dates method that checks to see if the orm column is just a Date, if so, it crops the DateTime object to a Date, this is because I was running into a problem where if I queried Lincolns birthday, strings_to_dates would change my 1900-01-02 to a datetime.datetime(1900,1,2,0,0) object which would then cause the query to return nothing.

Please let me know your thoughts when you're able.

resp = self.app.search('/api/person', dumps(search))
assert loads(resp.data)['name'] == u'Lincoln'

This comment has been minimized.

@jfinkels

jfinkels Jan 12, 2015

Owner

Leave only one space between function definitions.

@@ -1058,6 +1061,31 @@ def _search(self):
for preprocessor in self.preprocessors['GET_MANY']:
preprocessor(search_params=search_params)
# resolve date-strings as required by the model
for param in search_params.get("filters", list()):

This comment has been minimized.

@jfinkels

jfinkels Jan 12, 2015

Owner

Use single quotes for string literals to match the rest of the code. (This comment applies to a few other lines as well.)

@@ -1058,6 +1061,31 @@ def _search(self):
for preprocessor in self.preprocessors['GET_MANY']:
preprocessor(search_params=search_params)
# resolve date-strings as required by the model
for param in search_params.get("filters", list()):
if all(key in param for key in ('name', 'val')):

This comment has been minimized.

@jfinkels

jfinkels Jan 12, 2015

Owner

Two conditions is not enough to warrant an all invocation. I prefer

if 'name' in param and 'val' in param:

for its simplicity and readability.

@jfinkels

This comment has been minimized.

Owner

jfinkels commented Feb 3, 2015

This looks good. Can you please squash your six commits on this branch into a single commit? Then I will merge it.

@doobeh

This comment has been minimized.

Contributor

doobeh commented Feb 17, 2015

Well, I obviously don't get the idea of squashing quite right-- if you can't work with the above let me know and I'll work out how I'm meant to handle it in Pycharm correctly.

@jfinkels

This comment has been minimized.

Owner

jfinkels commented Feb 17, 2015

I'll just rebase it myself, don't worry about it :)

I don't use PyCharm, but if you want to try it from the command line next time, git rebase -i master.

@jfinkels

This comment has been minimized.

Owner

jfinkels commented Feb 18, 2015

I squashed your commits into 62cfcea. They now appear in the master branch. Thanks!

@jfinkels jfinkels closed this Feb 18, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment