Loading by default something that is not a relation

[IlCallo]

Here's the scenario: on one of my models I have an accessor to retrieve some multimedia related indirectly to it (aka I cannot setup a relation). The property given by the accessor is a collection of models and I want it to be loaded by default.

I tryed to put it like

protected $load = [
    'multimedia' => MultimediaTransformer::class
];

but it complained about multimedia not being a relation. I then checked the docs and tryed without the mapping and just the name, still relation error. I thought that probably without the includeMultimedia method it tries to find the multimedia() method by default, so I wrote it to override the default behaviour

public function includeMultimedia(EntityModel $entity)
{
    return $this->resource($entity->multimedia);
}

Still giving the relation error even at this point. Is it impossible to load by default something that is not a relation even when defining the includeXxx method? And I guess that base fractal options, like defaultIncludes property, are not usable when using laravel-responder, right?

[flugg]

Oh, this is the auto-eager loading kicking in, which uses the $load properties to find all nested relations. If you don’t use the an includeXx method, the package will assume it’s a model, so I think a solution is to just filter out all relations where an includeXxx method exists when getting eager loads.

And yeah, the $availableIncludes and $defaultIncludes are ignored by the package.

[IlCallo]

This make sense, but you could also re-enable $availableIncludes and $defaultIncludes merging their values after having eager loaded the relations. I think this way could be better because it avoids adding complexity to your code (every exception you make in your flow will probably fire back in some time on unexpected user scenarios) and you are actually extending Fractal instead of masking it out.

Of course, if your aim with this library is exactly to mask out Fractal, your proposed fix is the right one. But I don't know how the library works under the hood, so if the eager load is actually useful and possible only when there is no includeXxx method, then it's better doing it your way I guess.

[flugg]

The idea has been to mask out Fractal (with the exception of the serializers, which already kind of are decoupled) from the point of view of the user. Almost all parts of the package which utilizes Fractal are built with the idea that sometime in the future we can swap Fractal out for something else (for instance Laravel's own resources - more on that in this issue), but leave the API the same. The extracting of eager loads only happens once and adding an extra array_filter shouldn't add too much complexity.

[IlCallo]

Oh well, ok then!

[flugg]

Hmm, I've been tinkering a bit with this and as long as you use the $load property with transformer mappings, I can get things working fine. However, this issue is closely related: https://github.com/flugger/laravel-responder/issues/73. So the same thing is happening for relations you include with with() as well, because of the eager loading.

I'll try to explain the problem more in-depth. Let's say we request relation 'foo' of a hypothetical resource using the with() method. The resource's transformer also has a default relation 'bar' => BarTransformer::class in its $load property, and the BarTransformer has a default relation of 'baz'. Now, we need to eager load 'foo' and 'bar.baz', but the package only knows of 'foo'. So it needs to investigate by going into the transformer and read the $load property, where it finds 'bar', which is mapped to a BarTransformer. It then checks that transformer for its $load property and finds 'baz'. It can now eager load all three relations before executing the transformation.

However, let's imagine 'baz' has another default nested relation of 'qux', but the BazTransformer has an includeQux method, so this should not be eager loaded. When it checks the BazTransformer, it will find the method, skip the 'qux' relation and continue the search.

This all works fine, but now let's imagine 'bar.baz.quz' wasn't loaded by default, but requested using the with() method: with('foo', 'bar.baz.qux'). Since there are no transformer mappings for the $relations property, the package would have no clue what transformers to investigate for 'baz' or 'qux'.

I see two possible solutions: Either make people add a transformer mapping to all relations in the $relations property too. Or, add adontEagerLoad() method to explicitly ignore certain relations. However, to also account for https://github.com/flugger/laravel-responder/issues/73, the package should never auto eager load relations that are not part of the transformer's $relations or $load properties. Considering this, I think the first option of adding transformer mappings to $relations is the only valid solution here. A bit more verbose, but at least it's consistent with the $load property, what do you think?

[IlCallo]

I think that I found strange there wasn't a mapping in $relations in the first place, I was wandering why in $load and not there...

Question strictly related: what about bindings done with $this->app->make(TransformerResolver::class)->bind()? If there is an explicit mapping set in the transformer which one wins? If there is no explicit mapping, the one defined in AppServiceProvider is used anyway? I tried it without explicit mappings on the transformer and it worked just fine. Maybe you could decide to rely mostly on those bindings, maybe using a slightly better syntax, leaving the explicit declaration in transformer as an "override" property, if actually needed. In Laravel there are plenty of examples of features relying heavily on a centralized binding in a provider, like Policies, Events, Relation morphMap etc.

Those are just general thoughts, but if I understood well the problem removing the explicit binding from all transformers and putting them in single centralized method should resolve the problem: you would always know which Model is related to which Transformer.

This also perfectly address the scenario in which a relation contains different kind of models: I'm dealing with it right now and with the centralized bindings without defining any mapping on the transformer it seems to work flawlessy. With explicit mappings that scenario could not be addressed at all, right?

Little side-effect: in this way it becomes more difficult to support multiple Transformers for a single Model while loading relations, but I don't know if it can be an useful scenario, expecially when conditional fields and conditional pivot data will be implemented.

[flugg]

The problem is that I don't yet have the related models because I need to verify they're allowed through their transformers before eager loading them. Which means the explicit binding doesn't help. We're basically looking at finding a transformer solely based on the root transformer and the relation identifier.

The mappings can be optional, and if they're skipped the package will not eager load the nested relations. However, the eager loading is often invisible to the user unless you use a package to see all executed queries, so I'm afraid people might miss it and not realize they're getting the n+1 problem. Then again, I feel that's a safer option than just eager loading everything blindly and hoping for the best. I'm imagining a scenario where the user requests 'user.delete' as a relation. If there is no mapping from the UserTransformer to the delete relation, the package will in its current state, call delete() method on the user model in an attempt to load the relation. In older versions of Laravel this would actually delete the model! However, newer versions has a check when eager loading testing that the method actually exists on the model (and not the query builder). You may have other methods on your model which you don't want people to access though. The package also has a warning in the documentation about using the wildcard for just this reason.

I think allowing transformer mappings on both $relations and $load would solve most problems. This also allows you to have different transformers for related models than root models. I believe the transformer mappings should take effect over the TransformerResolver bindings or the Transformable contract.

[IlCallo]

The problem is that I don't yet have the related models because I need to verify they're allowed through their transformers before eager loading them.

Right, I forgot about that.

The package also has a warning in the documentation about using the wildcard for just this reason.

About this, I think that if you have safeness in mind, you should set an empty array as default instead of the wildcard, like Laravel does with mass assignable fields (which are guarded by default). The warning in the documentation could be not enough to make people realize the possible related security issues.

I believe the transformer mappings should take effect over the TransformerResolver bindings or the Transformable contract.

I agree with you on this.

This also allows you to have different transformers for related models than root models.

I've not fully understood this sentence.

Little side-effect: in this way it becomes more difficult to support multiple Transformers for a single Model while loading relations

Were you talking about this scenario I depicted?

This also perfectly address the scenario in which a relation contains different kind of models: I'm dealing with it right now and with the centralized bindings without defining any mapping on the transformer it seems to work flawlessy. With explicit mappings that scenario could not be addressed at all, right?

What about this? I was talking about morphable relations, btw. May it be possible to define multiple Transformer mappings for one relation in order to eager load also the morphable relations or is it too much? Something like

protected $load = [
    'morphable' => [UserTransformer::class, ActivityTransformer::class],
];

[flugg]

Where you talking about this scenario I depicted?

Yes, indeed! You're right though that with conditional attributes this might not be too common, and it might be a bit repetitive to bind transformers for both root models (with TransformerResolver or Transformable) and related models (using $relations and $load). However, I don't find it too bad and can live with a bit duplication, for the flexibility you get. The only other option I can see is to remove the auto eager loading and let the user eager load before sending the data to the response builder. However, then you end up having to repeat the relations twice for every response.

About the morphable mapping, it looks good, but how would the package know whether to load the UserTransformer or ActivityTransformer?

[flugg]

Oh, and yes, I agree with you about the security aspect. I'll get that fixed :)

[IlCallo]

However, then you end up having to repeat the relations twice for every response.

Yeah, this is worse

About the morphable mapping, it looks good, but how would the package know whether to load the UserTransformer or ActivityTransformer?

That's the problem: morphable relations can contain both those models at the same time, so you must load relations you find on both transformers. I guess that with a left join or full join instead of a normal join it's possible to get something done, maybe checking how Laravel eager-loads relations of morphable relations in Eloquent can suggest some way to solve this problem.

[IlCallo]

Could I suggest to check for attributes, when no relations with the requested name are found? Eager-load is lost because accessing properties cause possible magic methods to resolve and do the request on their own, but in some cases there is an array/collections of models stored into a property which would be accessible with a simple include like

public function includeCollection(EntityModel $entity)
{
    return $this->resource($entity->collection);
}

that can be avoided with a check just after the one for relation existence. The error at that point should become "No relations/attributes matching :name has been found on model :model" or something similar

[flugg]

I've read this thread a couple times now, but not exactly sure what the exact problem here was again. Can you check if this is solved with the latest release?

[IlCallo]

Well, there are some lot of things in this issue actually... Maybe some could be put in a separated issues, like morphable mapping feature. I think that the thing I was trying to explain here is still not possible, but I'll check in these days.

[flugg]

Yeah, would be nice you could open an issue on the morphable feature, not sure I fully understand how it should work.

Also, with v3 you now map transformers on $relations in the transformers as well as $load so I believe that regard to this issue has been resolved at least.

[IlCallo]

I've read this thread a couple times now, but not exactly sure what the exact problem here was again. Can you check if this is solved with the latest release?

The problem I made the issue for is not resolved, because it was about using methods and properties (not relations, something like a collection obtained via an accessor) into $relations and $load like they were actual relations (obviously without eager loading and related stuff).

It relates to the proposed resolution workflow I proposed here without the point 3 and 4 (because you found the reference about the camelCase notation on Laravel relations).

The feature I'm thinking could work like this:

1. read the relation to load
2. search the relation
3. ~if not found, generate camelCase name (we don't actually know or care if the original was already in camelCase)~
4. ~search the relation with new name~
5. if not found, search properties with given name
6. if not found, search properties with snake case name

As I pointed out in the very first post, this can be easily done wrapping up the property into an includeXxx method, which is what I'm doing right now, my request was to embed this behaviour directly into the package.

public function includeMultimedia(EntityModel $entity)
{
    return $this->resource($entity->multimedia); <--- multimedia here is a property or a method, not a relation
}

[flugg]

The code for fetching a relationship from a model is located here: https://github.com/flugger/laravel-responder/blob/master/src/Transformers/Concerns/HasRelationships.php#L220. As you can see it resolves the relation as if it was a model attribute and should cover both point 2 and 5. It doesn't, however, check for snake case attributes as pointed out in point 6.

The problem is, if a relationship exists but there is no records null will be returned. Basically, all attributes on a model that don't exist will return null. This could become problematic if we're to check if one exists and proceed to point 6.

[IlCallo]

It doesn't, however, check for snake case attributes as pointed out in point 6.

I'll check again then, it may be that I did the check on a snake_cased attribute 🤔

The problem is, if a relationship exists but there is no records null will be returned. Basically, all attributes on a model that don't exist will return null. This could become problematic if we're to check if one exists and proceed to point 6.

The only problems I can see are when a model has an attribute with a snake_cased name which corresponds to the camel cased name of a relation and the relation is null and a possible attribute with camel cased name is null. To me, it doesn't seems very likely to happen, and when it happens the snake cased attribute will probably be null itself because it will most probably be the attribute on which the relation itself is created (and if the relation is null, the chances are the attribute is null too). But I guess there are ways to check if the snake case attribute actually exists on the model instead of returning it rightaway.

Could you elaborate the problems you were thinking about? Anyway, if you remove the camel_case($identifier) line, the user is free to use the case it prefers and there is no need to change the remaining code, right?

[flugg]

The only problems I can see are when a model has an attribute with a snake_cased name which corresponds to the camel cased name of a relation and the relation is null and a possible attribute with camel cased name is null.

The problem is your last point here will always be true. It doesn't matter which attribute you access from a model, if it's invalid it will still return null. This makes it impossible to distinguish between a nullable attribute and an attribute that simply doesn't exist. In fact, an Eloquent model has no real knowledge of the database structure other than what you specify in the $fillable array, but even that is optional.

[flugg]

The camel_case was added to solve this issue btw: https://github.com/flugger/laravel-responder/issues/85

[IlCallo]

I understand your point now, but if I define a 'attributeName' into $relations doesn't it get the same exact behaviour you are describing, giving the fact that you are calling it as a dynamic attribute even when it is a relation? Or is the underlying idea "The package works with relations, you can try to use attributes but we do not guarantee it will actually work as intended"?

The camel_case was added to solve this issue btw: #85 Yeah, I remember, the list I quoted comes from that issue :)