Skip to content

avoid bulk overwriting global commands #2115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
akiraveliara merged 6 commits into from
Oct 24, 2024
Merged

avoid bulk overwriting global commands #2115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0b850e6
preliminary mitigation of the issue
akiraveliara Oct 14, 2024
c2c8be4
rewire everything to avoid global bulk updates
akiraveliara Oct 18, 2024
f182880
formatting artifact
akiraveliara Oct 18, 2024
5dfd9d1
Merge branch 'master' into aki/avoid-bulk-overwriting-commands-if-pos…
akiraveliara Oct 18, 2024
15b7fc1
treat GuildInstall as default
akiraveliara Oct 18, 2024
01f30c8
some pattern matching, as a treat
akiraveliara Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ...cessors/SlashCommands/RemoteRecordRetentionPolicies/DefaultRemoteRecordRetentionPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
using System.Threading.Tasks;

using DSharpPlus.Entities;

namespace DSharpPlus.Commands.Processors.SlashCommands.RemoteRecordRetentionPolicies;

internal sealed class DefaultRemoteRecordRetentionPolicy : IRemoteRecordRetentionPolicy
{
public Task<bool> CheckDeletionStatusAsync(DiscordApplicationCommand command)
=> Task.FromResult(command.Type != DiscordApplicationCommandType.ActivityEntryPoint);
}
16 changes: 16 additions & 0 deletions ...ds/Processors/SlashCommands/RemoteRecordRetentionPolicies/IRemoteRecordRetentionPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
using System.Threading.Tasks;

using DSharpPlus.Entities;

namespace DSharpPlus.Commands.Processors.SlashCommands.RemoteRecordRetentionPolicies;

/// <summary>
/// Provides a means to customize when and which application commands get deleted from your bot.
/// </summary>
public interface IRemoteRecordRetentionPolicy
{
/// <summary>
/// Returns a value indicating whether the application command should be deleted or not.
/// </summary>
public Task<bool> CheckDeletionStatusAsync(DiscordApplicationCommand command);
}
22 changes: 22 additions & 0 deletions DSharpPlus.Commands/Processors/SlashCommands/SlashCommandConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using DSharpPlus.Commands.Processors.SlashCommands.NamingPolicies;
using DSharpPlus.Commands.Processors.SlashCommands.RemoteRecordRetentionPolicies;

namespace DSharpPlus.Commands.Processors.SlashCommands;

Expand All @@ -21,4 +22,25 @@ public sealed class SlashCommandConfiguration
/// How to name parameters when registering or receiving interaction data.
/// </summary>
public IInteractionNamingPolicy NamingPolicy { get; init; } = new SnakeCaseNamingPolicy();

/// <summary>
/// Instructs DSharpPlus to always overwrite the command records Discord has of our bot on startup.
/// </summary>
/// <remarks>
/// This skips the startup procedure of fetching commands and overwriting only if additions are detected. While
/// this may save time on startup, it also makes the library less resistant to unrecognized command types or
/// structures it cannot correctly handle. <br/>
/// Currently, removals are <i>not</i> considered a reason to overwrite by default so as to work around an issue
/// where certain commands will cause bulk overwrites to fail.
/// </remarks>
public bool UnconditionallyOverwriteCommands { get; init; } = false;

/// <summary>
/// Controls when DSharpPlus deletes an application command that does not have a local equivalent.
/// </summary>
/// <remarks>
/// By default, this will delete all application commands except for activity entrypoints.
/// </remarks>
public IRemoteRecordRetentionPolicy RemoteRecordRetentionPolicy { get; init; }
= new DefaultRemoteRecordRetentionPolicy();
}
130 changes: 130 additions & 0 deletions DSharpPlus.Commands/Processors/SlashCommands/SlashCommandProcessor.Registration.Remote.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

using DSharpPlus.Entities;
using DSharpPlus.Net.Models;

namespace DSharpPlus.Commands.Processors.SlashCommands;

