Returning single record only (postgres)

[paulbjensen]

Hi,

I'm getting a weird bug - when querying a model that has 2 records with Graphql, it is returning the first record only, even though the database is definitely receiving the SQL query without any LIMIT conditions.

I have created a repository to replicate this issue and provide some more detail: https://github.com/paulbjensen/objection-graphql-postgres-issue

On a side note, I would like to say thank you for Objection.js. I used to be a Ruby on Rails developer a long time ago, and this is the closest ORM I've seen in the Node.js space to what ActiveRecord provided way back then.

[ghost]

This same thing happened to me, @paulbjensen, for me it was a query like: { snapshots {name,id} } And I would only get one record at a time.

What fixed it for me was doing instead:

{ snapshotss {name, id} }

It looks strange, but adding the "s" cleared things up right away. Categories also ends in "s", maybe try "categoriess" instead.

[paulbjensen]

@PatrickBeamECR thanks for the tip. I will give it a try in a bit, and try and dig into what is happening there.

[paulbjensen]

So work project ate my homework, but I've checked this now, and can confirm that putting an extra s on the end returns the records, so it looks like a bug with pluralization.

Thank you so much for finding this, I would never have discovered it and just assumed that I was doing something wrong. I will dig into the code a bit more into the source code and see whats going in.

[ghost]

Of course! I found it completely by accident trying to figure the same thing out--held down the 's' a bit too long and ran it. Best of luck to you.

[paulbjensen]

Here's the bit of code that appends the s on the end:

https://github.com/Vincit/objection-graphql/blob/master/lib/SchemaBuilder.js#L119

Going to keep digging around to understand the context around singleFieldName and listFieldName, as I wasn't aware of this distinction.

[paulbjensen]

I'd like to propose this change.

Given a model with a modelClass value of Category, the singleFieldName value should be 'category', and the listFieldName value should be 'categories'.

I will fork the repo and make the change there, and then submit a PR if it's working fine and there's no objection to the idea (I love a good pun).

[paulbjensen]

Actually, I explored the code a bit more and realised that passing some additional options to the model builder worked fine:

const graphQlSchema = graphQlBuilder()
    .model(Category, { fieldName: 'category', listFieldName: 'categories' })
    .build();

One issue that remains though is that although the single field result is one item, the database query is still making a request for all of the records in the database table. Ideally we'd want to pass a LIMIT 1 to the database query if returning a single record to the result

[paulbjensen]

I scanned the code a bit more and realised that 'limit' and 'offset' options were used for the 'range' query argument's function, but had no separate implementations, so I've added them in this PR:

https://github.com/Vincit/objection-graphql/pull/45

There is one lingering question that remains. When requesting a single object for a type, would it make sense for limit: 1 to be applied to this, as it would stop us making a request that fetches all of the records in the database table and returns the first, and actually tells the DB to return 1 record.

[nasushkov]

Hi @paulbjensen , thanks for the PR. I agree, that the default pluralization rules here are primitive and adding 's' to the end of the word doesn't make sense for many cases. I'll think how to make it work for more cases out of the box and maybe utilize some external libraries that solve this problem like pluralize. Regarding the #45, I see that many changes are just formatting. Did you use npm run lint:js before commit?

[paulbjensen]

Hi @nasushkov, I didn't. Apologies, my text editor had Prettier enabled, but it didn't pick up rules on formatting tabs/spaces in the .eslintrc file.

I will make those tweaks to the PR to remove the formatting changes, and potentially make a new PR with just the required changes.

As for the way to apply singular/plural naming of the modelClass in the library, it was possible to do it with tweaking the combination of the _.camelCase and utils.typeNameForModel to achieve the desired singular/plural combinations. I will find that code in a bit.

[paulbjensen]

I'm going to close the PR for #45 and make a fresh PR that doesn't contain extra formatting.

[paulbjensen]

Submitted https://github.com/Vincit/objection-graphql/pull/46

[nasushkov]

Hi @paulbjensen, sorry for a late response. Could you also add tests to the PR #46?

[paulbjensen]

@nasushkov yes, that's done now.