Skip to content

Remove ModelWithScope and add handleError #450

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.

Sign up for GitHub

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

Merged
fratzinger merged 6 commits into from
Mar 12, 2025
Merged

Remove ModelWithScope and add handleError #450

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cfc0fb4
Remove ModelWithScope
DaddyWarbucks Feb 20, 2025
cd5dbb8
Use handleError method
DaddyWarbucks Feb 20, 2025
f8f291a
Fix spacing
DaddyWarbucks Feb 20, 2025
408a47e
Add semicolon
DaddyWarbucks Feb 20, 2025
f167ea3
Better error handling
DaddyWarbucks Feb 20, 2025
e473e9f
Update README
DaddyWarbucks Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ A [Feathers](https://feathersjs.com) database adapter for [Sequelize](http://seq
- [`service(options)`](#serviceoptions)
- [params.sequelize](#paramssequelize)
- [operators](#operatormap)
- [Modifying the model](#modifyModel)
- [Caveats](#caveats)
- [Sequelize `raw` queries](#sequelize-raw-queries)
- [Working with MSSQL](#working-with-mssql)
Expand Down Expand Up @@ -154,6 +155,27 @@ app.service('users').find({
GET /users?name[$like]=Dav%
```

## Modifying the Model

Sequelize allows you to call methods like `Model.scope()`, `Model.schema()`, and others. To use these methods, extend the class to overwrite the `getModel` method.

```js
const { SequelizeService } = require('feathers-sequelize');

class Service extends SequelizeService {
getModel(params) {
let Model = this.options.Model;
if (params?.sequelize?.scope) {
Model = Model.scope(params?.sequelize?.scope);
Copy link
Contributor

@fratzinger fratzinger Feb 20, 2025

Choose a reason for hiding this comment

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

You could remove the optional chaining (?.). I think, if you use the chains, ts complains that you can't pass undefined to Model.scope() even though it should know that the optional chaining is useless.

It's just docs and everyone should be capable to fix this for themselves, but yeah.

}
if (params?.sequelize?.schema) {
Model = Model.schema(params?.sequelize?.schema);
Copy link
Contributor

@fratzinger fratzinger Feb 20, 2025

Choose a reason for hiding this comment

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

same

}
return Model;
}
}
```

## Caveats

### Sequelize `raw` queries
Expand Down
43 changes: 20 additions & 23 deletions src/adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,6 @@ export class SequelizeAdapter<
return this.options.Model;
}

ModelWithScope (params: ServiceParams) {
const Model = this.getModel(params);
if (params?.sequelize?.scope) {
return Model.scope(params.sequelize.scope);
}
return Model;
}

convertOperators (q: any): Query {
if (Array.isArray(q)) {
Expand Down Expand Up @@ -222,23 +215,27 @@ export class SequelizeAdapter<
return sequelize;
}

handleError (error: any) {
return errorHandler(error);
}
Copy link
Contributor

@fratzinger fratzinger Feb 20, 2025

Choose a reason for hiding this comment

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

It's really really important for handleError or errorHandler to throw an error. If you overwrite handleError with a function that does not throw anything, you swallow the error easily.

We call that function in every .catch() clause. We just should be double sure, that the error cannot be swallowed. So at least we should add a private _handleError function or something that calls the overwritable handleError function and throws the error at the end. If handleError throws an error, our fallback error won't be reached.

In addition the default errorHandler is pretty convenient. With a custom handler it's easy to mess things up. What if errorHandler already receives the wrapped error from errorHandler? Does this fit your use case? Or is a signature handleError(feathersError: any, sequelizeError: any) conceivable?

Copy link
Contributor Author

@DaddyWarbucks DaddyWarbucks Feb 20, 2025

Choose a reason for hiding this comment

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

This is good feedback. I intended to use it like this

handleError(error) {
  if (error.message regex something) {
    error.message = "Safe Error message"
  }
  return super.handleError(error);
}

But you are right. Other users may not handle and re-throw the error and it would be swallowed. I like the idea of wrapping our use in another handler. No need to attach it to the class as _handleError. We will just keep a private function and wrap our own usage of handleError.

Copy link
Contributor Author

@DaddyWarbucks DaddyWarbucks Feb 20, 2025

Choose a reason for hiding this comment

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

@fratzinger This has been updated with your suggestions. The double try/catch thing in catchHandler is a little goofy. So I stared to update errorHandler to be convertError and return an error instead of throw one. But then I realized errorHandler is exported from the package and is probably more useful as is than as a convertError. So even though the try/catch thing is a little goofy, I think its fine.

Copy link
Contributor Author

@DaddyWarbucks DaddyWarbucks Feb 20, 2025

Choose a reason for hiding this comment

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

I also used GeneralError instead of Error in the few other places.


async _find (params?: ServiceParams & { paginate?: PaginationOptions }): Promise<Paginated<Result>>
async _find (params?: ServiceParams & { paginate: false }): Promise<Result[]>
async _find (params?: ServiceParams): Promise<Paginated<Result> | Result[]>
async _find (params: ServiceParams = {} as ServiceParams): Promise<Paginated<Result> | Result[]> {
const Model = this.ModelWithScope(params);
const Model = this.getModel(params);
const { paginate } = this.filterQuery(params);
const sequelizeOptions = this.paramsToAdapter(null, params);

if (!paginate || !paginate.default) {
const result = await Model.findAll(sequelizeOptions).catch(errorHandler);
const result = await Model.findAll(sequelizeOptions).catch(this.handleError);
return result;
}

if (sequelizeOptions.limit === 0) {
const total = (await Model
.count({ ...sequelizeOptions, attributes: undefined })
.catch(errorHandler)) as any as number;
.catch(this.handleError)) as any as number;

return {
total,
Expand All @@ -248,7 +245,7 @@ export class SequelizeAdapter<
}
}

const result = await Model.findAndCountAll(sequelizeOptions).catch(errorHandler);
const result = await Model.findAndCountAll(sequelizeOptions).catch(this.handleError);

return {
total: result.count,
Expand All @@ -259,9 +256,9 @@ export class SequelizeAdapter<
}

async _get (id: Id, params: ServiceParams = {} as ServiceParams): Promise<Result> {
const Model = this.ModelWithScope(params);
const Model = this.getModel(params);
const sequelizeOptions = this.paramsToAdapter(id, params);
const result = await Model.findAll(sequelizeOptions).catch(errorHandler);
const result = await Model.findAll(sequelizeOptions).catch(this.handleError);
if (result.length === 0) {
throw new NotFound(`No record found for id '${id}'`);
}
Expand All @@ -283,13 +280,13 @@ export class SequelizeAdapter<
return []
}

const Model = this.ModelWithScope(params);
const Model = this.getModel(params);
const sequelizeOptions = this.paramsToAdapter(null, params);

if (isArray) {
const instances = await Model
.bulkCreate(data as any[], sequelizeOptions)
.catch(errorHandler);
.catch(this.handleError);

if (sequelizeOptions.returning === false) {
return [];
Expand Down Expand Up @@ -318,7 +315,7 @@ export class SequelizeAdapter<

const result = await Model
.create(data as any, sequelizeOptions as CreateOptions)
.catch(errorHandler);
.catch(this.handleError);

if (sequelizeOptions.raw) {
return select((result as Model).toJSON())
Expand All @@ -334,7 +331,7 @@ export class SequelizeAdapter<
throw new MethodNotAllowed('Can not patch multiple entries')
}

const Model = this.ModelWithScope(params);
const Model = this.getModel(params);
const sequelizeOptions = this.paramsToAdapter(id, params);
const select = selector(params, this.id);
const values = _.omit(data, this.id);
Expand All @@ -361,7 +358,7 @@ export class SequelizeAdapter<
raw: false,
where: { [this.id]: ids.length === 1 ? ids[0] : { [Op.in]: ids } }
} as UpdateOptions)
.catch(errorHandler) as [number, Model[]?];
.catch(this.handleError) as [number, Model[]?];

if (sequelizeOptions.returning === false) {
return []
Expand Down Expand Up @@ -436,7 +433,7 @@ export class SequelizeAdapter<
await instance
.set(values)
.update(values, sequelizeOptions)
.catch(errorHandler);
.catch(this.handleError);

if (isPresent(sequelizeOptions.include)) {
return this._get(id, {
Expand All @@ -462,7 +459,7 @@ export class SequelizeAdapter<
}

async _update (id: Id, data: Data, params: ServiceParams = {} as ServiceParams): Promise<Result> {
const Model = this.ModelWithScope(params);
const Model = this.getModel(params);
const sequelizeOptions = this.paramsToAdapter(id, params);
const select = selector(params, this.id);

Expand All @@ -484,7 +481,7 @@ export class SequelizeAdapter<
await instance
.set(values)
.update(values, sequelizeOptions)
.catch(errorHandler);
.catch(this.handleError);

if (isPresent(sequelizeOptions.include)) {
return this._get(id, {
Expand Down Expand Up @@ -516,7 +513,7 @@ export class SequelizeAdapter<
throw new MethodNotAllowed('Can not remove multiple entries')
}

const Model = this.ModelWithScope(params);
const Model = this.getModel(params);
const sequelizeOptions = this.paramsToAdapter(id, params);

if (id === null) {
Expand All @@ -539,7 +536,7 @@ export class SequelizeAdapter<
await Model.destroy({
...params.sequelize,
where: { [this.id]: ids.length === 1 ? ids[0] : { [Op.in]: ids } }
}).catch(errorHandler);
}).catch(this.handleError);

if (sequelizeOptions.returning === false) {
return [];
Expand Down
28 changes: 1 addition & 27 deletions test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,19 +111,7 @@ const Model = sequelize.define('people', {
defaultValue: 'pending'
}
}, {
freezeTableName: true,
scopes: {
active: {
where: {
status: 'active'
}
},
pending: {
where: {
status: 'pending'
}
}
}
freezeTableName: true
});
const Order = sequelize.define('orders', {
name: {
Expand Down Expand Up @@ -780,20 +768,6 @@ describe('Feathers Sequelize Service', () => {
}
});
});

it('can set the scope of an operation #130', async () => {
const people = app.service('people');
const data = { name: 'Active', status: 'active' };
const SCOPE_TO_ACTIVE = { sequelize: { scope: 'active' } };
const SCOPE_TO_PENDING = { sequelize: { scope: 'pending' } };
await people.create(data);

const staPeople = await people.find(SCOPE_TO_ACTIVE) as Paginated<any>;
assert.strictEqual(staPeople.data.length, 1);

const stpPeople = await people.find(SCOPE_TO_PENDING) as Paginated<any>;
assert.strictEqual(stpPeople.data.length, 0);
});
});

describe('ORM functionality', () => {
Expand Down