launchpad-reviewers team mailing list archive

[Yuliy Schwartzburg <mp+478395@xxxxxxxxxxxxxxxxxx>]

Commented on the while True loop. Do we have tests for this?

Diff comments:

> 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:

Don't really like a `while True`, imagine the API is broken and just keeps returning the same thing. Can we put a certain number of loops here and if it gets to the max we oops.

> +            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


-- 
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.