Skip to content
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.

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

[9.x] Strict mode for RedisStore: preserve type of numeric values #38933

Closed
wants to merge 2 commits into from

Conversation

halaei
Copy link
Contributor

@halaei halaei commented Sep 23, 2021

In order to optimize memory consumption and make RedisStore::increment() and RedisStore::decrement() work using the built-in Redis incrby and decrby commands, numeric values are stored as their base 10 string representation. Sadly, it causes a type inconsistency issue. If we get a numeric string from Redis, we can't know if the original value was string or int/float. Currently Laraval returns all numeric values as string:

Cache::put('k', 1);
dump(is_int(Cache::get('k'))); // Must be true, but it is false.
dump(is_string(Cache::get('k'))); // Must be false, but it is true. 

This PR tries to fix this type inconsistency by adding a "strict" mode option to Redis cache configuration, with the default value being true. It may be a breaking change. If the non-strict behavior is more appropriate for an existing application, strict should be set to false.

Alternative choices regarding backward compatibility and the default behavior:

  1. Don't give any config option to disable the strict mode. Basically it was a bug to not preserve types, so why giving option to stick with the bug? Besides, other cache drivers don't have such option.
  2. Make sure by default strict mode is disabled so it doesn't cause an extra pain during the major upgrade. In case you choose this option you may consider sending a PR to 8.x as well. But be aware of the optional parameter to the constructor of RedisStore.

Let me know if you prefer an alternative choice or the current PR. I can add some relevant tests later.

Note: incrby and decrby Redis commands doesn't work with float values. Also Redis doesn't store non-integer numbers in a compressed way.

Fixes #31345

@@ -342,6 +355,10 @@ protected function serialize($value)
*/
protected function unserialize($value)
{
if ($this->strict && filter_var($value, FILTER_VALIDATE_INT)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be:

if ($this->strict && filter_var($value, FILTER_VALIDATE_INT) !== false) {

filter_var('0', FILTER_VALIDATE_INT) evaluates to 0 which is PHP falsy so the string '0' is unexpectedly returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @derekmd Fixed

@halaei
Copy link
Contributor Author

halaei commented Sep 24, 2021

There are some other inconsistencies on how cache drivers treat numbers and increment/decrement functions:

  • They may or may not preserve data types.
  • The type of numbers they support might be signed, unsigned, integer, float, or even something else.
  • If the key is missing in the cache, increment and decrement does nothing in some drivers, set the value to the increment/decrement value in some, and 0 in some others. If any value set, the key wont expire.

Not all of the inconsistencies can be solved, but some can.
We can also add some notes to the documentations regarding these issues.

@driesvints
Copy link
Member

I'm going to close this for now. Feel free to resubmit once you're ready to work on this again 👍

@driesvints driesvints closed this Oct 5, 2021
@halaei
Copy link
Contributor Author

halaei commented Oct 5, 2021

@driesvints I haven't come to a conclusion on how the issues should be handled. Is it OK to just add some notes to docs? e.g:

{note} The supported numeric data types for increment and decrement methods varies across the drivers. For example, Memcached supports unsigned integers, but not negative or float numbers.
{note} Some drivers doesn't preserve the type of numeric values.
{note} The behavior of calling increment and decrement on missing keys varies across drivers.

@driesvints
Copy link
Member

@halaei just feel free to attempt a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants