Ensure group_by skips groups with null values

backdrop/core/storage/postgres.py

@@ -194,8 +194,13 @@ def _get_grouped_postgres_query(self, data_set_id, query, groups_lookup):
                 ", ".join([elem for elem in [
                     "count(*) as _count",
                     self._get_period_group(query),
-                ] + groups.values() if elem]),
+
+                ] + [
+                    # Fine not to use mogrify here too, because everything has been mogrified already
+                    "%s as %s" % (value, key) for key, value in groups.iteritems()
+                ] if elem]),
                 "FROM mongo",
+                self._get_groups_not_null_conditional(psql_cursor, groups.values()),
                 # query.period.name is not user input, so no need to mogrify this
                 "GROUP BY" if query.period or query.group_by else "",
                 ", ".join([elem for elem in [
@@ -209,6 +214,15 @@ def _get_period_group(self, query):
             return "date_trunc('{period}', timestamp) as _{period}_start_at".format(period = query.period.name)
         return ''
 
+    def _get_groups_not_null_conditional(self, psql_cursor, group_values):
+        if group_values:
+            return "WHERE " + " AND ".join([
+                # keys are `record_XX` not user input, so no need to mogrify this
+                "%s IS NOT NULL" % value for value in group_values
+            ])
+        else:
+            return ""
+
     def _get_groups_lookup(self, query):
         """
         Returns a dictionary of sql friendly name to user-provided name
@@ -227,7 +241,7 @@ def _get_groups_lookup(self, query):
     def _get_groups(self, psql_cursor, groups_lookup):
         return {
             key: psql_cursor.mogrify(
-                "record->%(field)s as {key}".format(key=key),
+                "record->%(field)s",
                 {'field':value}
             )
             for key, value in groups_lookup.iteritems()

tests/core/storage/test_postgres.py

@@ -55,7 +55,7 @@ def test_get_group_by_record_and_period_postgres_query(self):
         )
         assert_that(
             result,
-            is_("SELECT count(*) as _count, date_trunc('week', timestamp) as _week_start_at, record->'foo' as record_0 FROM mongo GROUP BY _week_start_at, record_0")
+            is_("SELECT count(*) as _count, date_trunc('week', timestamp) as _week_start_at, record->'foo' as record_0 FROM mongo WHERE record->'foo' IS NOT NULL GROUP BY _week_start_at, record_0")
         )
 
     @unittest.skip('The postgres datastore does not support the creation of empty datasets')