public sealed partial class SlashCommandProcessor
{
private async Task<IReadOnlyList<DiscordApplicationCommand>> VerifyAndUpdateRemoteCommandsAsync
(
IReadOnlyList<DiscordApplicationCommand> local,
IReadOnlyList<DiscordApplicationCommand> remoteCommands
)
{
int added = 0, edited = 0, unchanged = 0, deleted = 0;
List<DiscordApplicationCommand> updated = [];
List<DiscordApplicationCommand> remoteTracking = new(remoteCommands);

foreach (DiscordApplicationCommand command in local)
{
DiscordApplicationCommand remote;

if ((remote = remoteTracking.SingleOrDefault(x => x.Name == command.Name)) is not null)
{
if (command.WeakEquals(remote))
{
unchanged++;
updated.Add(command);
remoteTracking.Remove(remote);
continue;
}
else
{
edited++;
updated.Add(await ModifyGlobalCommandAsync(remote.Id, command));
remoteTracking.Remove(remote);
continue;
}
}
else
{
added++;
updated.Add(await CreateGlobalCommandAsync(command));
}
}

deleted = remoteTracking.Count;

foreach (DiscordApplicationCommand toDelete in remoteTracking)
{
if (await this.Configuration.RemoteRecordRetentionPolicy.CheckDeletionStatusAsync(toDelete))
{
await DeleteGlobalCommandAsync(toDelete);
}
}

if (added != 0 || edited != 0 || deleted != 0)
{
SlashLogging.detectedCommandRecordChanges(this.logger, unchanged, added, edited, deleted, null);
}
else
{
SlashLogging.commandRecordsUnchanged(this.logger, null);
}

return updated;
}

private async ValueTask<DiscordApplicationCommand> CreateGlobalCommandAsync(DiscordApplicationCommand command)
{
return this.extension.DebugGuildId == 0
? await this.extension.Client.CreateGlobalApplicationCommandAsync(command)
: await this.extension.Client.CreateGuildApplicationCommandAsync(this.extension.DebugGuildId, command);
}

#pragma warning disable IDE0046
private async ValueTask<DiscordApplicationCommand> ModifyGlobalCommandAsync(ulong id, DiscordApplicationCommand command)
{
if (this.extension.DebugGuildId == 0)
{
return await this.extension.Client.EditGlobalApplicationCommandAsync(id, x => CopyToEditModel(command, x));
}
else
{
return await this.extension.Client.EditGuildApplicationCommandAsync
(
this.extension.DebugGuildId,
id,
x => CopyToEditModel(command, x)
);
}
}
#pragma warning restore IDE0046

private async ValueTask DeleteGlobalCommandAsync(DiscordApplicationCommand command)
{
if (this.extension.DebugGuildId == 0)
{
await this.extension.Client.DeleteGlobalApplicationCommandAsync(command.Id);
}
else
{
await this.extension.Client.DeleteGuildApplicationCommandAsync(this.extension.DebugGuildId, command.Id);
}
Copy link

@Voroniyx Voroniyx Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code stylistically, should be the same as in CreateGlobalCommandAsync, because except for the different method call it is the same. So either an if-else or ?: in both

Copy link
Member Author

@akiraveliara akiraveliara Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't be, because this one doesn't return a value it can't be a ternary

Copy link

@Voroniyx Voroniyx Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad then, overlooked it sorry

}

private static void CopyToEditModel(DiscordApplicationCommand command, ApplicationCommandEditModel editModel)
{
editModel.AllowDMUsage = command.AllowDMUsage.HasValue
? new(command.AllowDMUsage.Value)
: Optional.FromNoValue<bool>();
editModel.DefaultMemberPermissions = command.DefaultMemberPermissions;
editModel.Description = command.Description;
editModel.NameLocalizations = command.NameLocalizations;
editModel.DescriptionLocalizations = command.DescriptionLocalizations;
editModel.IntegrationTypes = command.IntegrationTypes is not null
? new(command.IntegrationTypes)
: Optional.FromNoValue<IEnumerable<DiscordApplicationIntegrationType>>();
editModel.AllowedContexts = command.Contexts is not null
? new(command.Contexts)
: Optional.FromNoValue<IEnumerable<DiscordInteractionContextType>>();
editModel.NSFW = command.NSFW;
editModel.Options = command.Options is not null
? new(command.Options)
: Optional.FromNoValue<IReadOnlyList<DiscordApplicationCommandOption>>();
}
}
50 changes: 34 additions & 16 deletions DSharpPlus.Commands/Processors/SlashCommands/SlashCommandProcessor.Registration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Text;
using System.Text.RegularExpressions;
using System.Threading.Tasks;

