Skip to content

Respect caller-owned invite CSV streams #759

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
Lulalaby merged 4 commits into from
Jan 22, 2026
Merged

Respect caller-owned invite CSV streams #759

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92ed6fe
Initial plan
Copilot Jan 22, 2026
a7398ff
Respect caller-owned invite CSV streams
Copilot Jan 22, 2026
bc975e3
Simplify null checks per review
Copilot Jan 22, 2026
fa07919
Dispose only internally-created target user CSV streams
Copilot Jan 22, 2026
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
16 changes: 8 additions & 8 deletions DisCatSharp/Net/Rest/DiscordApiClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3853,6 +3853,7 @@ internal async Task<DiscordInvite> CreateChannelInviteAsync(ulong channelId, int
var targetUsersCsv = targetUsersFile is not null
? CreateTargetUsersFile(targetUsersFile)
: BuildTargetUsersCsvFile(mergedTargetUsers);
var disposeTargetUsersCsv = targetUsersFile is null && targetUsersCsv is not null;

var pld = new RestChannelInviteCreatePayload
{
Expand Down Expand Up @@ -3893,17 +3894,14 @@ internal async Task<DiscordInvite> CreateChannelInviteAsync(ulong channelId, int
res = await this.DoRequestAsync(this.Discord, bucket, url, RestRequestMethod.POST, route, headers, DiscordJson.SerializeObject(pld)).ConfigureAwait(false);
}

if (targetUsersCsv?.ResetPositionTo is not null)
targetUsersCsv.Stream.Position = targetUsersCsv.ResetPositionTo.Value;

var ret = DiscordJson.DeserializeObject<DiscordInvite>(res.Response, this.Discord);
Comment on lines 3896 to 3897
Copy link

@sentry sentry bot Jan 22, 2026

Choose a reason for hiding this comment

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

Bug: The CreateChannelInviteAsync method fails to restore the original position of a caller-provided stream in targetUsersCsv, leaving it at an unexpected position after the operation completes.
Severity: MEDIUM

Suggested Fix

Reintroduce the stream position reset logic in CreateChannelInviteAsync before the method exits. Add back the check if (targetUsersCsv?.ResetPositionTo is not null) targetUsersCsv.Stream.Position = targetUsersCsv.ResetPositionTo.Value; to ensure the stream's position is restored for caller-owned streams, mirroring the implementation in UpdateInviteTargetUsersAsync.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: DisCatSharp/Net/Rest/DiscordApiClient.cs#L3896-L3897

Potential issue: The `CreateChannelInviteAsync` method, when provided with a
caller-owned stream via `targetUsersCsv`, repositions the stream to read it but fails to
restore its original position afterward. The logic to reset the stream's position was
removed in the pull request. This leaves the caller's stream at an unexpected position
(likely the end), which can cause bugs if the caller attempts to reuse the stream. This
behavior is inconsistent with the similar `UpdateInviteTargetUsersAsync` method, which
correctly restores the stream's position.

Did we get this right? 👍 / 👎 to inform future reviews.

ret.Discord = this.Discord;

if (targetUsersCsv is not null && targetUsersCsv.ResetPositionTo is null)
if (disposeTargetUsersCsv)
{
Comment on lines 3897 to 3901
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Jan 22, 2026

Choose a reason for hiding this comment

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

P2 Badge Restore caller stream position after invite upload

When targetUsersFile is caller-provided and seekable, CreateTargetUsersFile seeks it to position 0 and records ResetPositionTo. In CreateChannelInviteAsync the post-request reset was removed, so after the multipart upload the stream stays at the end rather than returning to the original position. This means caller-owned streams are no longer “untouched” and any subsequent reuse (e.g., re-sending the same CSV or reading from the original offset) will fail or read nothing. Consider restoring targetUsersCsv.Stream.Position when ResetPositionTo is set, as is still done in UpdateInviteTargetUsersAsync.

Useful? React with 👍 / 👎.

try
{
targetUsersCsv.Stream.Dispose();
targetUsersCsv?.Stream.Dispose();
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

The null-conditional operator on targetUsersCsv?.Stream.Dispose() is redundant. The outer condition if (disposeTargetUsersCsv) already ensures targetUsersCsv is not null because disposeTargetUsersCsv is only true when targetUsersFile is null && targetUsersCsv is not null. You can safely use targetUsersCsv.Stream.Dispose() instead.

Suggested change
targetUsersCsv?.Stream.Dispose();
targetUsersCsv.Stream.Dispose();

Copilot uses AI. Check for mistakes.
}
catch
{
Expand Down Expand Up @@ -5136,6 +5134,7 @@ internal async Task UpdateInviteTargetUsersAsync(string inviteCode, Stream? targ
var targetUsersCsv = targetUsersFile is not null
? CreateTargetUsersFile(targetUsersFile)
: BuildTargetUsersCsvFile(mergedTargetUsers);
var disposeTargetUsersCsv = targetUsersFile is null && targetUsersCsv is not null;

if (targetUsersCsv is null)
throw new ArgumentException("No target users provided for update.");
Expand All @@ -5153,13 +5152,14 @@ internal async Task UpdateInviteTargetUsersAsync(string inviteCode, Stream? targ
var url = Utilities.GetApiUriFor(path, this.Discord.Configuration);
await this.DoMultipartAsync(this.Discord, bucket, url, RestRequestMethod.PUT, route, headers, files: new[] { targetUsersCsv }, fileFieldNameFactory: _ => "target_users_file").ConfigureAwait(false);

if (targetUsersCsv.ResetPositionTo is not null)
if (targetUsersCsv is not null && targetUsersCsv.ResetPositionTo is not null)
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

The null check targetUsersCsv is not null is redundant. At this point in the code flow, targetUsersCsv is guaranteed to be non-null because line 5139 throws an ArgumentException if it's null. The condition can be simplified to just checking targetUsersCsv.ResetPositionTo is not null.

Suggested change
if (targetUsersCsv is not null && targetUsersCsv.ResetPositionTo is not null)
if (targetUsersCsv.ResetPositionTo is not null)

Copilot uses AI. Check for mistakes.
targetUsersCsv.Stream.Position = targetUsersCsv.ResetPositionTo.Value;
else if (targetUsersCsv is not null)

if (disposeTargetUsersCsv)
{
try
{
targetUsersCsv.Stream.Dispose();
targetUsersCsv?.Stream.Dispose();
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

The null-conditional operator on targetUsersCsv?.Stream.Dispose() is redundant. The outer condition if (disposeTargetUsersCsv) already ensures targetUsersCsv is not null because disposeTargetUsersCsv is only true when targetUsersFile is null && targetUsersCsv is not null. You can safely use targetUsersCsv.Stream.Dispose() instead.

Suggested change
targetUsersCsv?.Stream.Dispose();
targetUsersCsv.Stream.Dispose();

Copilot uses AI. Check for mistakes.
}
catch
{
Expand Down
Loading