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

Soft delete will call service.get only once #160

Merged

Conversation

NikitaVlaznev
Copy link
Contributor

@NikitaVlaznev NikitaVlaznev commented Apr 24, 2017

If you have a users service covered with softDelete hook, then there will be 2 users.get calls for every request to it and to eny other service, that requires a populated user, e.g. services that are covered with restrictToRoles.

Since users.get most likely is fetching data from the database it is expensive, because database ops usually are the most expensive operations in request handling code.

It have a significant impact on performance in high load projects.

Second call appears when softDelete hook is calling service.get while already processing service.get request, it is needed to check whether the resource is deleted or not. But after this check we have a complete result already, this result was processed by all succeeding hooks, so we do not need to call service.get second time.

The patch is eliminating unnecessary call while keeping an ability to work in the old way.

The only problem here as i can see is that after hooks will be called twice, it is not guaranteed that they correctly process the same result twice, so it would be great to find an option to return the result skipping other hooks, otherwise it is better to set optimize=false by default.

Tests patch is not the best of possible i'm sure, it would be great to see proposals of how to make it better.

Issue: #161.

@eddyystop
Copy link
Collaborator

Yes we can indeed do a soft-delete get call with one get method call. Thanks!

I think we can reduce code complexity in the PR by removing the optimize option and always only do 1 get. What do you think? Would you mind doing so?

The test failed on "Soft delete will call service.get only once" because:
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

But only on node and not node 6 or node 4. So let's try that again.

@NikitaVlaznev
Copy link
Contributor Author

I have removed the optimize option. Tests now are much simpler.
What about skipping all other hooks? The 'after' hooks have been fired already and, though i think it will work well in most cases, it is generally not safe to give them processed result.
Is there any method to return the result right from the 'before' hook? May be i should add some 'sendResult' callback to the hook object?

@eddyystop
Copy link
Collaborator

eddyystop commented Apr 24, 2017

Once you set hook.result = data; in a before hook, both the actual DB call and the after hooks are skipped.

Let me know if this satisfied the point you raised, so I can merge this PR.

@NikitaVlaznev
Copy link
Contributor Author

If after hooks are skipped, then it is just what is needed.
I think it can be merged.

@eddyystop eddyystop merged commit 6344828 into feathersjs-ecosystem:master Apr 24, 2017
@eddyystop
Copy link
Collaborator

Will be in 3.1.0 when it is pushed to npm.

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

Successfully merging this pull request may close these issues.

2 participants