Respect caller-owned invite CSV streams #759
Conversation
Co-authored-by: Lulalaby <14029133+Lulalaby@users.noreply.github.com>
Co-authored-by: Lulalaby <14029133+Lulalaby@users.noreply.github.com>
|
@copilot as codex mentioned. We shouldn't dispose if it's not our stream. Relying on reset position is wrong here. Find another way |
Co-authored-by: Lulalaby <14029133+Lulalaby@users.noreply.github.com>
Lulalaby
requested review from
BloodDragooner,
ProjectMakotoBot,
byterand and
Copilot
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa07919dea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var ret = DiscordJson.DeserializeObject<DiscordInvite>(res.Response, this.Discord); | ||
| ret.Discord = this.Discord; | ||
|
|
||
| if (targetUsersCsv is not null && targetUsersCsv.ResetPositionTo is null) | ||
| if (disposeTargetUsersCsv) | ||
| { |
There was a problem hiding this comment.
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 👍 / 👎.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| if (targetUsersCsv?.ResetPositionTo is not null) | ||
| targetUsersCsv.Stream.Position = targetUsersCsv.ResetPositionTo.Value; | ||
|
|
||
| var ret = DiscordJson.DeserializeObject<DiscordInvite>(res.Response, this.Discord); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where caller-provided CSV streams for invite target users were being disposed by the API client, violating stream ownership principles. The fix introduces explicit tracking of stream ownership: only internally-created streams (from BuildTargetUsersCsvFile) are disposed, while caller-provided streams remain untouched.
Changes:
- Introduced
disposeTargetUsersCsvflag to track whether the CSV stream was created internally - Modified disposal logic to only dispose internally-created streams
- Adjusted position reset behavior: removed for
CreateChannelInviteAsync(POST), retained forUpdateInviteTargetUsersAsync(PUT)
Comments suppressed due to low confidence (32)
DisCatSharp/Net/Rest/DiscordApiClient.cs:3139
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var embed in embeds)
if (embed.Timestamp != null)
embed.Timestamp = embed.Timestamp.Value.ToUniversalTime();
DisCatSharp/Net/Rest/DiscordApiClient.cs:3192
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var embed in builder.Embeds)
if (embed?.Timestamp != null)
embed.Timestamp = embed.Timestamp.Value.ToUniversalTime();
DisCatSharp/Net/Rest/DiscordApiClient.cs:3569
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var embed in builder.Embeds)
if (embed.Timestamp != null)
embed.Timestamp = embed.Timestamp.Value.ToUniversalTime();
DisCatSharp/Net/Rest/DiscordApiClient.cs:5568
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var embed in builder.Embeds)
if (embed.Timestamp != null)
embed.Timestamp = embed.Timestamp.Value.ToUniversalTime();
DisCatSharp/Net/Rest/DiscordApiClient.cs:7457
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var embed in builder.Embeds)
if (embed.Timestamp != null)
embed.Timestamp = embed.Timestamp.Value.ToUniversalTime();
DisCatSharp/Net/Rest/DiscordApiClient.cs:7705
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var embed in builder.Embeds)
if (embed.Timestamp != null)
embed.Timestamp = embed.Timestamp.Value.ToUniversalTime();
DisCatSharp/Net/Rest/DiscordApiClient.cs:3783
- This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
foreach (var voterRaw in votersRawList)
{
var xr = voterRaw.ToDiscordObject<TransportUser>();
var usr = new DiscordUser(xr)
{
Discord = this.Discord
};
usr = this.Discord.UserCache.AddOrUpdate(xr.Id, usr, (id, old) =>
{
old.Username = usr.Username;
old.Discriminator = usr.Discriminator;
old.AvatarHash = usr.AvatarHash;
old.GlobalName = usr.GlobalName;
return old;
});
voters.Add(usr);
}
DisCatSharp/Net/Rest/DiscordApiClient.cs:7010
- The contents of this container are never accessed.
var values = new Dictionary<string, string>
DisCatSharp/Net/Rest/DiscordApiClient.cs:7615
- The contents of this container are never accessed.
var values = new Dictionary<string, string>();
DisCatSharp/Net/Rest/DiscordApiClient.cs:7652
- The contents of this container are never accessed.
var values = new Dictionary<string, string>();
DisCatSharp/Net/Rest/DiscordApiClient.cs:1486
- Condition is always not null because of ... != ....
urlParams["after"] = after?.ToString(CultureInfo.InvariantCulture);
DisCatSharp/Net/Rest/DiscordApiClient.cs:1488
- Condition is always not null because of ... != ....
urlParams["before"] = before?.ToString(CultureInfo.InvariantCulture);
DisCatSharp/Net/Rest/DiscordApiClient.cs:1490
- Condition is always not null because of ... != ....
urlParams["user_id"] = responsible?.ToString(CultureInfo.InvariantCulture);
DisCatSharp/Net/Rest/DiscordApiClient.cs:1492
- Condition is always not null because of ... != ....
urlParams["action_type"] = actionType?.ToString(CultureInfo.InvariantCulture);
DisCatSharp/Net/Rest/DiscordApiClient.cs:2565
- Condition is always not null because of access to property HasValue.
urlParams["with_user_count"] = withUserCount?.ToString();
DisCatSharp/Net/Rest/DiscordApiClient.cs:2609
- Condition is always not null because of access to property HasValue.
urlParams["with_user_count"] = withUserCount?.ToString();
DisCatSharp/Net/Rest/DiscordApiClient.cs:3508
- Condition is always not null because of ... != ....
urlParams["around"] = around?.ToString(CultureInfo.InvariantCulture);
DisCatSharp/Net/Rest/DiscordApiClient.cs:3510
- Condition is always not null because of ... != ....
urlParams["before"] = before?.ToString(CultureInfo.InvariantCulture);
DisCatSharp/Net/Rest/DiscordApiClient.cs:3512
- Condition is always not null because of ... != ....
urlParams["after"] = after?.ToString(CultureInfo.InvariantCulture);
DisCatSharp/Net/Rest/DiscordApiClient.cs:3748
- Condition is always not null because of ... != ....
urlParams["after"] = after?.ToString(CultureInfo.InvariantCulture);
DisCatSharp/Net/Rest/DiscordApiClient.cs:3750
- Condition is always not null because of ... > ....
urlParams["limit"] = limit?.ToString(CultureInfo.InvariantCulture);
DisCatSharp/Net/Rest/DiscordApiClient.cs:5085
- Condition is always not null because of access to property HasValue.
urlParams["with_counts"] = withCounts?.ToString();
DisCatSharp/Net/Rest/DiscordApiClient.cs:5087
- Condition is always not null because of access to property HasValue.
urlParams["guild_scheduled_event_id"] = guildScheduledEventId?.ToString();
DisCatSharp/Net/Rest/DiscordApiClient.cs:5089
- Condition is always not null because of access to property HasValue.
urlParams["with_permissions"] = withPermissions?.ToString();
DisCatSharp/Net/Rest/DiscordApiClient.cs:7486
- Condition is always not null because of ... is ....
DiscordPollRequest = builder?.Poll?.Build()
DisCatSharp/Net/Rest/DiscordApiClient.cs:1548
- Inefficient use of 'ContainsKey' and indexer.
ret.Guild = this.Discord.Guilds.ContainsKey(guildId) ? this.Discord.Guilds[guildId] : null;
DisCatSharp/Net/Rest/DiscordApiClient.cs:2579
- This assignment to guild is useless, since its value is never read.
var guild = this.Discord.Guilds[guildId];
DisCatSharp/Net/Rest/DiscordApiClient.cs:2622
- This assignment to guild is useless, since its value is never read.
var guild = this.Discord.Guilds[guildId];
DisCatSharp/Net/Rest/DiscordApiClient.cs:7013
- This assignment to values is useless, since its value is never read.
var values = new Dictionary<string, string>
{
["payload_json"] = DiscordJson.SerializeObject(pld)
};
DisCatSharp/Net/Rest/DiscordApiClient.cs:7615
- This assignment to values is useless, since its value is never read.
var values = new Dictionary<string, string>();
DisCatSharp/Net/Rest/DiscordApiClient.cs:7652
- This assignment to values is useless, since its value is never read.
var values = new Dictionary<string, string>();
DisCatSharp/Net/Rest/DiscordApiClient.cs:7561
- These 'if' statements can be combined.
if (builder is not null)
if (!string.IsNullOrEmpty(builder.Content) || builder.Embeds?.Count > 0 || builder.IsTts || builder.Mentions?.Count > 0 || builder.Files?.Count > 0 || builder.Components?.Count > 0)
values["payload_json"] = DiscordJson.SerializeObject(pld);
| try | ||
| { | ||
| targetUsersCsv.Stream.Dispose(); | ||
| targetUsersCsv?.Stream.Dispose(); |
There was a problem hiding this comment.
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.
| targetUsersCsv?.Stream.Dispose(); | |
| targetUsersCsv.Stream.Dispose(); |
| try | ||
| { | ||
| targetUsersCsv.Stream.Dispose(); | ||
| targetUsersCsv?.Stream.Dispose(); |
There was a problem hiding this comment.
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.
| targetUsersCsv?.Stream.Dispose(); | |
| targetUsersCsv.Stream.Dispose(); |
| 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) |
There was a problem hiding this comment.
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.
| if (targetUsersCsv is not null && targetUsersCsv.ResetPositionTo is not null) | |
| if (targetUsersCsv.ResetPositionTo is not null) |
Description
Invite APIs now leave caller-provided target user CSV streams entirely untouched after the request; only internally created CSV streams are disposed. This prevents disposing user-owned streams while keeping ownership clear for internal streams.
Type of change
How Has This Been Tested?
Test Configuration:
Checklist:
@Aiko-IT-Systems/discatsharp
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.