🐛 FIX: REST API date-time query

[NinadBhat]

Fixes #4957

Added hashing function for DatetimePrecision object.

@chrisjsewell Not sure if this is entirely right, but this method solved the error for me.

[codecov]

Codecov Report

Merging #4959 (149daa6) into develop (6606a5b) will increase coverage by 0.06%.
The diff coverage is 84.62%.

@@             Coverage Diff             @@
##           develop    #4959      +/-   ##
===========================================
+ Coverage    80.17%   80.23%   +0.06%     
===========================================
  Files          515      515              
  Lines        36741    36746       +5     
===========================================
+ Hits         29454    29478      +24     
+ Misses        7287     7268      -19     

Flag	Coverage Δ
django	74.70% <84.62%> (+0.06%)	⬆️
sqlalchemy	73.63% <84.62%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/common/utils.py	79.65% <77.78%> (-0.07%)	⬇️
aiida/common/hashing.py	93.81% <100.00%> (+0.17%)	⬆️
aiida/restapi/common/utils.py	78.98% <100.00%> (+4.91%)	⬆️
aiida/transports/plugins/local.py	81.41% <0.00%> (-0.25%)	⬇️
...iida/orm/implementation/sqlalchemy/querybuilder.py	81.61% <0.00%> (+0.58%)	⬆️
aiida/orm/implementation/django/querybuilder.py	77.85% <0.00%> (+0.64%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6606a5b...149daa6. Read the comment docs.

[chrisjsewell]

Thanks @NinadBhat!
The fix looks fine, I think though it would be good to also add a test in tests/restapi/test_routes.py:RESTApiTestSuite, with an mtime/ctime filter on nodes

[unkcpz]

Here are some questions from my view:

1. We already know the DatetimePrecision has a datatime and an int as its objects, then is there any particular reason that you dispatch it to return like a abc.Sequence instead of dispatch it like numbers.Complex which also defined by two auguments.
2. I would suggest moving the DatatimePrecision definition from aiida.restapi.common.utils to aiida.common. Since aiida.common is a more 'internal' module then aiida.restapi. or maybe can put new code in aiida.restapi.common.utils and leave aiida.common untouched?
3. I believe would be also good to add test in tests/common/test_hashing.py.

[unkcpz]

just look into the tests/common/test_hashing.py, since there are lots of tests assertion which compare expected with hash strings, it would be suitable to use pytest-regression to do this. But this will be in other PR. Also looking for your opinion @chrisjsewell .

[NinadBhat]

@chrisjsewell, the one test failing is because docker does not install flask. Not sure where I should add flask dependency for it to be installed with docker.

[unkcpz]

the one test failing is because docker does not install flask. Not sure where I should add flask dependency for it to be installed with docker.

From my point of view, I think this is a side effect that I mentioned, where you import restapi.common in aiida.common. In fact, aiida.common would be better to know nothing about aiida.restapi I guess.

[NinadBhat]

@unkcpz makes sense. Thanks for the quick reply. @chrisjsewell should I move DatatimePrecision to aiida.common ?

[chrisjsewell]

should I move DatatimePrecision to aiida.common

Yeh I don't think that is a problem; it is a very simple class with no dependencies

We already know the DatetimePrecision has a datatime and an int as its objects, then is there any particular reason that you dispatch it to return like a abc.Sequence instead of dispatch it like numbers.Complex which also defined by two auguments.

meh, I'm not too bothered either (particularly as we are likely going to scrap all the current restapi code soon 😆). It would actually be kind of nice to know where/why it is actually being hashed in the first place; from a quick look in the code, this was not immediately apparent (but don't worry about spending too long on this)

it would be suitable to use pytest-regression to do this. But this will be in other PR. Also looking for your opinion @chrisjsewell .

yeh always happy to use pytest-regression 👍 , although I don't think its a test dependency yet so yeh wouldn't add it in this PR