Refactor sort/filter code once again

Now we handle sorting on the bot side, but still filter in the database

PluralKit.Bot/Commands/SystemList.cs

@@ -1,29 +1,18 @@
-using System.Collections.Generic;
 using System.Linq;
 using System.Text;
 using System.Threading.Tasks;
 
-using Dapper;
-
-using NodaTime;
-
 using PluralKit.Core;
 
-using Serilog;
-
 namespace PluralKit.Bot
 {
     public class SystemList
     {
-        private readonly IClock _clock;
         private readonly IDatabase _db;
-        private readonly ILogger _logger;
         
-        public SystemList(IDatabase db, ILogger logger, IClock clock)
+        public SystemList(IDatabase db)
         {
             _db = db;
-            _logger = logger;
-            _clock = clock;
         }
 
         public async Task MemberList(Context ctx, PKSystem target)
@@ -34,7 +23,8 @@ namespace PluralKit.Bot
             // GetRendererFor must be called before GetOptions as it consumes a potential positional full argument that'd otherwise land in the filter
             var renderer = GetRendererFor(ctx);
             var opts = GetOptions(ctx, target);
-            var members = await GetMemberList(target, opts);
+            
+            var members = (await _db.Execute(c => opts.Execute(c, target))).ToList();
             await ctx.Paginate(
                 members.ToAsyncEnumerable(),
                 members.Count,
@@ -43,27 +33,11 @@ namespace PluralKit.Bot
                 (eb, ms) =>
                 {
                     eb.WithFooter($"{opts.CreateFilterString()}. {members.Count} results.");
-                    renderer.RenderPage(eb, ctx.System, ms);
+                    renderer.RenderPage(eb, ctx.System.Zone, ms);
                     return Task.CompletedTask;
                 });
         }
 
-        private async Task<IReadOnlyList<PKListMember>> GetMemberList(PKSystem target, SortFilterOptions opts)
-        {
-            await using var conn = await _db.Obtain();
-            var query = opts.BuildQuery();
-            var args = new {System = target.Id, opts.Filter};
-            _logger.Debug("Executing sort/filter query `{Query}` with arguments {Args}", query, args);
-            
-            var timeBefore = _clock.GetCurrentInstant();
-            var results = (await conn.QueryAsync<PKListMember>(query, args)).ToList();
-            var timeAfter = _clock.GetCurrentInstant();
-            
-            _logger.Debug("Executed sort/filter query `{Query}` returning {ResultCount} results in {QueryTime}", query, results.Count, timeAfter - timeBefore);
-
-            return results;
-        }
-
         private string GetEmbedTitle(PKSystem target, SortFilterOptions opts)
         {
             var title = new StringBuilder("Members of ");

PluralKit.Bot/Lists/IListRenderer.cs

@@ -2,6 +2,8 @@
 
 using DSharpPlus.Entities;
 
+using NodaTime;
+
 using PluralKit.Core;
 
 namespace PluralKit.Bot
@@ -9,6 +11,6 @@ namespace PluralKit.Bot
     public interface IListRenderer
     {
         int MembersPerPage { get; }
-        void RenderPage(DiscordEmbedBuilder eb, PKSystem system, IEnumerable<PKListMember> members);
+        void RenderPage(DiscordEmbedBuilder eb, DateTimeZone zone, IEnumerable<ListedMember> members);
     }
 }

PluralKit.Bot/Lists/LongRenderer.cs

@@ -1,5 +1,4 @@
-using System;
+using System.Collections.Generic;
-using System.Collections.Generic;
 
 using DSharpPlus.Entities;
 
@@ -21,8 +20,10 @@ namespace PluralKit.Bot
             _fields = fields;
         }
 
-        public void RenderPage(DiscordEmbedBuilder eb, PKSystem system, IEnumerable<PKListMember> members)
+        public void RenderPage(DiscordEmbedBuilder eb, DateTimeZone zone, IEnumerable<ListedMember> members)
         {
+            string FormatTimestamp(Instant timestamp) => DateTimeFormats.ZonedDateTimeFormat.Format(timestamp.InZone(zone));
+
             foreach (var m in members)
             {
                 var profile = $"**ID**: {m.Hid}";
@@ -31,8 +32,8 @@ namespace PluralKit.Bot
                 if (_fields.ShowBirthday && m.Birthday != null) profile += $"\n**Birthdate**: {m.BirthdayString}";
                 if (_fields.ShowPronouns && m.ProxyTags.Count > 0) profile += $"\n**Proxy tags:** {m.ProxyTagsString()}";
                 if (_fields.ShowMessageCount && m.MessageCount > 0) profile += $"\n**Message count:** {m.MessageCount}";
-                if (_fields.ShowLastMessage && m.LastMessage != null) profile += $"\n**Last message:** {FormatTimestamp(system, DiscordUtils.SnowflakeToInstant(m.LastMessage.Value))}";
+                if (_fields.ShowLastMessage && m.LastMessage != null) profile += $"\n**Last message:** {FormatTimestamp(DiscordUtils.SnowflakeToInstant(m.LastMessage.Value))}";
-                if (_fields.ShowLastSwitch && m.LastSwitchTime != null) profile += $"\n**Last switched in:** {FormatTimestamp(system, m.LastSwitchTime.Value)}";
+                if (_fields.ShowLastSwitch && m.LastSwitchTime != null) profile += $"\n**Last switched in:** {FormatTimestamp(m.LastSwitchTime.Value)}";
                 if (_fields.ShowDescription && m.Description != null) profile += $"\n\n{m.Description}";
                 if (_fields.ShowPrivacy && m.MemberPrivacy == PrivacyLevel.Private)
                     profile += "\n*(this member is private)*";
@@ -41,8 +42,6 @@ namespace PluralKit.Bot
             }
         }
         
