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

PHP JSON vs MySQL JSON Extra Space Between key:value #46247

Closed
kerbs-ns opened this issue Feb 23, 2023 · 9 comments
Closed

PHP JSON vs MySQL JSON Extra Space Between key:value #46247

kerbs-ns opened this issue Feb 23, 2023 · 9 comments

Comments

@kerbs-ns
Copy link

kerbs-ns commented Feb 23, 2023

  • Laravel Version: 8-10
  • PHP Version: all
  • Database Driver & Version: Mysql 8

Description:

When using json as colum type the way mysql works and php we have issue having isDirty even if we just access attribute.

https://stackoverflow.com/questions/72958866/php-json-vs-mysql-json-extra-space-between-keyvalue

Steps To Reproduce:

create migration
$table->json('video')->nullable();

have cast

 protected $casts = [
        'video' => Asset::class,

Asset::class is just json_encode json_decode nothing special.


attribute
"video" => "{"disk":"s3","path":"content/english/A2/text.mp4"}"
original => mysql
 "video" => "{"disk": "s3", "path": "content/english/A2/text.mp4.mp4"}"

This creates:

> issue for isDirty
> force update when not needed
> create fake audit logs... anything that uses isDirty.

This is not laravel/eloqeunt issue per say. 

But it is pain and the real question is how can we what strategy would if one has json column, since mysql will not:

> guaranty the order of keys
> will have extra space when data is loaded from mysql

This is some pseudo solution that is expensive but hides issue under rug

/**
     * Determine if the new and old values for a given key are equivalent.
     *
     * @param  string  $key
     * @return bool
     */
    public function originalIsEquivalent($key)
    {
        if (!array_key_exists($key, $this->original)) {
            return false;
        }

        if ($key == 'video') {
            return $this->checkOriginalIsEquivalentFlexField($key);
        }

        return parent::originalIsEquivalent($key);
    }

    protected function checkOriginalIsEquivalentFlexField($key)
    {
        $original = Arr::get($this->original, $key);
        $attribute = Arr::get($this->attributes, $key);

        if ($original === $attribute) {
            return true;
        } elseif (empty($attribute)) {
            return false;
        } elseif (empty($original) and !empty($attribute)) {
            return false;
        }

        $o = json_decode($original, true);
        $a = json_decode($attribute, true);

        $o = Arr::dot($o);
        $a = Arr::dot($a);

        ksort($o);
        ksort($a);

        if (json_encode($a) == json_encode($o)) {
            return true;
        }

        return false;
    }`
@driesvints
Copy link
Member

I'm sorry but I'm not sure I see the issue here right now. If you want, you can attempt a PR. Thanks

@kerbs-ns
Copy link
Author

kerbs-ns commented Feb 23, 2023

Issue is that if one has json colum in database and has [laravel Custom Casts ](https://laravel.com/docs/9.x/eloquent-mutators#custom-casts) and just access attribute isDirty will be true.

I have just provided context.

@taka-oyama
Copy link
Contributor

FYI, I have seen similar issue raised here -> #28029.

Kind of a hard issue to solve since MySQL's json format is weird.

@kerbs-ns
Copy link
Author

kerbs-ns commented Feb 24, 2023

@driesvints I have no issue doing PR.

The question is where to or what to do. I have trait that solves it for specific model column. This is not something that most users will not have but...

When you do some enterprise apps you need audit and this is killing it.

From Usage perspective this is clear laravel bug. When you get json colum that is custom casted you get isDirty as true. Laravel is not causer of this issue, agreed. it is more whole ecosystem of mysql8 and php...

One can use text instead of json, but that is big compromise.

So maybe if one can steer me in direction how this PR should be implemented in framework, i can do it.

My question is more. What is ideal way to deal with this. What options are good one

  1. on save to detect json colum and db type to hook in change feels to big
  2. having trait MySqlFixJson feels hacky.
  3. having json_encode(Str::replace('":"', '": "') //feels even more hacky and it want cover issue of mysql ordering keys

@driesvints
Copy link
Member

@kerbs-ns I honestly don't know sorry. I don't see a good solution for this.

@taka-oyama
Copy link
Contributor

@netzknecht
Copy link

netzknecht commented Oct 26, 2023

@kerbs-ns I honestly don't know sorry. I don't see a good solution for this.

We have to extend the database grammer with an json encoder method to be able to emulate the database specific json encoding for mysql as an example.

The method should be used in all grammer context instead of json_encode() and should also be callable as facade DB::jsonEncode() instead of calling json_encode() to encode json data in attribute mutators or attribute casts.

@netzknecht
Copy link

netzknecht commented Oct 26, 2023

FYI, this is how rails solves it...

https://github.com/rails/rails/pull/21110/files#diff-868f1dccfcbed26a288bf9f3fd8a39c863a4413ab0075e12b6805d9798f556d1R1054-R1057

This approach does not take into account the different ordering of JSON elements that results from the normalization of Mysql's JSON data.

But what should work is the recursive calculation of the difference between the original and current data as an associative array with index checking.

This is how it works from my point of view:
10.x...netzknecht:laravel-framework:bugfix-issue-46247

Casts that work with JSON fields must extend the JSON cast class Illuminate\Database\Eloquent\Casts\Json.

netzknecht added a commit to netzknecht/laravel-framework that referenced this issue Oct 26, 2023
netzknecht added a commit to netzknecht/laravel-framework that referenced this issue Oct 27, 2023
netzknecht added a commit to netzknecht/laravel-framework that referenced this issue Oct 27, 2023
netzknecht added a commit to netzknecht/laravel-framework that referenced this issue Oct 30, 2023
netzknecht added a commit to netzknecht/laravel-framework that referenced this issue Oct 31, 2023
netzknecht added a commit to netzknecht/laravel-framework that referenced this issue Oct 31, 2023
@netzknecht
Copy link

netzknecht commented Oct 31, 2023

Casts that work with JSON fields must extend the JSON cast class Illuminate\Database\Eloquent\Casts\Json.

This is no longer necessary with the recent commit.

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

No branches or pull requests

4 participants