search

feathersjs-ecosystem / feathers-sequelize

A Feathers service adapter for the Sequelize ORM. Supporting MySQL, MariaDB, Postgres, SQLite, and SQL Server
MIT License
208 stars 75 forks source link

Run tests against PostgreSQL #105

Closed daffl closed 7 years ago

daffl commented 7 years ago

Since there are dialect specific branches we should run the CI tests against PostgreSQL.

ryanthegiantlion commented 7 years ago

A case in point . . .

Issue #146 is actually covered by this test in the feathers-service-test. However the issue was only occurring with postgres so was missed by the tests.

daffl commented 7 years ago

108 has all tests also run against Postgres. The only problem is some errors that need to be transformed properly.

ryanthegiantlion commented 7 years ago

Aah I see. It doesn't seem straightforward with how best to handle the not 'not found' error.

The common tests are using a non integer id. From the perspective of keeping things persistence agnostic that is perhaps ok. But there is also an argument that the test should be using a natural key with the correct type. And that an invalid-key error is more natural than a does-not-exist error if the incorrect type is used. Or if this is more an issue with feathers-service-tests rather than an issue with feathers sequelize.

ryanthegiantlion commented 7 years ago

Actually I think the best way to handle it is to modify feathers-service-tests. The test should probably inspect the type of key from the model. And use the correct type of key for 'not found' tests. Thoughts @daffl ?

daffl commented 7 years ago

I think a non-integer id should still throw a 404. Oddly it does seem to work with the standard SQLite test setup.

ryanthegiantlion commented 7 years ago

SqlLite seems to handle wrong types differently. For example see:

https://stackoverflow.com/questions/29476818/how-to-avoid-inserting-the-wrong-data-type-in-sqlite-tables

I did some investigation into the sequelize tests for both postgres and sqllite and can confirm that Model.update throws an error when using postgres but not when using sqllite (when using an incorrect id type that is). The reason why the Service throws an error correctly with sqllite is that for non-postgres dialects the service follows the update with a get - which does throw the required error.

Some options to fix this behaviour are (and I am happy to add this btw)

  1. Do a get before doing the Model.update

  2. Do a check on the type of id coming into patch etc queries. If the id is of the wrong type return a notfound error immediately. There is no point even querying the database.

  3. Try and parse the sequelize model.update error and handle the error specificially for when the id is the incorrect type. (As opposed to there being some issues with the data being updated). The problem here is that the error being returned is pretty generic: http://docs.sequelizejs.com/class/lib/errors/index.js~DatabaseError.html And on inspecting the error I saw no reliable way of determining whether the error is caused by an incorrect id type.