using DSharpPlus.Commands.ArgumentModifiers;
using DSharpPlus.Commands.ContextChecks;
using DSharpPlus.Commands.Converters;
Expand All @@ -17,6 +18,7 @@
using DSharpPlus.Commands.Trees;
using DSharpPlus.Commands.Trees.Metadata;
using DSharpPlus.Entities;

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
Expand Down Expand Up @@ -57,6 +59,7 @@ public async ValueTask RegisterSlashCommandsAsync(CommandsExtension extension)
List<DiscordApplicationCommand> globalApplicationCommands = [];
Dictionary<ulong, List<DiscordApplicationCommand>> guildsApplicationCommands = [];
globalApplicationCommands.AddRange(applicationCommands);

foreach (Command command in processorSpecificCommands)
{
// If there is a SlashCommandTypesAttribute, check if it contains SlashCommandTypes.ApplicationCommand
Expand Down Expand Up @@ -87,30 +90,44 @@ public async ValueTask RegisterSlashCommandsAsync(CommandsExtension extension)
}
}

// we figured our structure out, fetch discord's records of the commands and match basic criteria
// skip if we are instructed to disable this behaviour

List<DiscordApplicationCommand> discordCommands = [];
if (extension.DebugGuildId == 0)
{
discordCommands.AddRange(await extension.Client.BulkOverwriteGlobalApplicationCommandsAsync(globalApplicationCommands));
foreach ((ulong guildId, List<DiscordApplicationCommand> guildCommands) in guildsApplicationCommands)
{
discordCommands.AddRange(await extension.Client.BulkOverwriteGuildApplicationCommandsAsync(guildId, guildCommands));
}

if (this.Configuration.UnconditionallyOverwriteCommands)
{
discordCommands.AddRange
(
this.extension.DebugGuildId == 0
? await this.extension.Client.BulkOverwriteGlobalApplicationCommandsAsync(globalApplicationCommands)
: await this.extension.Client.BulkOverwriteGuildApplicationCommandsAsync
(
this.extension.DebugGuildId,
globalApplicationCommands
)
);
}
else
{
// 1. Aggregate all guild specific and global commands
// 2. GroupBy name
// 3. Only take the first command per name
IEnumerable<DiscordApplicationCommand> distinctCommands = guildsApplicationCommands
.SelectMany(x => x.Value)
.Concat(globalApplicationCommands)
.GroupBy(x => x.Name)
.Select(x => x.First());
IReadOnlyList<DiscordApplicationCommand> preexisting = this.extension.DebugGuildId == 0
? await this.extension.Client.GetGlobalApplicationCommandsAsync()
: await this.extension.Client.GetGuildApplicationCommandsAsync(this.extension.DebugGuildId);

discordCommands.AddRange(await VerifyAndUpdateRemoteCommandsAsync(globalApplicationCommands, preexisting));
}

discordCommands.AddRange(await extension.Client.BulkOverwriteGuildApplicationCommandsAsync(extension.DebugGuildId, distinctCommands));
// for the time being, we still overwrite guilds by force
foreach (KeyValuePair<ulong, List<DiscordApplicationCommand>> kv in guildsApplicationCommands)
{
discordCommands.AddRange
(
await this.extension.Client.BulkOverwriteGuildApplicationCommandsAsync(kv.Key, kv.Value)
);
}

applicationCommandMapping = MapApplicationCommands(discordCommands).ToFrozenDictionary();

SlashLogging.registeredCommands(
this.logger,
applicationCommandMapping.Count,
Expand All @@ -134,6 +151,7 @@ public IReadOnlyDictionary<ulong, Command> MapApplicationCommands(IReadOnlyList<
Dictionary<ulong, Command> commandsDictionary = [];
IReadOnlyList<Command> processorSpecificCommands = this.extension!.GetCommandsForProcessor(this);
IReadOnlyList<Command> flattenCommands = processorSpecificCommands.SelectMany(x => x.Flatten()).ToList();

foreach (DiscordApplicationCommand discordCommand in applicationCommands)
{
bool commandFound = false;
Expand Down
2 changes: 2 additions & 0 deletions DSharpPlus.Commands/Processors/SlashCommands/SlashLogging.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ internal static class SlashLogging
internal static readonly Action<ILogger, int, int, Exception?> registeredCommands = LoggerMessage.Define<int, int>(LogLevel.Information, new EventId(1, "Slash Commands Startup"), "Registered {TopLevelCommandCount:N0} top-level slash commands, {TotalCommandCount:N0} total slash commands.");
internal static readonly Action<ILogger, Exception?> interactionReceivedBeforeConfigured = LoggerMessage.Define(LogLevel.Warning, new EventId(2, "Slash Commands Startup"), "Received an interaction before the slash commands processor was configured. This interaction will be ignored.");
internal static readonly Action<ILogger, string, Exception?> unknownCommandName = LoggerMessage.Define<string>(LogLevel.Trace, new EventId(1, "Slash Commands Runtime"), "Received Command '{CommandName}' but no matching local command was found. Was this command for a different process?");
internal static readonly Action<ILogger, int, int, int, int, Exception?> detectedCommandRecordChanges = LoggerMessage.Define<int, int, int, int>(LogLevel.Information, default, "Detected changes in slash command records: {Unchanged} without changes, {Added} added, {Edited} edited, {Deleted} deleted");
internal static readonly Action<ILogger, Exception?> commandRecordsUnchanged = LoggerMessage.Define(LogLevel.Information, default, "No application command changes detected.");
}
39 changes: 37 additions & 2 deletions DSharpPlus/Clients/DiscordClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,8 +634,25 @@ public async Task<DiscordApplicationCommand> EditGlobalApplicationCommandAsync(u
{
ApplicationCommandEditModel mdl = new();
action(mdl);

ulong applicationId = this.CurrentApplication?.Id ?? (await GetCurrentApplicationAsync()).Id;
return await this.ApiClient.EditGlobalApplicationCommandAsync(applicationId, commandId, mdl.Name, mdl.Description, mdl.Options, mdl.DefaultPermission, mdl.NSFW, default, default, mdl.AllowDMUsage, mdl.DefaultMemberPermissions);

return await this.ApiClient.EditGlobalApplicationCommandAsync
(
applicationId,
commandId,
mdl.Name,
mdl.Description,
mdl.Options,
mdl.DefaultPermission,
mdl.NSFW,
mdl.NameLocalizations,
mdl.DescriptionLocalizations,
mdl.AllowDMUsage,
mdl.DefaultMemberPermissions,
mdl.AllowedContexts,
mdl.IntegrationTypes
);
}

/// <summary>
Expand Down Expand Up @@ -692,8 +709,26 @@ public async Task<DiscordApplicationCommand> EditGuildApplicationCommandAsync(ul
{
ApplicationCommandEditModel mdl = new();
action(mdl);

ulong applicationId = this.CurrentApplication?.Id ?? (await GetCurrentApplicationAsync()).Id;
return await this.ApiClient.EditGuildApplicationCommandAsync(applicationId, guildId, commandId, mdl.Name, mdl.Description, mdl.Options, mdl.DefaultPermission, mdl.NSFW, default, default, mdl.AllowDMUsage, mdl.DefaultMemberPermissions);

return await this.ApiClient.EditGuildApplicationCommandAsync
(
applicationId,
guildId,
commandId,
mdl.Name,
mdl.Description,
mdl.Options,
mdl.DefaultPermission,
mdl.NSFW,
mdl.NameLocalizations,
mdl.DescriptionLocalizations,
mdl.AllowDMUsage,
mdl.DefaultMemberPermissions,
mdl.AllowedContexts,
mdl.IntegrationTypes
);
}

/// <summary>
Expand Down
Loading