Added search_distributor api and Matcher.less_than matcher #43
Conversation
rajulkumar
commented
Aug 29, 2019
rajulkumar
commented
- search_distributor api to get distributors on defined criteria
- less_than(value) matcher to get objects with field value less than 'value'
Client has a method to search distributors as well. It will return a Page with Distributor objects. Added the field repo_type to Distributor object and updated the schema to validate it.
less_than matcher returns the objects whose field values is less than the matcher value. Dates can also be compared with this matcher. It requires the dates to be in a specific ISO format so as to identify it as a date and generate the criteria accordingly. Also, the fake client is updated for this new matcher. Corresponding tests were added as well.
Added a new api and matcher. So incermented the version number
rohanpm
left a comment
rohanpm
left a comment
There was a problem hiding this comment.
I think when searching for fields stored in Pulp/mongo as a date, we should support passing a datetime value to the API; and we should avoid doing special things for string inputs which "look like" ISO8601 datetimes if at all possible.
pubtools/pulplib/_impl/criteria.py
Outdated
| .. code-block:: python | ||
| # would match where last_publish is before "2019-08-27T00:00:00Z" | ||
| # date comparison requries a dateime.datetime object |
There was a problem hiding this comment.
| # date comparison requries a dateime.datetime object | |
| # date comparison requires a datetime.datetime object |
pubtools/pulplib/_impl/fake/match.py
Outdated
|
|
||
| @visit(LessThanMatcher) | ||
| def match_less(matcher, field, obj): | ||
| # for datetime.dateime fields, obj_value is returned as pulp_value(iso format) |
There was a problem hiding this comment.
| # for datetime.dateime fields, obj_value is returned as pulp_value(iso format) | |
| # for datetime.datetime fields, obj_value is returned as pulp_value(iso format) |
But actually, I don't think it is right to have the datetime conversion embedded directly next to the less-than comparison here. Is there a way to make it work for all of the comparisons like equals, in_ and so on?
There was a problem hiding this comment.
I added bf15429 onto your PR which I believe should fix this problem in a better way, could you please have a look?
There was a problem hiding this comment.
Thanks. This patch gets rid of that obj value vs matcher value inconsistencies.
In the fake client, our logic for deciding whether we had found a model field or a raw Pulp field was: using_model_field = mapped_field is not field But that clearly doesn't work if the model field and the Pulp field happen to have the same name. This would lead to fields wrongly using a Pulp conversion where none is required. Up to now, it seems no fields hit the conditions where this mattered, which is a field having: - the same name on model and in Pulp - and a (nontrivial) conversion required With the addition of Distributor.last_publish as a datetime, this bug now matters and must be fixed. Instead of trying to reuse a single piece of data to mean multiple things, let's just define the function as returning None if no mapping occurred.
some values like datetime.datetime requires transaltion to iso format. this translation was there for an object. this patch extends that to translate the values in list or dict type matcher values too.
distributor object has a field repo_id with the name of the repository the distributor is attached to. the validation checks if the distributor.repo_id is same as the repo.id of the repo this distributor is being attached to else raises an exception.
| # hidden attribute for client attached to this object | ||
|
|
||
| @distributors.validator | ||
| def check_repo_id(self, _, value): |
There was a problem hiding this comment.
Should also be named with _ to make it private API, this method isn't meant to be invoked directly by users.
| def check_repo_id(self, _, value): | |
| def _check_repo_id(self, _, value): |
There was a problem hiding this comment.
I'd have committed this fix myself but it seems like I don't have permission (is the checkbox ticked to allow maintainers to edit this pull request?)
There was a problem hiding this comment.
Yes, it is checked.
| # hidden attribute for client attached to this object | ||
|
|
||
| @distributors.validator | ||
| def check_repo_id(self, _, value): |
There was a problem hiding this comment.
This is the last remaining issue I could see.
2 participants
