Open RaduGrama opened 5 years ago
eddyystop
commented
5 years ago Its failing in the FeathersJS hook handler either on the get call or after it returns. Please log the id and params before this line https://github.com/feathers-plus/feathers-hooks-common/blob/master/lib/services/stash-before.js#L29 and log that control gets to before https://github.com/feathers-plus/feathers-hooks-common/blob/master/lib/services/stash-before.js#L31 .
cdelaorden
commented
5 years ago We also have the same issue and have found a workaround.
The reason is that the stashBefore hook is setting a $disableStashBefore property in the params.query object (for reasons I've yet to understand as it feels like a hack, when you have the full context to share info between hooks). It seems that params.query is also send/recognized by the DB adapter get method, and marked as invalid when using feathers-mongoose, in my case.
The outcome of the error is that context.params.before is undefined because the get call fails, defeating the purpose of using stashBefore at all. :)
To prove my point, the simple solution is to use another hook from this package, discardQuery, and set it up before get to discard that parameter. Add it and the error disappears:
Example config:
before: {
get: [discardQuery('$disableStashBefore')],
patch: [stashBefore(), yourHookThatNeedsStash, ...]
}Regarding the reasons of putting $disableStashBefore, if it's there to avoid double stashing, it does seem pretty weird that anyone would be stashing before a get method, because that method is idempotent, nothing can change.
Of course this workaround may have another implications, perhaps @eddyystop you can explain the reasoning or the need of that query parameter to let people know of possible side-effects.
eddyystop
commented
5 years ago Any of the populate hooks will cause another service to read records. We don't want those other services to also try to stash records if they too use the stashRecird hook. The $disableStash flag is set by the root service to prevent this.
It may look hacky but it's traditional Feathers.
On Thu., May 30, 2019, 06:38 Carlos de la Orden, notifications@github.com wrote:
We also have the same issue and have found a workaround.
The reason is that the stashBefore hook is setting a $disableStashBefore property in the params.query object (for reasons I've yet to understand as it feels like a hack, when you have the full context to share info between hooks). Now that is not valid query parameter to send to mongoose, for example.
The outcome of the error is that context.params.before is undefined because the get call fails, defeating the purpose of using stashBefore at all. :)
To prove my point, the simple solution is to use another hook from this package, discardQuery, and set it up before get` to discard that parameter. Add it and the error disappears:
Example config:
before: { get: [discardQuery('$disableStashBefore')], patch: [stashBefore(), yourHookThatNeedsStash, ...] }
Of course this workaround may have another implications, perhaps @eddyystop https://github.com/eddyystop you can explain the reasoning or the need of that query parameter to let people know of possible side-effects.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/feathers-plus/feathers-hooks-common/issues/511?email_source=notifications&email_token=AAL3HH4EW3NM6LTS4BH3TBDPX6VDRA5CNFSM4G4XD442YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWR75RA#issuecomment-497286852, or mute the thread https://github.com/notifications/unsubscribe-auth/AAL3HH5LIEVGKAATVRLPH6TPX6VDRANCNFSM4G4XD44Q .
cdelaorden
commented
5 years ago Any of the populate hooks will cause another service to read records. We don't want those other services to also try to stash records if they too use the stashRecird hook. The $disableStash flag is set by the root service to prevent this. It may look hacky but it's traditional Feathers.
Sure, I'm sorry if it sounded rude on my part, not my intention at all. :)
In that case perhaps the hook should check if something is already in context.params.before or the configured before param to avoid other stashes, instead of setting an incompatible param?
I'm not too familiar with the hooks common code, but the status now is that, at least in some ORM adapters (knex from the original issue, mongoose in my case), using this stashBefore hook throws the error mentioned in this issue's title, preventing the hook from working at all. Do you have any suggestions on the issue itself?
marshallswain
commented
5 years ago I believe the simplest solution to this is to move the $disableStashBefore directive out of the query, and up one level into context.params. This sidesteps the query sanitization process while allowing the rest of the functionality to work as expected.
https://github.com/feathers-plus/feathers-hooks-common/blob/master/lib/services/stash-before.js#L11
So we replace
params.query.$disableStashBeforewith
params.$disableStashBefore
FossPrime
commented
4 years ago I'm in agreement with @marshallswain. The proposed API looks like https://feathers-plus.github.io/v1/feathers-hooks-common/#paramsforserver ... and thus is not unusual. It also allows us to use stashBefore only on patch for instance. Which helps performance and readability.
As a temporary fix, we should update the doc and mention that you MUST have stashBefore on the get before hook, the other placements are optional.
Here's how my patch2Update application hook looks because of this issue:
import patch2Update from './hooks/patch2Update.js'
import { discardQuery } from 'feathers-hooks-common'
export default {
before: {
get: [ discardQuery('$disableStashBefore') ],
patch: [ patch2Update() ]
}
}Here's how it COULD look with marshall's fix:
import patch2Update from './hooks/patch2Update.js'
export default {
before: {
patch: [ patch2Update() ]
}
}Are we taking PR's?
Steps to reproduce
Add
stashBefore()to the hooks of a simple service. The code looks like:Expected behavior
No errors.
Actual behavior
Not sure if this is an issue or misconfiguration.