Skip to content

Conversation

@antoniopresto
Copy link
Contributor

I'll update tests, for now, is this acceptable?

I need beforeQuery to handle custom user/regions/etc rules.

@nodkz
Copy link
Member

nodkz commented Oct 23, 2019

I'll try to review it after several hours.

I had a fast look and your improvement quite interesting 👍

@nodkz
Copy link
Member

nodkz commented Oct 23, 2019

Yep, graphql-compose-mongoose needs in this changes 👍🔥 My thoughts

1) I think that we need to pass mongoose model to

- const res = await beforeQueryHelper(resolveParams).exec();
+ const res = await beforeQueryHelper(resolveParams, model).exec();

It helps to recreate query from scratch, make additional queries, check what models are using for cases when one beforeQuery are used for different models.

2) We need to hide exec() in beforeQueryHelper

It helps mock results or return data from other sources eg cache.

- const res = await beforeQueryHelper(resolveParams, model).exec();
+ const res = await beforeQueryHelper(resolveParams, model);

PS. Count resolver should be rewritten.

- resolveParams.query = model.find();
- return beforeQueryHelper(resolveParams)
-          .countDocuments()
-          .exec();
+ resolveParams.query = model.countDocuments();
+ return beforeQueryHelper(resolveParams, model)

await UserModel.countDocuments().where({ name: 'userName2' }) works perfectly (firstly applies where condition then makes count)

3) Tests, need more tests )

@antoniopresto
Copy link
Contributor Author

Nice! Thanks for the quick response. I'll take a look at this right now

@antoniopresto
Copy link
Contributor Author

antoniopresto commented Oct 24, 2019

Hi @nodkz! Do you see any disadvantages in put the model in ExtendedResolveParams?

@nodkz
Copy link
Member

nodkz commented Oct 24, 2019

@antoniopresto Yep, put the model to ExtendedResolveParams. I wanted to suggest the same solution.

@antoniopresto
Copy link
Contributor Author

implemented more tests :) I think its complete

it('should call `beforeQuery` method with non-executed `query` as arg', async () => {
const mongooseActions = [];

UserModel.base.set('debug', function debugMongoose(...args) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice👍 didn't know such spying way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discovered this week haha

resolveParams.model = model; // eslint-disable-line
projectionHelper(resolveParams);
return resolveParams.query.exec();
return beforeQueryHelper(resolveParams, model);
Copy link
Member

Choose a reason for hiding this comment

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

Forgot to remove second arg (model)?
All resolvers affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch :)

@nodkz
Copy link
Member

nodkz commented Oct 24, 2019

Great work! 👍
After removing model I'm ready to merge. But can do it only tomorrow (in 9 hours).

PS. Any example in readme will be very appreciated but not obligatory.

validate beforeQueryHelper returning query

beforeQueryHelper: hide exec + pass mongoose model

improve tests
@antoniopresto
Copy link
Contributor Author

Thanks! Your work is inspiring. I can try to help with the readme, but my english doesn't help much hehe

@nodkz nodkz merged commit 8bfefc4 into graphql-compose:master Oct 25, 2019
@nodkz
Copy link
Member

nodkz commented Oct 25, 2019

Very cool PR! Thanks! 🙏

@nodkz
Copy link
Member

nodkz commented Oct 25, 2019

🎉 This PR is included in version 7.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nodkz nodkz added the released label Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants