launchpad-reviewers team mailing list archive

[Simone Pelosi <mp+478395@xxxxxxxxxxxxxxxxxx>]

Simone Pelosi has proposed merging ~pelpsi/launchpad:artifactory-properties-missing into launchpad:master.

Commit message:
Artifactory properties missing
    
We should consume the pool until it's empty,
we use a lower limit with an offset to process chunks of artifacts,
avoiding timeouts and limits on jfrog platform.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/478395
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:artifactory-properties-missing into launchpad:master.

diff --git a/lib/lp/archivepublisher/artifactory.py b/lib/lp/archivepublisher/artifactory.py
index 08eb83a..413cef3 100644
--- a/lib/lp/archivepublisher/artifactory.py
+++ b/lib/lp/archivepublisher/artifactory.py
@@ -636,31 +636,43 @@ class ArtifactoryPool:
         # length):
         #   https://www.jfrog.com/confluence/display/JFROG/\
         #     Artifactory+Query+Language
-        artifacts = self.rootpath.aql(
-            "items.find",
-            {
-                "repo": repository_name,
-                "$or": [
-                    {"name": {"$match": pattern}}
-                    for pattern in self.getArtifactPatterns(repository_format)
-                ],
-            },
-            ".include",
-            # We don't use "repo", but the AQL documentation says that
-            # non-admin users must include all of "name", "repo", and "path"
-            # in the include directive.
-            ["repo", "path", "name", "property"],
-        )
+
+        offset = 0
+        limit = 100000
         artifacts_by_path = {}
-        for artifact in artifacts:
-            path = PurePath(artifact["path"], artifact["name"])
-            properties = defaultdict(set)
-            for prop in artifact["properties"]:
-                properties[prop["key"]].add(prop.get("value", ""))
-            # AQL returns each value of multi-value properties separately
-            # and in an undefined order.  Always sort them to ensure that we
-            # can compare properties reliably.
-            artifacts_by_path[path] = {
-                key: sorted(values) for key, values in properties.items()
-            }
+
+        while True:
+            artifacts = self.rootpath.aql(
+                "items.find",
+                {
+                    "repo": repository_name,
+                    "$or": [
+                        {"name": {"$match": pattern}}
+                        for pattern in self.getArtifactPatterns(
+                            repository_format
+                        )
+                    ],
+                },
+                ".include",
+                # We don't use "repo", but the AQL documentation says that
+                # non-admin users must include all of "name", "repo",
+                # and "path" in the include directive.
+                ["repo", "path", "name", "property"],
+                ".offset(%s)" % offset,
+                ".limit(%s)" % limit,
+            )
+            if len(artifacts) == 0:
+                break
+            for artifact in artifacts:
+                path = PurePath(artifact["path"], artifact["name"])
+                properties = defaultdict(set)
+                for prop in artifact["properties"]:
+                    properties[prop["key"]].add(prop.get("value", ""))
+                # AQL returns each value of multi-value properties separately
+                # and in an undefined order.  Always sort them to ensure that
+                # we can compare properties reliably.
+                artifacts_by_path[path] = {
+                    key: sorted(values) for key, values in properties.items()
+                }
+            offset += limit
         return artifacts_by_path
diff --git a/lib/lp/archivepublisher/tests/artifactory_fixture.py b/lib/lp/archivepublisher/tests/artifactory_fixture.py
index 95f6d75..7b20a87 100644
--- a/lib/lp/archivepublisher/tests/artifactory_fixture.py
+++ b/lib/lp/archivepublisher/tests/artifactory_fixture.py
@@ -272,12 +272,25 @@ class FakeArtifactoryFixture(Fixture):
             return 400, {}, ""
         # Treating this as JSON is cheating a bit, but it works.
         criteria = json.loads(match.group(1))
+
+        pattern = r"\.offset\((\d+)\)\.limit\((\d+)\)"
+
+        offset = 0
+        limit = 1000
+        match = re.search(pattern, request.body)
+
+        if match:
+            offset = int(match.group(1))  # Group 1 is the offset value
+            limit = int(match.group(2))
+
         items = [
             self._make_aql_item(path)
             for path in sorted(self._fs)
             if "size" in self._fs[path]
         ]
         results = [item for item in items if self._matches_aql(item, criteria)]
+        limit = min(limit, len(results) - offset)
+        results = results[offset : offset + limit]
         return 200, {}, json.dumps({"results": results})
 
     def _handle_delete(self, request):