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

Add Eloquent adapter query hooks #292

Open
lucianholt97 opened this issue Jan 31, 2019 · 9 comments
Open

Add Eloquent adapter query hooks #292

lucianholt97 opened this issue Jan 31, 2019 · 9 comments
Labels
Milestone

Comments

@lucianholt97
Copy link
Contributor

I was wondering where is the best place to add some custom joins to the index query?
Currently, I am overwriting the queryAll method in the Adapter but it doesn‘t feel right.
Is there any function or hook which is better suited?

Thanks in advance!

@lindyhopchris
Copy link
Member

Hey! queryAll would seem like the right place. The thing to be aware of is that method is also used when the resource appears in a to-many relationship (the $query will be an Eloquent relationship query).

Do you have any suggestions as to how it would be better to add joins? What is your join being used for?

@lucianholt97
Copy link
Contributor Author

Thank you! I would like to query relationship counts and for example the customer_value (the sum of all related orders for a customer) in an efficient way. Additionally, sometimes it is more efficient to add the customer_name as an attribute directly to the order. That way I don't need a separate query for the related customer when I only need the name. I'd love to see a function, like the filter() function, where I can easily extend the query without overwriting anything.

@lucianholt97
Copy link
Contributor Author

lucianholt97 commented Mar 1, 2019

Hey @lindyhopchris ! Do you have any update to this issue or do you plan to include a separate function? Because it seems like the queryAll() or with() method does not affect the read request for a single resource. I would like to avoid to duplicate my logic for the read method.

@lindyhopchris
Copy link
Member

Hi! I'm not adverse to adding a new hook in, but I'm not sure what this hook would be, and under what circumstances it would get called? That would need to be clear for me to add it in - do you have any suggestions?

One thing to mention is that in these kind of scenarios, we've implemented it by putting all these values in a related resource that the client can choose to include or not. So for example, we'd have customers resource, and that would have all the attributes to do with the order's customer. Then that gets included only if the client asks for it to be included. Resources don't need to match your Eloquent models. The thing that makes me think that might apply in your scenario is this comment:

Additionally, sometimes it is more efficient to add the customer_name as an attribute directly to the order.

The purpose of the JSON API spec is to give the client control over what it is asking for. How are you determining when to put the customer name attribute on the order or not? Whether attributes are in the response or not is defined in the spec using the fields query param.

(Sorry for all the question - just trying to figure out what the best way to approach this use case is!)

@lucianholt97
Copy link
Contributor Author

Hey @lindyhopchris,
we would need a function (e.g. join()) which is called from the AbstractAdapter's query() and the find() function and should have the $query as an argument. You would then override the join() method in the Adapter and like:

public function join($query) {
$query->join('customers', 'orders.customer_id', '=', 'customers.id')
->addSelect(DB::raw("CONCAT(customers.given_name, ' ', customers.family_name) AS customer_name"));
}

To use the newly selected attribute, you can simply add the customer_name to the Schema and the fieldset selection from the client will work just fine. The client would even be able to sort by those new columns after adding them to the $sortColumns array and implementing #303 .

Another important use case for me is to add custom selection, like concatenating the customer name to the full_name on the fly or add counts to my resource. Therefore the method could also be named addSelect().

@lindyhopchris
Copy link
Member

lindyhopchris commented Mar 13, 2019

Ok. I think that join is too specific a use-case name for a hook that could be used for multiple use-cases.

It's more about how you are querying the resource, so the hooks I think should be:

  • querying: always called
  • queryingOne: called only when querying a single resource e.g. /comments/123
  • queryingMany: called when querying zero-to-many resources, e.g. /comments and /posts/1/comments

@lindyhopchris lindyhopchris changed the title Best place for custom joins? Add Eloquent adapter query hooks Mar 13, 2019
@lindyhopchris lindyhopchris added this to the 1.1.0 milestone Mar 13, 2019
@lucianholt97
Copy link
Contributor Author

I totally agree with you. The naming and separation of the hook looks perfect.
Thank you in advance for the implementation! 🙏

@lucianholt97
Copy link
Contributor Author

Hi @lindyhopchris, are there any updates on this issue?
Would really appreciate a querying hook.

@lindyhopchris
Copy link
Member

No update I'm afraid - combination of too much other work going on, and me being in two minds about adding these querying hooks, or supporting them in some other way.

Let me have another think about these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants