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

fixed UniqueViolationError bug for mysql clients #141

Merged
merged 4 commits into from
Mar 2, 2021
Merged

fixed UniqueViolationError bug for mysql clients #141

merged 4 commits into from
Mar 2, 2021

Conversation

Tr3ffel
Copy link
Contributor

@Tr3ffel Tr3ffel commented Feb 22, 2021

Hi,

currently, I develop an app where users have an email which should be unique in the users table. So I added a unique index for the email column.

But when tried to save a user with an email which is already used, feathers-objection throws the following error: Cannot read property 'join' of undefined

After some research I found out that the feathers-objection error handler tries to generate an error message with the following code:

feathersError = new errors.Conflict(`${error.columns.join(', ')} must be unique`, {
  columns: error.columns,
  table: error.table,
  constraint: error.constraint
});

After some more research I found out that objection uses the db-error package. And as documented the UniqueViolationError does not support the columns field for mysql.

This probably causes error.columns to be undefined and error.columns.join(', ') will not work.

I fixed the bug by checking if the error.client is mysql and if so the native sqlMessage will be thrown. I also added a test for this.

I hope it helps.

@dekelev
Copy link
Member

dekelev commented Feb 22, 2021 via email

@dekelev
Copy link
Member

dekelev commented Mar 1, 2021

@Tr3ffel Which version of objection NPM module do you use in your project?

According to Objection's docs, the current implementation should work regardless of the DB type. I haven't tested this myself on MySQL and the current tests definitely not suffice and do not protect from this type of errors (ObjectionJS throws unexpected error object). We must test this against real MySQL using the latest objection NPM module version before making any changes.

@Tr3ffel
Copy link
Contributor Author

Tr3ffel commented Mar 1, 2021

Hi,

currently, I use version 2.2.14.

I'm not sure want you mean by: ObjectionJS throws unexpected error object. I think this problem has nothing to do with Objection.js. Getting the table and columns field of an Unqiue Index Error just not seems to be possible for MySQL.

Also, I found a Pull-Request where this problem was already described.

@dekelev
Copy link
Member

dekelev commented Mar 2, 2021

I see. So in ObjectionJS opinion, there's "no way to reliably get table and columns from mysql error".
It makes sense, since MySQL doesn't return a unified error message with the table & columns like PostgreSQL & SQLite do and ObjectionJS seems to generally decided not to rely on the DB query itself for reference to the table & columns.

@dekelev dekelev merged commit 359d0e0 into feathersjs-ecosystem:master Mar 2, 2021
@dekelev
Copy link
Member

dekelev commented Mar 2, 2021

Released with v7.1.2

estephan added a commit to dinode/feathers-objection that referenced this pull request Mar 29, 2022
* Added tests on relation sorting

* Fix tests clean-up

* - Fixed package-lock.json
- Fixed tests
- Bumped version to 5.1.1

* Updated CHANGELOG file

* Updated peer-dependency `objection` to v2 and up

* - Added $modify query operator
- Bumped version to 5.2.0

* Added $modify query operator

* Updated CHANGELOG file

* Removed unused dev dependency

* - Fixed sorting tests to work in Postgres
- Added missing cleanup code

* Added support for `ref` in $select & $sort

* Added support for `ref` in $select & $sort

* 5.3.0

* Updated CHANGELOG file

* Removed unneeded logic in get query after creating a row

* Reverted: Removed unneeded logic in get query after creating a row

* Changed create to insert & get by returned row fields by default (feathersjs-ecosystem#90)

* Changed create to insert & get by returned row fields by default

* Added test on create with ID when ID gets overridden in model

Co-authored-by: Dekel Barzilay <dekel@lusha.co>

* 5.3.1

* 5.3.2

* Updated CHANGELOG file

* Update README.md (feathersjs-ecosystem#91)

Fixed Modal -> Model typos

* Updated project dependencies

* 5.3.3

* Updated CHANGELOG file

* Patch with same field in data & query now returns success result

* 5.4.0

* Updated CHANGELOG file

* Add types composite id and ERROR type (feathersjs-ecosystem#95)

* 5.4.1

* Updating changelog

* Added $allowRefs query operator to allow referencing other fields

* 5.5.0

* Updated CHANGELOG file

* Fixed count for queries with $modify query operator

* 5.5.1

* Updated CHANGELOG file

* Added a check on result when using $modify with pagination

* 5.5.2

* Updated CHANGELOG file

* Set schema option (feathersjs-ecosystem#105)

Added the `schema` service option and param operator

* Added the new `schema` service option & params operator to the docs

* Added the new `schema` service option & params operator to the docs

* 5.6.0

* Updated CHANGELOG file

* Updated docs with $modifyEager JSON limitation

* Bump lodash from 4.17.15 to 4.17.19 (feathersjs-ecosystem#107)

Bumps [lodash](https://github.com/lodash/lodash) from 4.17.15 to 4.17.19.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.15...4.17.19)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Added support for GROUP BY with ONLY_FULL_GROUP_BY enabled

* Added support for GROUP BY with ONLY_FULL_GROUP_BY enabled

* Added support for GROUP BY with ONLY_FULL_GROUP_BY enabled

* Added support for GROUP BY with ONLY_FULL_GROUP_BY enabled

* 5.7.0

* Added support for GROUP BY with ONLY_FULL_GROUP_BY enabled

* Fix for default operators getting overwritten by options.whitelist instead of concat (feathersjs-ecosystem#108)

* Fix for options.whitelist getting overwritten instead of concat

* Fixed issue with the whitelist option

Co-authored-by: Dekel Barzilay <dekelev@gmail.com>

* Updated project dependencies

* Added missing typescript dev module

* Changed minimum supported NodeJS version to 10

* 5.8.0

* Updated CHANGELOG file

* Fixed issue where query data would not properly copy if variable equates to false (feathersjs-ecosystem#115)

* 5.8.1

* Updated CHANGELOG file

* Added Redshift to the list of supported databases in the README file

* Added major supported databases to the package.json keywords list

* joinEager Fixes + $not Operator all with tests (feathersjs-ecosystem#114)

* Fix $joinEager

Allow $mergeEager query
* add test
* fix bug

Allow paginated query
* add test
* fix bug

* Add $not operator

* objectify modified
* include $not in default operators
* add test

* Lint

* Fix $select and $noSelect with mutating methods

* were not honored with graph
* $select prevented update to update a whole object
(only selected parts were updated)
* a consequence of this commit is that
$eager is not need anymore to upsert data (partial fix feathersjs-ecosystem#97)
* $noSelect always return input data instead of an empty object

* Fix feathersjs-ecosystem#97 update with user query and upsert

* Enforce user query in patch with upsert

* Ensure user data and query are not overwritten

* in upsert graph, id is added if needed but not modified
and if data[id] and patch/update id mismatch it throw

* in get request, id is added to query so that the query must match
id and whole user query

* Improve $select after upsert

* now support '*' and 'table.*' in $select

* Updated README file

* new features
* migrating

* Update README.md

Co-authored-by: Dekel Barzilay <dekelev@gmail.com>

* 6.0.0

* Updated CHANGELOG file

* $modify now supports passing different args to each modifier

* 6.1.0

* Updated CHANGELOG file

* Updated project dependencies

* 6.1.1

* Updated CHANGELOG file

* Atomic graph mutation (feathersjs-ecosystem#124)

* Fix schema use with graph

* Add $startTransaction params

* allow for atomic graph upsert/insert
* allow for atomic multi create

* Fix $startTransaction (atomic)

* Is now $atomic
* Better testing
* README infos

Co-authored-by: Dekel Barzilay <dekelev@gmail.com>

* 6.2.0

* Changed params operator $atomic to atomic

* 6.2.1

* Updated CHANGELOG file

* Batch insert support (feathersjs-ecosystem#127)

* Batch insert support. Closes feathersjs-ecosystem#125

* don't use .returning() with redshift dialect

* add a test case: create with $noSelect and without allowedInsert

* fix: Objection.js hooks don't work with '.toKnexQuery()' method

* 6.3.0

* Updated CHANGELOG file

* feat: use objection error types in error-handler (feathersjs-ecosystem#131)

* new error handler

* feat: use objection errors in error-handler

* cleanup commented out lines

* revert package lock

* add better error messages

* update readme

* fix: NotNullViolationError message

* update tests that rely on original error message

* Fixed typo in test description

Co-authored-by: Dekel Barzilay <dekelev@gmail.com>

* - Updated project dependencies
- Applied semistandard linter fixes

* 7.0.0

* Updated CHANGELOG file

* Bump ini from 1.3.5 to 1.3.7 (feathersjs-ecosystem#137)

Bumps [ini](https://github.com/isaacs/ini) from 1.3.5 to 1.3.7.
- [Release notes](https://github.com/isaacs/ini/releases)
- [Commits](npm/ini@v1.3.5...v1.3.7)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add optional parameter modifierFiltersResults for disabling count of $modify query results (feathersjs-ecosystem#136)

* Add optional parameter modifierFiltersResults for disabling count of $modify query results

* Improve test case for modifierFiltersResults parameter

* Updated project dependencies

* 7.1.0

* Updated CHANGELOG file

* Fixed issue where using $select with a composite PK returns a database error (feathersjs-ecosystem#140)

Co-authored-by: David Farrugia <david.farrugia@sagossgroup.com>

* - Added missing common tests
- Updated dev dependencies

* 7.1.1

* Updated CHANGELOG file

* fixed UniqueViolationError bug for mysql clients (feathersjs-ecosystem#141)

* fixed UniqueViolationError bug when using mysql

* fixed a typo and added a test

* Update index.test.js

* Update error-handler.js

Co-authored-by: Dekel Barzilay <dekelev@gmail.com>

* Updated dev dependencies

* 7.1.2

* Updated CHANGELOG file

* Bump y18n from 4.0.0 to 4.0.1 (feathersjs-ecosystem#145)

Bumps [y18n](https://github.com/yargs/y18n) from 4.0.0 to 4.0.1.
- [Release notes](https://github.com/yargs/y18n/releases)
- [Changelog](https://github.com/yargs/y18n/blob/master/CHANGELOG.md)
- [Commits](https://github.com/yargs/y18n/commits)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Updated dev dependencies

* 7.1.3

* Updated CHANGELOG file

* make params optional (feathersjs-ecosystem#146)

* 7.1.4

* Updated CHANGELOG.md file

* Bump lodash from 4.17.19 to 4.17.21 (feathersjs-ecosystem#148)

Bumps [lodash](https://github.com/lodash/lodash) from 4.17.19 to 4.17.21.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.19...4.17.21)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Updated dev dependencies

* 7.1.5

* 7.1.6

* Updated CHANGELOG.md file

* Handle missing top-level jsonSchema.properties in objectify() (feathersjs-ecosystem#149)

* 7.1.7

* Updated CHANGELOG.md file

* Remove groupBy when counting rows for pagination (feathersjs-ecosystem#150)

* 7.2.0

* Updated CHANGELOG.md file

* Fix patch method with complex query (feathersjs-ecosystem#151)

* Fix schema use with graph

* Add $startTransaction params

* allow for atomic graph upsert/insert
* allow for atomic multi create

* Fix $startTransaction (atomic)

* Is now $atomic
* Better testing
* README infos

* Fix confilts with upstream

* Fix/Add comments

* Fix patch query with complex query

* Fixed typo

Co-authored-by: Dekel Barzilay <dekelev@gmail.com>

* 7.3.0

* Updated CHANGELOG.md file

* Reverted PR feathersjs-ecosystem#150

* 7.4.0

* Updated CHANGELOG.md file

* Add upsert and insert graph options in params (feathersjs-ecosystem#153)

* Fix schema use with graph

* Add $startTransaction params

* allow for atomic graph upsert/insert
* allow for atomic multi create

* Fix $startTransaction (atomic)

* Is now $atomic
* Better testing
* README infos

* Fix confilts with upstream

* Fix/Add comments

* Fix patch query with complex query

* Fixed typo

* Add upsert and insert graph options in params

* allow for query based insert and upsert graph options
typical use case is different options based on authenticated user rights

* Fix param names in previous commit

Co-authored-by: Dekel Barzilay <dekelev@gmail.com>

* 7.5.0

* Updated CHANGELOG.md file

* - Updated dev dependencies
- Added relation to the example app

* 7.5.1

* Updated CHANGELOG.md file

* Update grammatical error in README (feathersjs-ecosystem#156)

* Fix type in README.MD (feathersjs-ecosystem#164)

Change `automaticaly` to `automatically`.

* Removed usage of Objection's skipUndefined deprecated method

* Upgrade project dependencies

* Updated minimum target version of NodeJS to 14

* 7.5.2

* Updated CHANGELOG.md file

* Bump ajv from 6.12.0 to 6.12.6 (feathersjs-ecosystem#170)

Bumps [ajv](https://github.com/ajv-validator/ajv) from 6.12.0 to 6.12.6.
- [Release notes](https://github.com/ajv-validator/ajv/releases)
- [Commits](ajv-validator/ajv@v6.12.0...v6.12.6)

---
updated-dependencies:
- dependency-name: ajv
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Upgrade dev dependencies

* 7.5.3

* Upgrade dev dependencies

Co-authored-by: Dekel Barzilay <dekel@lusha.co>
Co-authored-by: Dekel Barzilay <dekelev@gmail.com>
Co-authored-by: Konstantin <outluch@gmail.com>
Co-authored-by: David Luecke <daff@neyeon.com>
Co-authored-by: EmileSpecs <emile.sievers@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Satyadeep <satyadeep.1991@gmail.com>
Co-authored-by: Wes Winder <weswinder@users.noreply.github.com>
Co-authored-by: Thomas-git <watom@ritou.me>
Co-authored-by: Dmitry Merkulov <69001428+mdmitry01@users.noreply.github.com>
Co-authored-by: Rob Phillips <robbyphillips@gmail.com>
Co-authored-by: Alexander Friedl <52403795+alex-all3dp@users.noreply.github.com>
Co-authored-by: davidf84 <78353459+davidf84@users.noreply.github.com>
Co-authored-by: David Farrugia <david.farrugia@sagossgroup.com>
Co-authored-by: Marco Ahlers <61047871+Tr3ffel@users.noreply.github.com>
Co-authored-by: fratzinger <22286818+fratzinger@users.noreply.github.com>
Co-authored-by: Guillaume Mercey <guillaume.mercey@gmail.com>
Co-authored-by: Eric Irish <ecirish@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants