Merge ~philroche/simplestreams/+git/simplestreams:feture/filter-items-before-filter-versions into simplestreams:master

[Philip Roche]

Apply filters to items before filtering versions

Filters were applied to versions before being applied to
items which meant if you were using --max option then
you often were not getting the expected output as the
version filter was filtering out the expected items.

This happens if there is a version greater than the version
of the item you expect despite the fact that the item
with the greater version never passing your supplied
filters.

Apply filters to items before filtering versions

Filters were applied to versions before being applied to
items which meant if you were using --max option then
you often were not getting the expected output as the
version filter was filtering out the expected items.

This happens if there is a version greater than the version
of the item you expect despite the fact that the item
with the greater version never passing your supplied
filters.

[Philip Roche (philroche)]

An example query where the problem this commit fixes is

```
sstream-query --max=1 -U http://cloud-images.ubuntu.com/releases/streams/v1/com.ubuntu.cloud:released:aws-cn.sjson crsn=cn-north-1 release=xenial arch=amd64 virt=hvm root_store=ssd --output-format="16.04 %(version_name)s %(arch)s %(id)s %(root_store)s %(virt)s"
```

This would return no results despite there being 41 images that satisfy those filters.

[Cody Shepherd (codyshepherd)]

review: Approve

[Tobias Koch (tobijk)]

Nice work figuring this out!

review: Approve

[Scott Moser (smoser)]

I don't see any obvious holes in the implementation, but this was kind of functioning as designed. It seems you'd only hit this if "the version greater than the item you expect" had a subset of the items that you expected.

Why would that be?

Within a given product, I would not have expected that there was any case where a newer version contained a subset of an older version *AND* that older version's content was still desirable.

One solution would be to just fix your published data to have "full" content.

[Philip Roche (philroche)]

Thank you for your comment.

I take your point about publishing full content but this is not always possible to do at the same time, for example publishing to aws-cn partition is very error prone and can result in serials not being published to all regions. This shouldn't mean that streams start returning incorrect results though.

Eventually full content would be published but this MP fixes the issue of incorrect results being returned in the interim.

As you say, the older versions content is not desirable but we do require that content to be filterable instead of returning no results.

[Scott Moser (smoser)]

> Thank you for your comment.
>
> I take your point about publishing full content but this is not always
> possible to do at the same time, for example publishing to aws-cn partition is
> very error prone and can result in serials not being published to all regions.
> This shouldn't mean that streams start returning incorrect results though.
>
> Eventually full content would be published but this MP fixes the issue of

> incorrect results being returned in the interim.

Well... you clearly do not have to publish data until it is correct and
complete. I'm not going to argue all that much here, but I think it could
be argued that you're just taking a shortcut in publishing of data that
pushes complexity on to the consumer.

>
> As you say, the older versions content is not desirable but we do require that
> content to be filterable instead of returning no results.

Also... for us pesky humans, it might be nice to dump your json data with
sorted keys. (json.dumps(data, sort_keys=True))

[Philip Roche (philroche)]

I do agree that all data should be complete but when publication fails in one region for reasons outside of our control it shouldn't block updating of streams, especially if the publication is to address a CVE.

I will add sorting of streams to our process.

[Robert C Jennings (rcj)]

The changes look good. It should not occur that publication is incomplete but the ability to quickly service CVEs with images as they become available is important.

review: Approve

[Server Team CI bot (server-team-bot)]

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

Commit message lints:
 - Line #3 has 1 too many characters. Line starts with: "if you were using --max"... - Line #6 has 8 too many characters. Line starts with: "This happens if there"... - Line #7 has 18 too many characters. Line starts with: "despite the fact that"...

review: Needs Fixing

[Philip Roche (philroche)]

Commit message now updated

[Server Team CI bot (server-team-bot)]

review: Approve (continuous-integration)

diff --git a/simplestreams/mirrors/__init__.py b/simplestreams/mirrors/__init__.py
index 2a1dc7e..60d5ade 100644
--- a/simplestreams/mirrors/__init__.py
+++ b/simplestreams/mirrors/__init__.py
@@ -280,6 +280,23 @@ class BasicMirrorWriter(MirrorWriter):
 
         filtered_products = []
         prodname = None
+
+        # Apply filters to items before filtering versions
+        for prodname, product in list(stree.items()):
+
+            for vername, version in list(product.get('versions', {}).items()):
+                for itemname, item in list(version.get('items', {}).items()):
+                    pgree = (prodname, vername, itemname)
+                    if not self.filter_item(item, src, target, pgree):
+                        LOG.debug("Filtered out item: %s/%s", itemname, item)
+                        del stree[prodname]['versions'][vername]['items'][
+                            itemname]
+                        if not stree[prodname]['versions'][vername].get(
+                                'items', {}):
+                            del stree[prodname]['versions'][vername]
+                        if not stree[prodname].get('versions', {}):
+                            del stree[prodname]
+
         for prodname, product in stree.items():
             if not self.filter_product(product, src, target, (prodname,)):
                 filtered_products.append(prodname)
@@ -320,9 +337,6 @@ class BasicMirrorWriter(MirrorWriter):
                 added_items = []
                 for itemname, item in version.get('items', {}).items():
                     pgree = (prodname, vername, itemname)
-                    if not self.filter_item(item, src, target, pgree):
-                        LOG.debug("Filtered out item: %s/%s", itemname, item)
-                        continue
 
                     added_items.append(itemname)