-        private static string FormatTimestamp(PKSystem system, Instant timestamp) => DateTimeFormats.ZonedDateTimeFormat.Format(timestamp.InZone(system.Zone ?? DateTimeZone.Utc));
-
         public class MemberFields
         {
             public bool ShowDisplayName = true;

PluralKit.Bot/Lists/PKListMember.cs

@@ -1,12 +0,0 @@
-using NodaTime;
-
-using PluralKit.Core;
-
-namespace PluralKit.Bot
-{
-    public class PKListMember: PKMember
-    {
-        public ulong? LastMessage { get; set; }
-        public Instant? LastSwitchTime { get; set; }
-    }
-}

PluralKit.Bot/Lists/ShortRenderer.cs

@@ -3,6 +3,8 @@ using System.Text;
 
 using DSharpPlus.Entities;
 
+using NodaTime;
+
 using PluralKit.Core;
 
 namespace PluralKit.Bot
@@ -11,9 +13,9 @@ namespace PluralKit.Bot
     {
         public int MembersPerPage => 25;
         
-        public void RenderPage(DiscordEmbedBuilder eb, PKSystem system, IEnumerable<PKListMember> members)
+        public void RenderPage(DiscordEmbedBuilder eb, DateTimeZone timezone, IEnumerable<ListedMember> members)
         {
-            string RenderLine(PKListMember m)
+            string RenderLine(ListedMember m)
             {
                 if (m.HasProxyTags)
                 {

PluralKit.Bot/Lists/SortFilterOptions.cs

@@ -1,5 +1,11 @@
 using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Linq;
 using System.Text;
+using System.Threading.Tasks;
+
+using NodaTime;
 
 using PluralKit.Core;
 
@@ -15,7 +21,7 @@ namespace PluralKit.Bot
 
         public string CreateFilterString()
         {
-            StringBuilder str = new StringBuilder();
+            var str = new StringBuilder();
             str.Append("Sorting by ");
             str.Append(SortProperty switch
             {
@@ -47,66 +53,50 @@ namespace PluralKit.Bot
             return str.ToString();
         }
         
-        public string BuildQuery()
+        public async Task<IEnumerable<ListedMember>> Execute(IPKConnection conn, PKSystem system)
         {
-            // For best performance, add index:
+            var filtered = await QueryWithFilter(conn, system);
-            // - `on switch_members using btree (member asc nulls last) include (switch);`
+            return Sort(filtered);
-            // TODO: add a migration adding this, perhaps lumped with the rest of the DB changes (it's there in prod)
+        }
 
-            // Select clause
+        private Task<IEnumerable<ListedMember>> QueryWithFilter(IPKConnection conn, PKSystem system) =>
-            StringBuilder query = new StringBuilder("select * from member_list");
+            conn.QueryMemberList(system.Id, PrivacyFilter switch
-
-            // Filtering
-            query.Append(" where system = @System");
-            query.Append(PrivacyFilter switch
             {
-                PrivacyFilter.PrivateOnly => $" and member_privacy = {(int) PrivacyLevel.Private}",
+                PrivacyFilter.PrivateOnly => PrivacyLevel.Private,
-                PrivacyFilter.PublicOnly => $" and member_privacy = {(int) PrivacyLevel.Public}",
+                PrivacyFilter.PublicOnly => PrivacyLevel.Public,
-                _ => ""
+                PrivacyFilter.All => null
-            });
+            }, Filter, SearchInDescription);
 
-            // String filter
+        private IEnumerable<ListedMember> Sort(IEnumerable<ListedMember> input)
-            if (Filter != null)
+        {
+            IComparer<T> ReverseMaybe<T>(IComparer<T> c) =>
+                Reverse ? Comparer<T>.Create((a, b) => c.Compare(b, a)) : c;
+            
+            var culture = StringComparer.InvariantCultureIgnoreCase;
+            return (SortProperty switch
             {
-                // Use position rather than ilike to not bother with escaping and such
+                // As for the OrderByDescending HasValue calls: https://www.jerriepelser.com/blog/orderby-with-null-values/
-                query.Append(" and (");
+                // We want nulls last no matter what, even if orders are reversed
-                query.Append(
+                SortProperty.Hid => input.OrderBy(m => m.Hid, ReverseMaybe(culture)),
-                    "position(lower(@Filter) in lower(name)) > 0 or position(lower(@Filter) in lower(coalesce(display_name, ''))) > 0");
+                SortProperty.Name => input.OrderBy(m => m.Name, ReverseMaybe(culture)),
-                if (SearchInDescription)
+                SortProperty.CreationDate => input.OrderBy(m => m.Created, ReverseMaybe(Comparer<Instant>.Default)),
-                    query.Append(" or position(lower(@Filter) in lower(coalesce(description, ''))) > 0");
+                SortProperty.MessageCount => input.OrderByDescending(m => m.MessageCount, ReverseMaybe(Comparer<int>.Default)),
-                query.Append(")");
+                SortProperty.DisplayName => input
-            }
+                    .OrderByDescending(m => m.DisplayName != null)
-            
+                    .ThenBy(m => m.DisplayName, ReverseMaybe(culture)),
-            // Order direction
+                SortProperty.Birthdate => input
-            var direction = SortProperty switch
+                    .OrderByDescending(m => m.AnnualBirthday.HasValue)
-            {
+                    .ThenBy(m => m.AnnualBirthday, ReverseMaybe(Comparer<AnnualDate?>.Default)),
-                // Some of these make more "logical sense" as descending (ie. "last message" = descending order of message timestamp/ID)
+                SortProperty.LastMessage => input
-                SortProperty.MessageCount => SortDirection.Descending,
+                    .OrderByDescending(m => m.LastMessage.HasValue)
-                SortProperty.LastMessage => SortDirection.Descending,
+                    .ThenByDescending(m => m.LastMessage, ReverseMaybe(Comparer<ulong?>.Default)),
-                SortProperty.LastSwitch => SortDirection.Descending,
+                SortProperty.LastSwitch => input
-                _ => SortDirection.Ascending
+                    .OrderByDescending(m => m.LastSwitchTime.HasValue)
-            };
+                    .ThenByDescending(m => m.LastSwitchTime, ReverseMaybe(Comparer<Instant?>.Default)),
-            if (Reverse) direction = direction == SortDirection.Ascending ? SortDirection.Descending : SortDirection.Ascending;
+                _ => throw new ArgumentOutOfRangeException($"Unknown sort property {SortProperty}")
-            var order = direction == SortDirection.Ascending ? "asc" : "desc";
+            })
-            
+                // Lastly, add a by-name fallback order for collisions (generally hits w/ lots of null values)
-            // Order clause
+                .ThenBy(m => m.Name, culture);
-            const string fallback = "name collate \"C\" asc"; // how to handle null values
-            query.Append(" order by ");
-            query.Append(SortProperty switch
-            {
-                // Name/DN order needs `collate "C"` to match legacy .NET behavior (affects sorting of emojis, etc)
-                SortProperty.Name => $"name collate \"C\" {order}",
-                SortProperty.DisplayName => $"display_name collate \"C\" {order}, name collate \"C\" {order}",
-                SortProperty.Hid => $"hid {order}",
-                SortProperty.CreationDate => $"created {order}",
-                SortProperty.Birthdate => $"birthday_md {order} nulls last, {fallback}",
-                SortProperty.MessageCount => $"message_count {order} nulls last, {fallback}",
-                SortProperty.LastMessage => $"last_message {order} nulls last, {fallback}",
-                SortProperty.LastSwitch => $"last_switch_time {order} nulls last, {fallback}",
-                _ => throw new ArgumentOutOfRangeException($"Couldn't find order clause for sort property {SortProperty}")
-            });
-
-            return query.ToString();
         }
 
         public static SortFilterOptions FromFlags(Context ctx)
@@ -138,7 +128,6 @@ namespace PluralKit.Bot
             
             return p;
         }
-        
     }
 
     public enum SortProperty
@@ -153,12 +142,6 @@ namespace PluralKit.Bot
         Birthdate
     }
 
-    public enum SortDirection
-    {
-        Ascending,
-        Descending
-    }
-
     public enum PrivacyFilter
     {
         All,

PluralKit.Core/Database/Database.cs

@@ -82,7 +82,7 @@ namespace PluralKit.Core
             await ApplyMigrations(conn, tx);
 
             // Now, reapply views/functions (we deleted them above, no need to worry about conflicts)
-            await ExecuteSqlFile($"{RootPath}.views.sql", conn, tx);
+            await ExecuteSqlFile($"{RootPath}.Views.views.sql", conn, tx);
             await ExecuteSqlFile($"{RootPath}.Functions.functions.sql", conn, tx);
 
             // Finally, commit tx

PluralKit.Core/Database/Views/DatabaseViewsExt.cs

@@ -0,0 +1,34 @@
+#nullable enable
+using System.Collections.Generic;
+using System.Text;
+using System.Threading.Tasks;
+
+using Dapper;
+
+namespace PluralKit.Core
+{
+    public static class DatabaseViewsExt
+    {
+        public static Task<IEnumerable<SystemFronter>> QueryCurrentFronters(this IPKConnection conn, int system) =>
+            conn.QueryAsync<SystemFronter>("select * from system_fronters where system = @system", new {system});
+
+        public static Task<IEnumerable<ListedMember>> QueryMemberList(this IPKConnection conn, int system, PrivacyLevel? privacyFilter = null, string? filter = null, bool includeDescriptionInNameFilter = false)
+        {
+            StringBuilder query = new StringBuilder("select * from member_list where system = @system");
+
+            if (privacyFilter != null)
+                query.Append($" and member_privacy = {(int) privacyFilter}");
+
+            if (filter != null)
+            {
+                static string Filter(string column) => $"position(lower(@filter) in lower(coalesce({column}, ''))) > 0"; 
+
+                query.Append($" and ({Filter("name")} or {Filter("display_name")}");
+                if (includeDescriptionInNameFilter) query.Append($" or {Filter("description")}");
+                query.Append(")");
+            }
+            
+            return conn.QueryAsync<ListedMember>(query.ToString(), new {system, filter});
+        }
+    }
+}

PluralKit.Core/Database/Views/ListedMember.cs

@@ -0,0 +1,17 @@
+#nullable enable
+using NodaTime;
+
+namespace PluralKit.Core
+{
+    // TODO: is inheritance here correct?
+    public class ListedMember: PKMember
+    {
+        public ulong? LastMessage { get; }
+        public Instant? LastSwitchTime { get; }
+
+        public AnnualDate? AnnualBirthday =>
+            Birthday != null
+                ? new AnnualDate(Birthday.Value.Month, Birthday.Value.Day)
+                : (AnnualDate?) null;
+    }
+}

PluralKit.Core/Database/Views/SystemFronter.cs

@@ -0,0 +1,14 @@
+using NodaTime;
+
+namespace PluralKit.Core
+{
+    public class SystemFronter
+    {
+        public int SystemId { get; }
+        public int SwitchId { get; }
+        public Instant SwitchTimestamp { get; }
+        public int MemberId { get; }
+        public string MemberHid { get; }
+        public string MemberName { get; }
+    }
+}

PluralKit.Core/Database/Views/views.sql

@@ -1,4 +1,5 @@
-create view system_last_switch as
+-- Returns one row per system, containing info about latest switch + array of member IDs (for future joins)
+create view system_last_switch as
 select systems.id as system,
        last_switch.id as switch,
        last_switch.timestamp as timestamp,
@@ -6,6 +7,25 @@ select systems.id as system,
 from systems
     inner join lateral (select * from switches where switches.system = systems.id order by timestamp desc limit 1) as last_switch on true;
 
+-- Returns one row for every current fronter in a system, w/ some member info
+create view system_fronters as
+select
+    systems.id as system_id,
+    last_switch.id as switch_id,
+    last_switch.timestamp as switch_timestamp,
+    members.id as member_id,
+    members.hid as member_hid,
+    members.name as member_name
+from systems
+    -- TODO: is there a more efficient way of doing this search? might need to index on timestamp if we haven't in prod
+    inner join lateral (select * from switches where switches.system = systems.id order by timestamp desc limit 1) as last_switch on true
+        
+    -- change to left join to handle memberless switches?
+    inner join switch_members on switch_members.switch = last_switch.system
+    inner join members on members.id = switch_members.member
+-- return them in order of the switch itself
+order by switch_members.id;
+
 create view member_list as
 select members.*,
        -- Find last message ID

PluralKit.Core/Database/clean.sql

@@ -1,4 +1,9 @@
-drop view if exists system_last_switch;
+-- This gets run on every bot startup and makes sure we're starting from a clean slate
+-- Then, the views/functions.sql files get run, and they recreate the necessary objects
+-- This does mean we can't use any functions in row triggers, etc. Still unsure how to handle this.
+
+drop view if exists system_last_switch;
+drop view if exists system_fronters;
 drop view if exists member_list;
 
 drop function if exists message_context;