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

Sorting on deeply nested resources #43

Merged
merged 2 commits into from
Mar 8, 2018

Conversation

flmg
Copy link
Contributor

@flmg flmg commented Jan 5, 2018

Hi,

/persons?include=pets,pets.toy&sort=-pets.toy.type

didn't work and produced the following query:

select "person".* from "person" left outer join "pet" as "pets" on "person"."id" = "pets"."person_id" left outer join "toy" as "toy" on "pets"."id" = "toy"."pet_id" order by "pets"."toy"."type" desc

I used the formatRelation function so that the order by part is now correct:

order by "toy"."type" desc

As usual, I added a test.

Cheers,

src/index.js Outdated
@@ -554,7 +554,10 @@ export default (Bookshelf, options = {}) => {

_forEach(sortValues, (sortBy) => {

internals.model.orderBy(sortBy, sortDesc.indexOf(sortBy) === -1 ? 'asc' : 'desc');
internals.model.orderBy(
sortBy && internals.formatRelation(sortBy),
Copy link
Collaborator

@alechirsch alechirsch Jan 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure about this line, why not only pass in internals.formatRelation(sortBy)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case where sortBy is an empty string or null/undefined, formatRelation(sortBy) returns modelName. which makes the query fail.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that is the case, then I think we should put a conditional around the orderBy:

if (sortBy){
internals.model.orderBy(...)
}

@flmg
Copy link
Contributor Author

flmg commented Jan 18, 2018

@alechirsch Sorry for the delay, I was busy. I added the test around the sort value!

@alechirsch alechirsch merged commit e38a04a into scoutforpets:master Mar 8, 2018
@alechirsch
Copy link
Collaborator

@jamesdixon Would you be able to look at a couple of the other PRs and consider publishing a release?

@jamesdixon
Copy link
Contributor

@alechirsch unfortunately, I just don't have the time. I'd be more than willing to add you as an npm publisher if you give me your npm username. Sorry I can't be more help, but I have so many other priorities at the moment.

@alechirsch
Copy link
Collaborator

@jamesdixon, thanks for the offer. My npm user name is @alechirsch.

@jamesdixon
Copy link
Contributor

@alechirsch you should be all set to publish. Thank you!

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

Successfully merging this pull request may close these issues.

None yet

3 participants