From 6d06474d26c288708e7940cd17bb0c77c5e107f5 Mon Sep 17 00:00:00 2001 From: Ske Date: Sat, 13 Jun 2020 21:49:31 +0200 Subject: [PATCH] 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 | 34 +---- PluralKit.Bot/Lists/IListRenderer.cs | 4 +- PluralKit.Bot/Lists/LongRenderer.cs | 15 +-- PluralKit.Bot/Lists/PKListMember.cs | 12 -- PluralKit.Bot/Lists/ShortRenderer.cs | 6 +- PluralKit.Bot/Lists/SortFilterOptions.cs | 123 ++++++++---------- PluralKit.Core/Database/Database.cs | 2 +- .../Database/Views/DatabaseViewsExt.cs | 34 +++++ PluralKit.Core/Database/Views/ListedMember.cs | 17 +++ .../Database/Views/SystemFronter.cs | 14 ++ PluralKit.Core/Database/{ => Views}/views.sql | 22 +++- PluralKit.Core/Database/clean.sql | 7 +- 12 files changed, 164 insertions(+), 126 deletions(-) delete mode 100644 PluralKit.Bot/Lists/PKListMember.cs create mode 100644 PluralKit.Core/Database/Views/DatabaseViewsExt.cs create mode 100644 PluralKit.Core/Database/Views/ListedMember.cs create mode 100644 PluralKit.Core/Database/Views/SystemFronter.cs rename PluralKit.Core/Database/{ => Views}/views.sql (56%) diff --git a/PluralKit.Bot/Commands/SystemList.cs b/PluralKit.Bot/Commands/SystemList.cs index f225b457..3b633353 100644 --- a/PluralKit.Bot/Commands/SystemList.cs +++ b/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> 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(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 "); diff --git a/PluralKit.Bot/Lists/IListRenderer.cs b/PluralKit.Bot/Lists/IListRenderer.cs index 54c70d44..23368882 100644 --- a/PluralKit.Bot/Lists/IListRenderer.cs +++ b/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 members); + void RenderPage(DiscordEmbedBuilder eb, DateTimeZone zone, IEnumerable members); } } \ No newline at end of file diff --git a/PluralKit.Bot/Lists/LongRenderer.cs b/PluralKit.Bot/Lists/LongRenderer.cs index 00be46a8..48a88bf4 100644 --- a/PluralKit.Bot/Lists/LongRenderer.cs +++ b/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 members) + public void RenderPage(DiscordEmbedBuilder eb, DateTimeZone zone, IEnumerable 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.ShowLastSwitch && m.LastSwitchTime != null) profile += $"\n**Last switched in:** {FormatTimestamp(system, m.LastSwitchTime.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(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)*"; @@ -40,9 +41,7 @@ namespace PluralKit.Bot eb.AddField(m.Name, profile.Truncate(1024)); } } - - private static string FormatTimestamp(PKSystem system, Instant timestamp) => DateTimeFormats.ZonedDateTimeFormat.Format(timestamp.InZone(system.Zone ?? DateTimeZone.Utc)); - + public class MemberFields { public bool ShowDisplayName = true; diff --git a/PluralKit.Bot/Lists/PKListMember.cs b/PluralKit.Bot/Lists/PKListMember.cs deleted file mode 100644 index a16a1566..00000000 --- a/PluralKit.Bot/Lists/PKListMember.cs +++ /dev/null @@ -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; } - } -} \ No newline at end of file diff --git a/PluralKit.Bot/Lists/ShortRenderer.cs b/PluralKit.Bot/Lists/ShortRenderer.cs index 7fd48947..d2404c32 100644 --- a/PluralKit.Bot/Lists/ShortRenderer.cs +++ b/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 members) + public void RenderPage(DiscordEmbedBuilder eb, DateTimeZone timezone, IEnumerable members) { - string RenderLine(PKListMember m) + string RenderLine(ListedMember m) { if (m.HasProxyTags) { diff --git a/PluralKit.Bot/Lists/SortFilterOptions.cs b/PluralKit.Bot/Lists/SortFilterOptions.cs index 2422ec79..de5eed6c 100644 --- a/PluralKit.Bot/Lists/SortFilterOptions.cs +++ b/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 { @@ -46,69 +52,53 @@ namespace PluralKit.Bot return str.ToString(); } - - public string BuildQuery() - { - // For best performance, add index: - // - `on switch_members using btree (member asc nulls last) include (switch);` - // TODO: add a migration adding this, perhaps lumped with the rest of the DB changes (it's there in prod) - - // Select clause - StringBuilder query = new StringBuilder("select * from member_list"); - - // Filtering - query.Append(" where system = @System"); - query.Append(PrivacyFilter switch - { - PrivacyFilter.PrivateOnly => $" and member_privacy = {(int) PrivacyLevel.Private}", - PrivacyFilter.PublicOnly => $" and member_privacy = {(int) PrivacyLevel.Public}", - _ => "" - }); - - // String filter - if (Filter != null) - { - // Use position rather than ilike to not bother with escaping and such - query.Append(" and ("); - query.Append( - "position(lower(@Filter) in lower(name)) > 0 or position(lower(@Filter) in lower(coalesce(display_name, ''))) > 0"); - if (SearchInDescription) - query.Append(" or position(lower(@Filter) in lower(coalesce(description, ''))) > 0"); - query.Append(")"); - } - - // Order direction - var direction = SortProperty switch - { - // Some of these make more "logical sense" as descending (ie. "last message" = descending order of message timestamp/ID) - SortProperty.MessageCount => SortDirection.Descending, - SortProperty.LastMessage => SortDirection.Descending, - SortProperty.LastSwitch => SortDirection.Descending, - _ => SortDirection.Ascending - }; - if (Reverse) direction = direction == SortDirection.Ascending ? SortDirection.Descending : SortDirection.Ascending; - var order = direction == SortDirection.Ascending ? "asc" : "desc"; - - // Order clause - 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 async Task> Execute(IPKConnection conn, PKSystem system) + { + var filtered = await QueryWithFilter(conn, system); + return Sort(filtered); + } + + private Task> QueryWithFilter(IPKConnection conn, PKSystem system) => + conn.QueryMemberList(system.Id, PrivacyFilter switch + { + PrivacyFilter.PrivateOnly => PrivacyLevel.Private, + PrivacyFilter.PublicOnly => PrivacyLevel.Public, + PrivacyFilter.All => null + }, Filter, SearchInDescription); + + private IEnumerable Sort(IEnumerable input) + { + IComparer ReverseMaybe(IComparer c) => + Reverse ? Comparer.Create((a, b) => c.Compare(b, a)) : c; + + var culture = StringComparer.InvariantCultureIgnoreCase; + return (SortProperty switch + { + // As for the OrderByDescending HasValue calls: https://www.jerriepelser.com/blog/orderby-with-null-values/ + // We want nulls last no matter what, even if orders are reversed + SortProperty.Hid => input.OrderBy(m => m.Hid, ReverseMaybe(culture)), + SortProperty.Name => input.OrderBy(m => m.Name, ReverseMaybe(culture)), + SortProperty.CreationDate => input.OrderBy(m => m.Created, ReverseMaybe(Comparer.Default)), + SortProperty.MessageCount => input.OrderByDescending(m => m.MessageCount, ReverseMaybe(Comparer.Default)), + SortProperty.DisplayName => input + .OrderByDescending(m => m.DisplayName != null) + .ThenBy(m => m.DisplayName, ReverseMaybe(culture)), + SortProperty.Birthdate => input + .OrderByDescending(m => m.AnnualBirthday.HasValue) + .ThenBy(m => m.AnnualBirthday, ReverseMaybe(Comparer.Default)), + SortProperty.LastMessage => input + .OrderByDescending(m => m.LastMessage.HasValue) + .ThenByDescending(m => m.LastMessage, ReverseMaybe(Comparer.Default)), + SortProperty.LastSwitch => input + .OrderByDescending(m => m.LastSwitchTime.HasValue) + .ThenByDescending(m => m.LastSwitchTime, ReverseMaybe(Comparer.Default)), + _ => throw new ArgumentOutOfRangeException($"Unknown sort property {SortProperty}") + }) + // Lastly, add a by-name fallback order for collisions (generally hits w/ lots of null values) + .ThenBy(m => m.Name, culture); + } + public static SortFilterOptions FromFlags(Context ctx) { var p = new SortFilterOptions(); @@ -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, diff --git a/PluralKit.Core/Database/Database.cs b/PluralKit.Core/Database/Database.cs index d0f1975f..85aac202 100644 --- a/PluralKit.Core/Database/Database.cs +++ b/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 diff --git a/PluralKit.Core/Database/Views/DatabaseViewsExt.cs b/PluralKit.Core/Database/Views/DatabaseViewsExt.cs new file mode 100644 index 00000000..2f156472 --- /dev/null +++ b/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> QueryCurrentFronters(this IPKConnection conn, int system) => + conn.QueryAsync("select * from system_fronters where system = @system", new {system}); + + public static Task> 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(query.ToString(), new {system, filter}); + } + } +} \ No newline at end of file diff --git a/PluralKit.Core/Database/Views/ListedMember.cs b/PluralKit.Core/Database/Views/ListedMember.cs new file mode 100644 index 00000000..47a68439 --- /dev/null +++ b/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; + } +} \ No newline at end of file diff --git a/PluralKit.Core/Database/Views/SystemFronter.cs b/PluralKit.Core/Database/Views/SystemFronter.cs new file mode 100644 index 00000000..c727dc22 --- /dev/null +++ b/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; } + } +} \ No newline at end of file diff --git a/PluralKit.Core/Database/views.sql b/PluralKit.Core/Database/Views/views.sql similarity index 56% rename from PluralKit.Core/Database/views.sql rename to PluralKit.Core/Database/Views/views.sql index 0f0714a2..87ffbe86 100644 --- a/PluralKit.Core/Database/views.sql +++ b/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 diff --git a/PluralKit.Core/Database/clean.sql b/PluralKit.Core/Database/clean.sql index e2431f43..21e3cbe8 100644 --- a/PluralKit.Core/Database/clean.sql +++ b/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;