laravel / ideas

Issues board used for Laravel internals discussions.
939 stars 28 forks source link

[proposal] allow indirect modification of overloaded JSON model attributes #1275

Open judahnator opened 6 years ago

judahnator commented 6 years ago

I have been working on a side project where I have multiple JSON columns on a given model. You can see an example of that functionality in the Laravel docs.

As I am sure you folks know, you cannot really manipulate the overloaded JSON object. You have to copy the data, manipulate it, then replace it. Like this:

class myModel extends Model
{
  protected $casts = ['json_field' => 'array'];
}
$json_data = $myModel->json_field;
$json_data->foo->bar = 'baz';
$myModel->json_field = $json_data;

It would be much easier to do this:

$myModel->json_field->foo->bar = 'baz';

Unfortunately that gives an error about indirect modification of overloaded properties.

In another library I wrote I worked around this issue in a bit of a hacky way, but it would be a killer feature to implement natively into Laravel core.

Here is the library I wrote for reference. If you are curious about its workings you can give it a peek, but the short version is that it holds a refrence to the original json string and whenever data is manipulated it updates the original string it was holding onto. What I propose is to to implement something similar to the way I integrated that library into this other library.

I do not believe a change like this would be a breaking change, as the element remains as a literal string in the model attributes array. I might be a bit biased though. My code is perfect according to me and all that.

Is this a feature the Laravel community would be interested in? Not in integrating my specific library, thats just for an example, but in adding the ability to manipulate JSON columns as object directly, instead of indirectly.

If there is interest for this feature, what would be the next steps? Does anyone have feedback, how should we attack this problem, etc. I want to do things "the Laravel way."

mfn commented 6 years ago

With your library, as it is, are you able to integrate it in a way you can actually use it on an Eloquent model?

judahnator commented 6 years ago

Sure, here is an example I am using in one of the projects I mentioned. It depends on this json manipulation library to do its work.

Adding on my example above, you could do something like this and it would work.

<?php

use Illuminate\Database\Eloquent\Model;
use function judahnator\JsonManipulator\load_json;

class MyModel extends Model
{

  protected $fillable = ['json_field'];
  public $timestamps = false;

  public function getJsonFieldAttribute()
  {
    return load_json($this->attributes['json_field']);
  }

}

// Create model
$newModel = MyModel::create(['json_field' => '{"foo":{"bar":"baz"}}']);

// Sanity check, make sure the json value is actually there
echo $newModel->json_field; // {"foo":{"bar":"baz"}}

// Update a nested value
$newModel->json_field->foo->bar = "new value";
echo $newModel->json_field; // {"foo":{"bar":"new value"}}

// Save, to make sure persisting the data works
$newModel->save();

// We can see the data has persisted
echo MyModel::find($newModel->id)->json_field; // {"foo":{"bar":"new value"}}

You can see its as easy as adding a quick accessor. Literally a one-liner, if you don't count the several dozen lines of code working behind the scenes.

It feels like something that should be taken care of by a cast rather than an accessor though.

mfn commented 6 years ago

It feels like something that should be taken care of by a cast rather than an accessor though.

Exactly my thoughts, that's why I was asking. Casts are in reality just shortcuts for accessors (but very convenient ones).

I don't know Eloquent well enough, but to me knowledge casts can't be "just extended" for new types or am I wrong?

However, one thing:

// Sanity check, make sure the json value is actually there
echo $newModel->json_field; // {"foo":{"bar":"baz"}}

// Update a nested value
$newModel->json_field->foo->bar = "new value";
echo $newModel->json_field; // {"foo":{"bar":"new value"}}

I'm surprised this works.

The accessor always returns a new instance per your code. How is it possible the updated value is still reflected on the model? I think I'm missing something.

judahnator commented 6 years ago

I'm surprised this works.

I honestly don't blame you. It looks like I am treating a string like an object and somehow attributes that have no mutator are being mutated.

Lets take on that example on line by line.


// Sanity check, make sure the json value is actually there
echo $newModel->json_field; // {"foo":{"bar":"baz"}}

Here we are "echoing" the results of the accessor. This might be the most straightforward step of the whole ordeal. The accessor returns an instance of the JsonBase class in my library, which implements the __toString() method. That method simply returns back the input for that function, which is a JSON string.

// Update a nested value
$newModel->json_field->foo->bar = "new value";

This is where the magic happens. This line is tricky, there are several things happening all at once. Lets hit them one at a time.

First, $newModel->json_field. This calls the json_field accessor, which returns the result of the load_json function, which is basically just a factory that spits out a JsonBase object. Notice how the first parameter of both the load_json and the JsonBase constructors are asking not for a copy of input data, but a reference to a variable. In our case, we are referencing the 'json_field` string variable on the models attributes array.

Next, ->foo. That hits the JsonObject classes __get() method. That checks what the "foo" element is. It sees that it is a nested object, so it calls the load_json() method on that element and returns that. We are still working with a JsonObject "object", but now we are working specifically with the "foo" element data. It is important to note that when calling the load_json() function on line 18 there that we are passing in a second parameter, load_json($json, $this). The second parameter is important for the next step.

Third, ->bar = "new value";. This is the most complicated step. It calls the JsonObject __set() method. This updates that classes local decoded version of the data, and calls the JsonBase update() method. This sets the pass-by-reference variable we had in the constructor to the json_encode()-d value of the class. It then checks to see if it has a parent (remember the second parameter in the load_json function from the previous step) and calls that classes update method as well.

Quick recap. The 'bar' object updated and triggered an update on the 'foo' object. The foo object performed this action $this->jsonString = json_encode($this);, and because $this->jsonString is a reference to the models 'json_field' attribute the change is immediately reflected directly on the model.

echo $newModel->json_field; // {"foo":{"bar":"new value"}}

See the first step. This creates a new JsonObject class with the now updated 'json_field' model attribute, and returns the string interpretation.


deep breath

The tl;dr version is that since we are working with a reference to the attribute there is no need for a mutator because any changes we make reflect on the original object directly.

$json = '{"foo": "bar"}';
load_json($json)->foo = 'baz';
echo $json; // {"foo": "baz"}

It all make sense?

mfn commented 6 years ago

we are working with a reference to the attribute

I guess this is the missing link.

Nowhere did I realize this, I still haven't found the definition of load_json although I checked your Gitlab repo, e.g. searched for it https://gitlab.com/search?utf8=%E2%9C%93&search=load_json&group_id=&project_id=7679045&search_code=true&repository_ref=master .

The awesome black magic arts of references.

Is the MetadataModel from your repo required to make this work?

judahnator commented 6 years ago

Ha, yeah.

I have been referencing a few different libraries, so ill clear it up a bit.

The MetadataModel is from that second library.

Quick edit: Here is the definition of the load_json() function.

mfn commented 6 years ago

Thanks for clearing this up!

My bottom line of this is:

I imagine a system where a ServiceProvider could do …->registerCast('json_mutator', …) or

protected $casts = [
  'some_json_field' => JsonAttributeMutator::class,

I apologize for taking your specific proposal offtrack a bit, but it triggered this thought process of mine.

judahnator commented 6 years ago

I think a better approach for Eloquent would be the possibility to provide custom cast operations in general (i.e. going on level higher than this specific proposal) instead of adding stuff to core

I think you might be right. Beyond maybe macros there is not much in the way of extending Eloquent Model functionality.

I imagine a system where a ServiceProvider could do …->registerCast('json_mutator', …)

I will admit that I am a bit less knowledgeable when it comes to advanced service provider and application container usage. The only problem I see with that is that if casts are in their own classes they lose protected access to the model. This might be a good thing separation of concerns wise, but it would break my pass-by-reference system that fixes the JSON issue we had in the first place.

Perhaps a system similar to the "Macroable" trait could be a solution. The HasAttribute trait could be extended like so:

protected static $casts = [];

public static function registerCast(string $name, callable $callback): void
{
  static::$casts[$name] = $callback;
}

Then down in the castAttribute() method the default case could be extended like so:

default:
  if (array_key_exists($this->getCastType($key), static::$casts) {
    return static::$casts[$this->getCastType($key)]($key, $value);
  }
  return $value;

I have not tested any of this, but if this were implemented I could integrate this in a service provider something like this:

Model:::registerCast('custom_json_cast', function(string $key) {
  return load_json($this->attributes[$key]);
});

We are moving a bit away from the original topic, but I think we might be onto something here.

judahnator commented 6 years ago

Just a quick update on this. I proposed a few ideas for allowing custom cast registration and extensions to the Eloquent model system, but think I will go forward with a PR for the original topic.

I am just waiting a few days until the PR is finalized. The fact that PRs done in September do not count towards Hacktoberfest has absolutely nothing to do with the delay. Nothing at all...

RoccoHoward commented 6 years ago

I wrote some packages awhile back that implement these things:-

Check them out and see how they can help.

JSON array allowing indirect modification https://github.com/hnhdigital-os/laravel-model-json

Custom cast registrations & extensions (but also brings all the attribute related things together which helps with extensions): https://github.com/hnhdigital-os/laravel-model-schema

(I plan on re-writing my JSON implementation as an extension of the second package)