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

[Feature Request] Abstract Relation logic to Core to enable better support of Non-eloquent relations #33

Open
DellanX opened this issue Feb 23, 2024 · 5 comments

Comments

@DellanX
Copy link

DellanX commented Feb 23, 2024

Use Case

I have a model, Calendar, which has events from the database (An Eloquent model occasion)
But now, I am adding support for external events, such as an ICalendar Feed (a URL for .ics files)

I can get the data into Laravel pretty easy, and I have a relationship on calendar called occurrences which combines occasions and all external events into a collection. Data has attributes to distinguish the source and ids, so my custom repository can handle CRUD operations.

I need to be able to interact with this via JSON API at the route /calendars/{calendar}/occurrences

Code changes

The issue, is that that the following code checks specifically if the Schema is an Eloquent Schema.

if ($schema instanceof Schema) {

I could resolve this, by making the typecheck compare against LaravelJsonApi\Core\Schema\Schema instead of LaravelJsonApi\Eloquent\Schema

But this opens a whole new can of worms. Thinks like Paginator, and another type check at:

assert($relation instanceof EloquentRelation, sprintf(

Conclusion

I am very much willing to assist in any code development needed here, but it seems to me like a lot of code is going to need to be touched to trick this library to be able to support non-eloquent relations.

I figure, the best bet, is to try and move some of this logic up to the Core Level, so that any LaravelJsonAPI schema.
And my current efforts are to reduce the scope of the Type Checks to the Core Level. That being said, I'll also need to move some of the logic from the eloquent library to the core library, to fix some of these type checks.

Let me know if y'all have any concerns, or if there are any ongoing efforts that I can assist.

@DellanX
Copy link
Author

DellanX commented Feb 23, 2024

Some notes to myself after digging into the code:

My understanding is, that most of this is triggered by the Laravel package at the FetchRelated trait's showRelated method, and that it makes a single assumption, that the store for the related model is the same as the store for the base model.

classDiagram
  class LaravelRepository["Laravel Repository"]
  LaravelRepository: calendars
  LaravelRepository: occasions
  class ICSRepository["ICS Repository"]
  ICSRepository: events
Loading

I could maybe inject data into the relationship field, which would contain information about the store to use for the related data. This would default to the same repository as the base model. This way, I can maybe defer the handling of the related query building unto the ICS Repository instead of trying to cast it to an EloquentRelation for query building.

I'll investigate how NonEloquent -> Eloquent works, to maybe create a smooth developer experience.

@lindyhopchris
Copy link
Contributor

lindyhopchris commented Feb 24, 2024

Thanks for opening this issue.

trick this library to be able to support non-eloquent relations.

So this topic - integration between Eloquent and non-Eloquent resources - is extremely complex. Almost mind-boggling complex 😆

The problem is, when a request comes in, we need to load the entire "tree" of data that is going to be serialised.

For Eloquent, this is about using eager loading through the relationships to load a tree of Eloquent models. I.e. the top-level model or models, and then all the models through the relationships that are being serialised. In Eloquent, these models are stored in the relationships of "parent" models.

The problem we'd need to solve, is how does the data load when it starts with Eloquent models then at some point in the tree hits non-Eloquent models? How do we know how to load the non-Eloquent models in a way that does not cause n+1 problems? Plus once they're loaded, where are they?! As they aren't going to be in the parent Eloquent model's relationships.

Then the same problems works in the other direction. If the top-level resource or resources are non-Eloquent models, how do we handle loading the Eloquent models when we hit them at some point in the tree of data being loaded?

This requires a complete re-writing of the loading data pattern, which will not be simple.

At the moment, this package is in a state of flux while I deal with other "big" new features. At some point in my work through of all those issues, I was going to have to improve the Eloquent loading patterns to assist with implementing a number of new features. I suppose that would be the point at which we could see if this is even solveable.

@skqr
Copy link

skqr commented Feb 25, 2024

@lindyhopchris Firstly, thank you so much for creating this package. It's a life saver.

Secondly, whereas I get the problems you're running into, I do believe those are just Laravel/Eloquent shooting themselves in the foot.

Here's the Symfony 2 bundle I created about ten years ago which implemented the full JSON-API spec back then, and had non-db model support https://github.com/skqr/hateoas/?tab=readme-ov-file#ghosts-ghost

I'm sure it won't help. I'm just super frustrated at reading that this is not possible in Laravel's version after having banged my head with non-Eloquent resources for over a week.

Cheers.

@lindyhopchris
Copy link
Contributor

Thanks for sharing that, interesting to look through that.

Yeah, we are probably too tightly coupled with Eloquent models. The problem with Laravel is that's what most developers expect. You're right that we need to decouple from Eloquent, i.e. have an abstraction that works natively with both Eloquent and non-Eloquent resources.

Definitely agree it needs to be the direction of travel.

Sorry this has caused you to run into problems and banging your head!

@skqr
Copy link

skqr commented Feb 26, 2024

@lindyhopchris absolutely no problem. As with anything, just taking a different approach unblocked me and made this a non-issue, for now.

Looking forward to the evolution of this project. From the (I think, maybe) odd perspective of somebody who's done something similar (albeit on a much reduced scale), I really admire everything you've done so far. Cheers.

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

3 participants