-
Notifications
You must be signed in to change notification settings - Fork 86
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
Loading by default something that is not a relation #84
Comments
Oh, this is the auto-eager loading kicking in, which uses the And yeah, the |
This make sense, but you could also re-enable Of course, if your aim with this library is exactly to mask out Fractal, your proposed fix is the right one. |
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. |
Oh well, ok then! |
Hmm, I've been tinkering a bit with this and as long as you use the I'll try to explain the problem more in-depth. Let's say we request relation However, let's imagine This all works fine, but now let's imagine I see two possible solutions: Either make people add a transformer mapping to all relations in the |
I think that I found strange there wasn't a mapping in Question strictly related: what about bindings done with 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. |
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 I think allowing transformer mappings on both |
Right, I forgot about that.
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).
I agree with you on this.
I've not fully understood this sentence.
Were you talking about this scenario I depicted?
What about this? I was talking about morphable relations, btw.
|
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 About the morphable mapping, it looks good, but how would the package know whether to load the |
Oh, and yes, I agree with you about the security aspect. I'll get that fixed :) |
Yeah, this is worse
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. |
Could I suggest to check for attributes, when no relations with the requested name are found?
that can be avoided with a check just after the one for relation existence. |
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? |
Well, there are some lot of things in this issue actually... |
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 |
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 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).
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.
|
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 |
I'll check again then, it may be that I did the check on a snake_cased attribute 🤔
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). Could you elaborate the problems you were thinking about? |
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 |
The |
I understand your point now, but if I define a
|
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
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 themultimedia()
method by default, so I wrote it to override the default behaviourStill 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?The text was updated successfully, but these errors were encountered: