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

Error on empty result from table definition queries #1686

Merged
merged 4 commits into from May 11, 2014

Conversation

3 participants
@joshperry
Contributor

joshperry commented Apr 26, 2014

See the comments. Basically any query generator that relies on information_schema will just return an empty result if the table and/or schema are spelled incorrectly, or with the wrong case. Sequelize would just error out with a confusing error further up the stack when a function like renameColumn attempted to enumerate an empty table definition.

It would be nice to backport this to 1.7 also.

@@ -328,7 +328,16 @@ module.exports = (function() {
sql = 'DESCRIBE ' + table + ';'
}
return this.sequelize.query(sql, null, { raw: true })
this.sequelize.query(sql, null, { raw: true }).then(function(data) {

This comment has been minimized.

@mickhansen

mickhansen Apr 26, 2014

Contributor

Still need the return afaik

// If no data is returned from the query, then the table name may be wrong.
// Query generators that use information_schema for retrieving table info will just return an empty result set,
// it will not throw an error like built-ins do (e.g. DESCRIBE on MySql).
if(Utils._.isEmpty(data)) {

This comment has been minimized.

@mickhansen

mickhansen Apr 26, 2014

Contributor

Could perhaps just be a throw if empty else return data?

@joshperry

This comment has been minimized.

Contributor

joshperry commented Apr 26, 2014

Good catch, crap. Fixed.

@joshperry

This comment has been minimized.

Contributor

joshperry commented Apr 26, 2014

Could throw I guess, just wanted it to act like the original query which simply rejects the promise with the error message from the server when something goes wrong, for example if you provide an invalid table name to the MySql DESCRIBE.

@joshperry

This comment has been minimized.

Contributor

joshperry commented Apr 26, 2014

Looks like this has exposed some bugs with schema handling in sqlite. Unfortunately, I'm not familiar enough with sqlite to be able to diagnose this.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 27, 2014

Throwing inside a promise is the same as rejecting it basically, it was just to reduce amount of code.

@janmeier

This comment has been minimized.

Member

janmeier commented May 6, 2014

@joshperry The error seems to be in the describeTableQuery fn of the sqlite query generator. When using tables with schema, it generates

PRAGMA TABLE_INFO('`schema.table`')

The table is both quoted and wrapped as a string.

 describeTableQuery: function(tableName, schema, schemaDelimiter) {
      if (schema) {
        schemaDelimiter = schemaDelimiter || '.'
        tableName = schema + schemaDelimiter + tableName
      }

      var sql = "PRAGMA TABLE_INFO('<%= tableName %>');"
      return Utils._.template(sql, { tableName: tableName})
    },

Changing the function to the above should fix it :)

@joshperry

This comment has been minimized.

Contributor

joshperry commented May 6, 2014

@janmeier Thank you for the heads-up. It looks like the parameters to the TABLE_INFO and other PRAGMA functions in sqlite accept an identifier name directly. So I just removed the single quotes from the query and let addSchema still do its thing to properly concatenate and quote the schema and table identifers.

@janmeier

This comment has been minimized.

Member

janmeier commented May 7, 2014

Cool, whatever works. Could you add a couple of tests while you're at it
:-)?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented May 11, 2014

@joshperry @janmeier status on this?

@janmeier

This comment has been minimized.

Member

janmeier commented May 11, 2014

Code looks good, but hoping for @joshperry to add a couple of tests before we merge :)

@janmeier janmeier self-assigned this May 11, 2014

@joshperry

This comment has been minimized.

Contributor

joshperry commented May 11, 2014

What kind of tests would you like to see? If I have to be honest, the sqlite codebase looks pretty rough -- especially the schema support -- and, like I mentioned, I don't really have the time or means to work on this dialect. I fixed this bug because fixing the "error on no table definition" exposed the fact that it existed and I'd like to have this feature integrated with the existing tests passing.

The test suite is in bad need of a refactor, it is difficult to follow and to upkeep because of the circuitous flow and disjoint hierarchy caused by logical blocks depending on the name of the dialect throughout.

I'm using the Postgres dialect, and I would guess that as my project progresses that I will have more PRs come up there.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented May 11, 2014

A test covering what you changed. I.e. a proper error when the table doesn't exist.

I don't see how the test suite is in bad need of a refactor, but people are of course entitled to their opinions :)

We welcome most PRs, however the intent is for Sequelize to support all dialects equally (although with extra support for stuff like json for PG).

mickhansen added a commit that referenced this pull request May 11, 2014

Merge pull request #1686 from joshperry/pg-describe-notable-error
Error on empty result from table definition queries

@mickhansen mickhansen merged commit 56bbf92 into sequelize:master May 11, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment