PATCH doesn't trigger Sequelize instance hooks #155
Comments
|
So, this has kind of been a recurring theme with PRs lately: people are creating multiple code paths to satisfy some edge conditions or micro-optimizations. This creates code that looks like this: if (someCondition) {
doThingsOneWay();
} else {
doThingsAnotherWay();
}These extra code paths do not get tested by the shared adapter tests. This ends up affecting people who accidentally trigger one code path instead of the other without knowing - causing new and unexpected behaviors to "just appear one day". My advise to you is to implement your code in both the "single" and "bulk" hooks. This should be very simple to do and will allow feathers-sequelize to continue to have a single code path for patches: function someCustomFunction (instance) {
// do custom stuff here
return Promise.resolve();
}
MyModel.hook('afterCreate', (instance, options) => {
return someCustomFunction(instance);
});
MyModel.hook('afterBulkCreate', (instances, options) => {
return Promise.all(instances.map(someCustomFunction));
}); |
|
I totally get that you want the code to be simple and stay out the way. The issue I've got with that is that it's acting unexpectedly. One would expect an action on a single record to perform an instance update, and a bulk action to trigger a bulk hook. Worse still, these class hooks lose the instance context. So if the original service.Model has its schema set, this info is lost. That's all the more frustrating (impossible?) if you're chaining hooks or using hooks on nested instances. |
|
It may not be possible to implement the same code in bulk hook instead of single variant, as they might not have access to the instance. For example |
|
@DesignByOnyx @skinofstars Any advice to implement the functionality of My project uses the library sequelize-paper-trail to implement some important features, which unfortunately got broken when I switched from PUT to PATCH method. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Apologies if the issue could not be resolved. FeathersJS ecosystem modules are community maintained so there may be a chance that there isn't anybody available to address the issue at the moment. For other ways to get help see here. |
|
I don't think this should be closed. New users should be aware that the adapter will make Sequelize act differently to its design and existing documentation. |
|
Then we need some actionable steps and someone to implement them. I'm not even sure if that means a documentation or code update in this case. |
|
Personally, I think it's a code change needed. It's not acting as expected and breaking core Sequelize functionality. We currently maintain our own monkey patched version of this module in order to be Sequelize compatible, but we're refactoring to just use Sequelize directly. |
|
Been talking to @alexisabril about a similar thing. Using Sequelize directly in a custom service is definitely starting to look like a more flexible, straightforward and viable approach going forward. Then this adapter could just provide some utility functions and default functionality but you are forced to use the Sequelize models directly. This adapter with all its edge cases and special requirements is definitely one of the more painful Feathers modules to maintain at the moment. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Apologies if the issue could not be resolved. FeathersJS ecosystem modules are community maintained so there may be a chance that there isn't anybody available to address the issue at the moment. For other ways to get help see here. |
|
I have stumbled into this problem too, was wondering why the Sequelize hooks were not being executed. |
|
If you aren't too far down the rabbit hole, then I'd recommend moving any event hooks out of Sequelize and in to the Feathers hooks system. It's easier to maintain and more apparent to the next dev looking at it. |
|
Hello @skinofstars.
{beforeBulkUpdate: function(options) {
options.individualHooks = true
},
//https://github.com/feathersjs-ecosystem/feathers-sequelize/issues/155
beforeUpdate: function(instance: any) {
transformInstance(instance)
}
}I get your point about easier and clearer code using the feathers Hooks system but Sequelize Models just seem like the right place to be doing data manipulation or Validation. |
|
I second @skinofstars recommendation. If you're going to use sequelize, use it just as a query generator/executor. Don't bother with its hooks, eager data loading, or anything else - always favor using feathers hooks for all things business related... which includes data manipulation and validation, relational data loading ("eager" loading), caching, etc. Furthermore - this is a bit out of scope... but I even advise against sequelize in general. Years ago I was a huge proponent of sequelize, I have contributed to this very repo, and I used to be the unofficial go-to guy in Slack for most things feathers-sequelize. I strongly recommend against it as it's bloated, a bit dated, rought with long-standing bugs, difficult to use, and overall has a diametrically opposing set of goals than feathers when it comes to dealing with data IMO. |
|
Ryan - do you have a recommendation for something other than sequliize?
a year ago i thought that MariaDB was taking over sequelize, or at least
that was mentioned on one of their presentations.
Thank you,
Mark Edwards
…On Wed, Oct 20, 2021 at 5:55 PM Ryan Wheale ***@***.***> wrote:
I second @skinofstars <https://github.com/skinofstars> recommendation. If
you're going to use sequelize, use it just as a query generator/executor.
Don't bother with its hooks, eager data loading, or anything else - always
favor using feathers hooks for all things business related... which
includes data manipulation and validation, relational data loading ("eager"
loading), caching, etc.
Furthermore - this is a bit out of scope... but I even advise against
sequelize in general. Years ago I was a huge proponent of sequelize, I have
contributed to this very repo, and I used to be the unofficial go-to guy in
Slack for most things feathers-sequelize. I strongly recommend against it
as it's bloated, a bit dated, rought with long-standing bugs, difficult to
use, and overall has a diametrically opposing set of goals than feathers
when it comes to dealing with data IMO.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#155 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAWJ3YTBKCIEGIHD7FWPJRDUH5QHDANCNFSM4DXVZ7HA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
@DesignByOnyx Thanks for the tips. I do find that Sequelize can be quite a pain, especially when you move away from Vanilla queries and I am always having a hard time finding the info I want in their doc. However I must admit I really how Sequelize integrates directly within Feathers, offering great querying functionality (pagination, filtering, sorting).
To me MariaDb is more of a DBMS than an ORM, so they are not really comparable. |
|
@chrisbag When I was hitting this problem, I was also using Postgres Like others, I've stopped using Sequelize. Too much bloat, too many issues. I now use Knex, it includes all the same Feathers support you're looking for, but in a lighter and more stable package. |
^^ This can be said with almost any adapter or tool integrated with Feathers. Favor feathers hooks over the built in methods/hooks. The only thing I disagree with here is the "relational data loading ("eager" loading)" when querying/sorting. You should use something like https://github.com/feathersjs-ecosystem/batch-loader for joining data onto the result. But, when you need to query/sort across tables, you should probably use I generally recommend a sequelize include that looks like this params.sequelize = {
include: [
model: MyModel,
// empty array tells Sequelize not to actually "join" the result,
// but still makes it available for querying/sorting.
attributes: []
]
}You can also checkout https://daddywarbucks.github.io/feathers-fletching/hooks.html#sequelizejoinquery to help with join query/sort. I don't mean to plug my own libraries, but that is the hook I use to do what we are describing, which is to use Sequelize as little as possible. @chrisbag I don't want to speak for Ryan (and he probably has a more eloquent answer), but I can give my opinion on what he means about Sequelize and Feathers having opposing goals. Sequelize favors models where Feathers favors services. I think it is best described in an example: // This is how you would fetch an author for a book in Sequelize. Sequelize stuffs all of its methods, hooks,
// helpers into its models. It's almost a monolithic structure.
// Uh oh... does this method join on the author's PW?
book.getAuthor();
// In Feathers, we use the service for everything. The service replaces the model and the service
// interface is the only thing we should use to access the data. Doing so ensures all of our actions
// run through our app logic, such as protecting the author's PW
const author = await app.service('authors').get(book.author_id)The goal of Sequelize is to be a robust ORM where all the config, logic, relationships, etc live in the ORM. Feathers breaks all of that out into services. |
|
I think Beau (DaddyWarbucks) captured my sentiments correctly. I should have probably specified "ORMs" in general as having an opposing agenda with feathers. It just happens that sequelize is the most "ORM-y" integration with feathers. ORMs provide a whole lot of complex functionality and API for managing data, but "the feathers way" would favor simple small bite-sized hooks for achieving the same functionality. When you drink the service-based koolaid for long enough (ala feathers), you start to really resent the difficulty and bloat of ORMs. The trap you fall into is when you try to use the ORM for all of it's "goodness" - I mean it's there, you should use it, right? You spend hours and hours trying to bend the ORM to your will. Then you finally get everything working correctly by turning the |
|
@DaddyWarbucks @DesignByOnyx I use sequelize.include a lot to attach associations for the find method with the downside that obviously the feathers hooks of the association are not executed. Imagine you have a model which is related to 3 other models and you want to attach associations. If you use the feathers way of attach data through a hook, it will generate 3 independant queries which does not seem optimal. The same would probably apply to attach association on a Find query. I checked out feathers-fletching doc, there are a lot of interesting use cases which I have been confronted to.
Clearly something I have experienced :) |
|
Not sure how those guys do it, but my approach is generally to map a service to a table. Then I'd have services that compose actions using these data services. If I do have to start doing some light associations, then I'd use a dedicated service that more often than not uses a raw query (I'm mostly just using Knex for sanitisation at this point). Once the queries start getting gnarly, or I want pagination, caching, etc, I move them to database views... and we're back to clean services. We as coders do too much data munging in code, it is often be more efficient and cleaner to have that handled by the database before it ever gets to our tangled web of JS. |
That's what
I used to do this (and it's still a good thing to do), but I have found that writing complex views was a separate nightmare, more maintenance, and not very accessible to developers who weren't strong in SQL. Furthermore, it's only possible with SQL databases In today's world I find myself combining data from multiple stores across multiple services/data-centers/et al - which is only possible using hooks. In summary, it is my experience that "do it in a hook" was always the better decision, and I wasted a lot of time of my life figuring that out. Hopefully I can save others from wasting theirs. |
|
Using batchloader is very performant. Checkout the guide for some more info about how batching/caching works: https://github.com/feathersjs-ecosystem/batch-loader/blob/master/docs/guide.md With the new batchloader v2 coming out, its even easier and more feathers-like to use. And paired with Node's Heres a vid of how I am using batchloader as of late: https://www.loom.com/share/bb5103effd654d18afb3aff86519965b |
|
You can also checkout this little demo app: https://test-feathers-client-joins.herokuapp.com/ |
|
Following on from this, I spent and afternoon writing up a service to use |
|
There are others here that can speak on the performance for larger datasets better than I can. I have had great performance for datasets that large, but I have never directly compared it to the equivalent Sequalize joins. But, as for the "pages of code", I can lend some advice. Give // Package.json
"@feathers-plus/batch-loader": "https://github.com/feathersjs-ecosystem/batch-loader.git#v2",// app.hooks.js
const { AppLoader } = require('@feathers-plus/batch-loader');
const initLoader = (context) => {
if (Object.prototype.hasOwnProperty.call(context.params, 'loader')) {
return context;
}
const loader = new AppLoader({ app: context.app });
context.params.loader = loader;
return context;
};
module.exports = {
before: {
all: [initLoader]
}
}//some resolvers/hooks file
const resolvers = {
user: (post, context) => {
// The appLoader is always available to you. No need to setup a
// usersLoader here. appLoader lazily creates/memoizes loaders as
// you call them
// Also be sure to pass the loader along to get the best performance
return context.params.loader.service('users').load({ id: post.user_id }, { loader: context.params.loader })
}
}You can also checkout my video above using feathers v5 and You may also want to checkout https://daddywarbucks.github.io/feathers-fletching/hooks.html#withresult, which is basically just a less verbose |
This refers to Sequelize hooks. Not Feathers hooks.
Doing a PATCH request doesn't trigger Sequelize inbuilt hooks, even when the service.options.raw is false.
The problem seems to be because we don't instance.update, rather we apply a Model.update
https://github.com/feathersjs-ecosystem/feathers-sequelize/blob/v3.1.0/lib/index.js#L219
This means the Sequelize hooks triggered are the bulk actions (afterBulkCreate/afterBulkUpdate) rather than the expected instance actions (afterCreate/afterUpdate).
The text was updated successfully, but these errors were encountered: