RFor the purpose of everyone's sanity when reading this issue please note that id, discrim, and discriminator can be used interchangeably. Offset, which may or may not be a discrim, is specifically used when referring to the key used to store the data in collections, repositories, or using the cache interface.
AbstractRepository's issue
The library will always explicitly overwrite the cache's parts in the same offset where the original part that was saved is located. This causes old parts that still exist to be overwritten when Discord returns a part with a different discrim. This can happen whenever a part's created property is set to false even when it exists on Discord' side, but also when a part already partially exists in the cache and the part being saved has an existing but differing discrim.
What introduced the issue
This bug was likely introduced prior the #895 when the cache interface was first implemented ~2 years ago. I discovered and reported the symptoms of the issue prior to then but a fix has never been introduced because of how hard it was to replicate the issue. The original behavior only updates the original $part, pushes it into the cache in a manner which would appropriately set the correct discrim, and returns it immediately. The new behavior is for the part to be updated, set in the cache using a method that would not appropriately set the correct discrim if a new part was returned, and then the part is returned to the user inside of a new promise scope.
This new logic flow may not be happening synchronously (as is the nature of the promise library) potentially allowing for data to be manipulated between the item being set to the cache and the next promise fulfilling and returning the result to the user. This could be anything from an external app touching the cache to Discord firing a channel create or update event. This is still not a confirmed root cause for part corruption and additional research will be needed to figure out where exactly the data manipulation is happening (if at all), although userland implementations can be completely ruled out due to being able to replicate without configuring an external cache. Regardless as to when the issue is happening is largely irrelevant for fixing this issue, however, and the incorrect functionality still needs to be fixed.
Why this is a library issue
The behavior as defined in AbstractRepository's save function, as it stands currently, is incorrect whenever a PATCH or POST request are made regardless of the context it was called in. Because part IDs are immutable, if Discord returns a part with a discrim that does not match the discrim of the original part, it should never overwrite the old part in the cache in the same offset but rather it should save a new part in the offset that is dictated by Discord and a new part (not the same one that was passed into repository->save() as a parameter) should be constructed using the fetched data and returned.
Reproduction steps
$channel = $guild->channels->get('id', '123');
$channel->created = false;
$channel->name = $channel->name . 'new';
$channel->guild->channels->save($channel), $success)->then(function ($new_channel)
{ // channel with discrim '123' is now overwritten with a new channel with a different discrim
$new_channel = $guild->channels->get('id', '123'); // Dumping the channel will show it to have a different ID entirely because ->save() overwrote the original object with the new channel that Discord created
});
It is important to note here that my original code does not set $channel->created to false, but rather there is some pre-existing issue within the library that is setting it to be such in some cases that are still currently unknown. This is irrelevant to this specific issue and a new issue should be created upon discovery of the root cause.
Known affected endpoints
PATCH/channels/{channel.id}Update a channel's settings. Returns achannelon success, and a 400 BAD REQUEST on invalid parameters. All JSON parameters are optional.
If the library thinks the part hasn't been created yet, it will overwrite the part located at whatever offset was passed as the original part's discriminator. This is ALWAYS incorrect any time the response has a discrim that differs from the original part's because Discord's discrims are immutable. The issue here becomes most evident when a user is trying to update a channel but a new channel is returned unexpectedly because, instead of a PATCH request, a POST request resulted instead, and therefore users will end up acting on a different channel than the one they intended to, which should have been the same one they started with.
POST/guilds/{guild.id}/channels
Create a newchannelobject for the guild. Requires the MANAGE_CHANNELS permission. If setting permission overwrites, only permissions your bot has in the guild can be allowed/denied. Setting MANAGE_ROLES permission in channels is only possible for guild administrators. Returns the newchannelobject on success. Fires aChannel CreateGateway event.
The ID of the original part would be entirely discarded as objects that don't exist will have one assigned by Discord when it is created, therefore saving a part that doesn't have a discrim already associated would be auto-filled with the discrim that's returned by Discord. This should be a non-issue because the part is filled by the new id with $part->fill((array) $response);, but the function does not honor this new id and instead uses the old one if for whatever reason one was already set. This would normally be due to user error, however as shown above with the PATCH endpoint, this becomes a library issue when a call that should have been a PATCH suddenly becomes a POST.
@SQKo has told me that parts do not have a discrim property so this is not the issue, however because this is easily replicable it's clear that's simply not the case. It's possible some part of the HTTP library is setting it, however I'm not exploring that possibility. Whatever is setting the discrim/id is irrelevant to this issue.
Conclusion
A newly created part doesn't imply that the original part being saved replaced the part that exists in the cache with the same discrim, $part was actually created, or that the original part that was located in the cache before being overwritten doesn't exist anymore, therefore the behavior of the AbstractRepository's save function is wrong on its face.
Proposed fix
Abstract the method more to handle the request to save the part differently based on what method was called, then inherit ids properly. This won't resolve the issue with $part->created potentially being incorrect, but will stop parts from being placed into the cache incorrectly even if the cached part or new part has a different discrim.
The ->save() function can easily avoid the above issue by constructing a new Part from the data retrieved by the http lib, save that where it belongs in the cache using its discrim/id, and return the new object instead of updating the original part's created property to true and expecting users to be able to use that as if it was what Discord had provided.
Alternatively, the ->save() function can simply create a new part and override the original $part in its entirety by passing it by reference. This would allow users to continue using the functionality without any changes whilst still ensuring the original part is updated correctly to reflect the new one.
Current Workaround
Assuming the original channel has not changed, clone the original part and overwrite the parts into their proper place in the repository. Please note the code below will not work if using an external cache, in which case $channels->cache->set($channel->id, $channel) should be used instead.
$channel = $guild->channels->get('id', '123');
$original_channel = clone $channel;
// Make changes to $channel here
return $channel->guild->channels->save($channel, 'Player count update')->then(function ($new_channel) use ($original_channel): void
{
if ($new_channel->id !== $original_channel->id) {
//$this->logger->warning('Channel ID mismatch: ' . $new_channel->id . ' !== ' . $original_channel->id);
//file_put_contents('original_channel.txt', print_r($original_channel, true));
//file_put_contents('new_channel.txt', print_r($new_channel, true));
$new_channel->guild->channels->offsetSet($original_channel->id, $original_channel);
$new_channel->guild->channels->offsetSet($new_channel->id, $new_channel);
}
});
Alternatively, if the user intends to work with the original channel rather than a new one, userland code may choose to retrieve the original channel from Discord within the promise scope outlined above. This method is not recommended for those who are aware of making numerous API calls or have bandwidth-sensitive concerns, but it will ensure that the client has the most up-to-date data.
$channel = $guild->channels->get('id', '123');
$original_channel = clone $channel;
// Make changes to $channel here
$channel->guild->channels->save($channel, 'Player count update')->then(function ($new_channel) use ($original_channel)
{
$callable = function ($channel)
{ // The thing you want to do
$channel->sendMessage('Hello!');
};
if ($new_channel->id !== $original_channel->id) { // New channel overwrote the original
$channel->guild->channels->fetch('123', true)->then(function ($fixed_channel) use ($callable) // Fix the old part by getting the correct info from Discord. Passing true as the 2nd variable skips checking the cache.
{ // Fix the channel before running your code
$callable($fixed_channel);
});
} else { // The channel matches, so just run your code.
$callable($new_channel);
}
});
Proposed Fix
Only update and return the original $part when it is appropriate to do so, otherwise create a new part and return it instead.
return $this->http->{$method}($endpoint, $attributes, $headers)->then(function ($response) use ($method, $part) {
switch ($method) {
case 'patch': // Update old part
$part->created = true;
$part->fill((array) $response);
return $this->cache->set($part->{$this->discrim}, $part)->then(fn ($success) => $part);
default: // Create new part
$newPart = $this->factory->create($this->class, (array) $response, true);
return $this->cache->set($newPart->{$this->discrim}, $newPart)->then(fn ($success) => $newPart);
}
});
RFor the purpose of everyone's sanity when reading this issue please note that id, discrim, and discriminator can be used interchangeably. Offset, which may or may not be a discrim, is specifically used when referring to the key used to store the data in collections, repositories, or using the cache interface.
https://github.com/discord-php/DiscordPHP/blob/a8a617dcc6f55e15dc2ec43d97d7836d3fbaedca/src/Discord/Repository/AbstractRepository.php#L186-L219
AbstractRepository's issue The library will always explicitly overwrite the cache's parts in the same offset where the original part that was saved is located. This causes old parts that still exist to be overwritten when Discord returns a part with a different discrim. This can happen whenever a part's created property is set to false even when it exists on Discord' side, but also when a part already partially exists in the cache and the part being saved has an existing but differing discrim.
What introduced the issue This bug was likely introduced prior the #895 when the cache interface was first implemented ~2 years ago. I discovered and reported the symptoms of the issue prior to then but a fix has never been introduced because of how hard it was to replicate the issue. The original behavior only updates the original $part, pushes it into the cache in a manner which would appropriately set the correct discrim, and returns it immediately. The new behavior is for the part to be updated, set in the cache using a method that would not appropriately set the correct discrim if a new part was returned, and then the part is returned to the user inside of a new promise scope.
This new logic flow may not be happening synchronously (as is the nature of the promise library) potentially allowing for data to be manipulated between the item being set to the cache and the next promise fulfilling and returning the result to the user. This could be anything from an external app touching the cache to Discord firing a channel create or update event. This is still not a confirmed root cause for part corruption and additional research will be needed to figure out where exactly the data manipulation is happening (if at all), although userland implementations can be completely ruled out due to being able to replicate without configuring an external cache. Regardless as to when the issue is happening is largely irrelevant for fixing this issue, however, and the incorrect functionality still needs to be fixed.
Why this is a library issue The behavior as defined in AbstractRepository's save function, as it stands currently, is incorrect whenever a PATCH or POST request are made regardless of the context it was called in. Because part IDs are immutable, if Discord returns a part with a discrim that does not match the discrim of the original part, it should never overwrite the old part in the cache in the same offset but rather it should save a new part in the offset that is dictated by Discord and a new part (not the same one that was passed into
repository->save() as a parameter) should be constructed using the fetched data and returned.Reproduction steps
It is important to note here that my original code does not set $channel->created to false, but rather there is some pre-existing issue within the library that is setting it to be such in some cases that are still currently unknown. This is irrelevant to this specific issue and a new issue should be created upon discovery of the root cause.
Known affected endpoints
PATCH/channels/{channel.id}
Update a channel's settings. Returns achannelon success, and a 400 BAD REQUEST on invalid parameters. All JSON parameters are optional.If the library thinks the part hasn't been created yet, it will overwrite the part located at whatever offset was passed as the original part's discriminator. This is ALWAYS incorrect any time the response has a discrim that differs from the original part's because Discord's discrims are immutable. The issue here becomes most evident when a user is trying to update a channel but a new channel is returned unexpectedly because, instead of a PATCH request, a POST request resulted instead, and therefore users will end up acting on a different channel than the one they intended to, which should have been the same one they started with.POST/guilds/{guild.id}/channels
Create a newchannelobject for the guild. Requires the MANAGE_CHANNELS permission. If setting permission overwrites, only permissions your bot has in the guild can be allowed/denied. Setting MANAGE_ROLES permission in channels is only possible for guild administrators. Returns the newchannelobject on success. Fires aChannel CreateGateway event.The ID of the original part would be entirely discarded as objects that don't exist will have one assigned by Discord when it is created, therefore saving a part that doesn't have a discrim already associated would be auto-filled with the discrim that's returned by Discord. This should be a non-issue because the part is filled by the new id with$part->fill((array) $response);, but the function does not honor this new id and instead uses the old one if for whatever reason one was already set. This would normally be due to user error, however as shown above with the PATCH endpoint, this becomes a library issue when a call that should have been a PATCH suddenly becomes a POST.@SQKo has told me that parts do not have a discrim property so this is not the issue, however because this is easily replicable it's clear that's simply not the case. It's possible some part of the HTTP library is setting it, however I'm not exploring that possibility. Whatever is setting the discrim/id is irrelevant to this issue.
Conclusion A newly created part doesn't imply that the original part being saved replaced the part that exists in the cache with the same discrim, $part was actually created, or that the original part that was located in the cache before being overwritten doesn't exist anymore, therefore the behavior of the AbstractRepository's save function is wrong on its face.
Proposed fix Abstract the method more to handle the request to save the part differently based on what method was called, then inherit ids properly. This won't resolve the issue with $part->created potentially being incorrect, but will stop parts from being placed into the cache incorrectly even if the cached part or new part has a different discrim.
The ->save() function can easily avoid the above issue by constructing a new Part from the data retrieved by the http lib, save that where it belongs in the cache using its discrim/id, and return the new object instead of updating the original part's created property to true and expecting users to be able to use that as if it was what Discord had provided.
Alternatively, the ->save() function can simply create a new part and override the original $part in its entirety by passing it by reference. This would allow users to continue using the functionality without any changes whilst still ensuring the original part is updated correctly to reflect the new one.
Current Workaround Assuming the original channel has not changed, clone the original part and overwrite the parts into their proper place in the repository. Please note the code below will not work if using an external cache, in which case
$channels->cache->set($channel->id, $channel)should be used instead.Alternatively, if the user intends to work with the original channel rather than a new one, userland code may choose to retrieve the original channel from Discord within the promise scope outlined above. This method is not recommended for those who are aware of making numerous API calls or have bandwidth-sensitive concerns, but it will ensure that the client has the most up-to-date data.
Proposed Fix Only update and return the original $part when it is appropriate to do so, otherwise create a new part and return it instead.