Fixes and improvements to getbugs() and related methods #160
Conversation
This allows users of python-bugzilla to override the default limit of 20 which can help with network overhead when pulling large amounts of data from bugzilla instances.
The new method `iterbugs()` is analogous to `getbugs()` in that it takes the same parameters and will fetch all bugs requested by the caller, however has two advantages over `getbugs()`: 1. Pagination is done automatically, meaning that `iterbugs()` returns a generator of chunks / pages so that users can simply iterate through the result instead of having to deal with pagination logic themselves. 2. Bug objects are created on demand when iterating through a chunk / page, helping save some memory which can be useful when dealing with large chunks. This commit also performs a mini-refactoring of the logic that converts bug data into a Bug object or None for reusability and clarity.
In `_getbugs()`, the bit of code that ensures that the output order of the bugs is the same as the input order of ids incorrectly checks whether idint / aliaslist are falsy instead of explicitly checking for None. If idlist / aliaslist contain falsy non-None values such as 0 or "", they will skip the check and be potentially incorrectly added to the return list. An example of where this fails would be an idlist = range(0, 20) From tests against the Red Hat bugzilla instance, 0 is not a valid bug_id thus the API will return 1-20, but since idval = 0 will skip the two checks, the end result will be of length 21 (unexpected) and the first bug will be in the return list twice.
| for i in range(0, len(idlist), limit): | ||
| yield ( | ||
| self._to_bug(bug) | ||
| for bug in self._getbugs( | ||
| idlist[i:i + limit], | ||
| include_fields=include_fields, | ||
| exclude_fields=exclude_fields, | ||
| extra_fields=extra_fields, | ||
| permissive=permissive, | ||
| limit=limit, |
There was a problem hiding this comment.
I like the idea of having a generator like this in the library a lot.
If you create the chunks of idlist yourself, the use of limit becomes redundant.
You could use limit in combination with offset, but for really long lists we would pass a lot of unneeded data. May I suggest to undo the changes related to the limit argument and just go for chunked islists?
And if you don't mind, a unittest for this new method would be much appreciated. 🙂
| @@ -1131,20 +1134,52 @@ def getbug(self, objid, | |||
| extra_fields=extra_fields) | |||
| return Bug(self, dict=data, autorefresh=self.bug_autorefresh) | |||
There was a problem hiding this comment.
Let's use your new private method here, too.
| return Bug(self, dict=data, autorefresh=self.bug_autorefresh) | |
| return self._to_bug(bug_data=data, autorefresh=self.bug_autorefresh) |
There was a problem hiding this comment.
Yeah, this _to_bug cleanup is nice. If it was a separate patch I'd apply it
| autorefresh=self.bug_autorefresh)) or None | ||
| for b in data] | ||
| permissive=permissive, limit=limit) | ||
| return [self._to_bug(b) for b in data] |
There was a problem hiding this comment.
When I look at the git history, I see that somebody added the construct (b and Bug(...)) or None deliberately. But from personal usage experience, I do not recall ever seeing a None instead of a Bug instance.
So, I would say, this simplifications makes a lot of sense 👍
@crobinso Would you agree?
There was a problem hiding this comment.
I believe old bugzilla instances would return None if 'permissive=True. Maybe we don't need to maintain it anymore. Safest thing to do is keep it intact unless someone wants to dig through old bugzilla docs.
| @@ -1061,7 +1061,8 @@ def _supports_getbug_extra_fields(self): | |||
|
|
|||
|
|
|||
| def _getbugs(self, idlist, permissive, | |||
| include_fields=None, exclude_fields=None, extra_fields=None): | |||
| include_fields=None, exclude_fields=None, extra_fields=None, | |||
| limit=None): | |||
There was a problem hiding this comment.
It seems like with bugzilla.redhat.com, getbugs does not have a limit anymore (query() still does that). So i think this was a transient change?
| autorefresh=self.bug_autorefresh)) or None | ||
| for b in data] | ||
| permissive=permissive, limit=limit) | ||
| return [self._to_bug(b) for b in data] |
There was a problem hiding this comment.
I believe old bugzilla instances would return None if 'permissive=True. Maybe we don't need to maintain it anymore. Safest thing to do is keep it intact unless someone wants to dig through old bugzilla docs.
| @@ -1131,20 +1134,52 @@ def getbug(self, objid, | |||
| extra_fields=extra_fields) | |||
| return Bug(self, dict=data, autorefresh=self.bug_autorefresh) | |||
There was a problem hiding this comment.
Yeah, this _to_bug cleanup is nice. If it was a separate patch I'd apply it
Yes, I'll take another look in the coming days and address the review feedback |
|
This PR has been stalled for a while, so I'm closing it. @Elkasitu if you are still interested, please feel free to resubmit after addressing the review comments. Thanks! |
See individual commit messages for context