Changeset View
Changeset View
Standalone View
Standalone View
swh/storage/db.py
Show First 20 Lines • Show All 425 Lines • ▼ Show 20 Lines | def origin_visit_exists(self, origin_id, visit_id, cur=None): | ||||
cur = self._cursor(cur) | cur = self._cursor(cur) | ||||
query = "SELECT 1 FROM origin_visit where origin = %s AND visit = %s" | query = "SELECT 1 FROM origin_visit where origin = %s AND visit = %s" | ||||
cur.execute(query, (origin_id, visit_id)) | cur.execute(query, (origin_id, visit_id)) | ||||
return bool(cur.fetchone()) | return bool(cur.fetchone()) | ||||
def origin_visit_get_latest_snapshot(self, origin_id, | def origin_visit_get_latest( | ||||
allowed_statuses=None, | self, origin_id, allowed_statuses=None, require_snapshot=False, | ||||
cur=None): | cur=None): | ||||
"""Retrieve the most recent origin_visit which references a snapshot | """Retrieve the most recent origin_visit of the given origin, | ||||
with optional filters. | |||||
douardda: This doc string is unclear and misleading IMHO.
This is not what this method does. If I… | |||||
Args: | Args: | ||||
origin_id: the origin concerned | origin_id: the origin concerned | ||||
allowed_statuses: the visit statuses allowed for the returned visit | allowed_statuses: the visit statuses allowed for the returned visit | ||||
require_snapshot (bool): If True, only a visit with a known | |||||
snapshot will be returned. | |||||
Returns: | Returns: | ||||
The origin_visit information, or None if no visit matches. | The origin_visit information, or None if no visit matches. | ||||
""" | """ | ||||
cur = self._cursor(cur) | cur = self._cursor(cur) | ||||
extra_clause = "" | query_parts = [ | ||||
'SELECT %s' % ', '.join(self.origin_visit_get_cols), | |||||
'FROM origin_visit'] | |||||
if require_snapshot: | |||||
# Makes sure the snapshot is known | |||||
query_parts.append( | |||||
'INNER JOIN snapshot ON (origin_visit.snapshot=snapshot.id)') | |||||
query_parts.append('WHERE origin = %s') | |||||
if require_snapshot: | |||||
Not Done Inline ActionsI'm a noob in sql, but isn't there a JOIN variant with which this condition is not needed? douardda: I'm a noob in sql, but isn't there a JOIN variant with which this condition is not needed? | |||||
Done Inline Actionsthat's indeed not needed for an inner join. I'm not sure why it was there. vlorentz: that's indeed not needed for an inner join. I'm not sure why it was there. | |||||
Done Inline ActionsIt turns out that the inner join shouldn't be there. I'll remove it in another diff (because it changes the behavior of the function) vlorentz: It turns out that the inner join shouldn't be there. I'll remove it in another diff (because it… | |||||
query_parts.append('AND snapshot is not null') | |||||
if allowed_statuses: | if allowed_statuses: | ||||
extra_clause = cur.mogrify("AND status IN %s", | query_parts.append( | ||||
(tuple(allowed_statuses),)).decode() | cur.mogrify('AND status IN %s', | ||||
(tuple(allowed_statuses),)).decode()) | |||||
query = """\ | query_parts.append('ORDER BY date DESC, visit DESC LIMIT 1') | ||||
SELECT %s | |||||
FROM origin_visit | query = '\n'.join(query_parts) | ||||
INNER JOIN snapshot ON (origin_visit.snapshot=snapshot.id) | |||||
WHERE | |||||
origin = %%s AND snapshot is not null %s | |||||
ORDER BY date DESC, visit DESC | |||||
LIMIT 1 | |||||
""" % (', '.join(self.origin_visit_get_cols), extra_clause) | |||||
cur.execute(query, (origin_id,)) | cur.execute(query, (origin_id,)) | ||||
r = cur.fetchone() | r = cur.fetchone() | ||||
if not r: | if not r: | ||||
return None | return None | ||||
return r | return r | ||||
@staticmethod | @staticmethod | ||||
▲ Show 20 Lines • Show All 447 Lines • Show Last 20 Lines |
This doc string is unclear and misleading IMHO.
This is not what this method does. If I understand correctly the code, it retrieve the most recent origin visit for a given origin, potentially limited to visits with a related snapshot.
Written as it is (and was before this diff, which is a good moment to fix this), I understand it looks for visits for a given snapshot.
Just noticed docstrings in subclasses seem more accurate in this regard.
We really should not have to duplicate those docstrings everywhere, it's a pain in the neck to manage...