search

balderdashy / waterline-sequel

A SQL generator for use in Waterline Adapters
MIT License
16 stars 61 forks source link

"SELEC FROM" syntax error #58

Open tjwebb opened 9 years ago

tjwebb commented 9 years ago

https://github.com/balderdashy/waterline-sequel/blob/daa4646d3e28fed11f835adb4e7675a63a08cde7/sequel/select.js#L108

This line can cause invalid SQL to be generated. There are other reports of this happening 'round the internets, but I'm tracing it to the line above.

particlebanana commented 9 years ago

Looks like it could happen when there are no attributes defined.

This line https://github.com/balderdashy/waterline-sequel/blob/daa4646d3e28fed11f835adb4e7675a63a08cde7/sequel/select.js#L73 builds up an array of keys to select then loops through and appends to the query: https://github.com/balderdashy/waterline-sequel/blob/daa4646d3e28fed11f835adb4e7675a63a08cde7/sequel/select.js#L95-L105

So if it's empty it would remove extra characters. Not that you would get anything back from an empty query anyway but you wouldn't get the error message.

acis commented 8 years ago

I got this error as well. It happens when selecting n*m attributes. A workaround for this is to add another simple type field to the select.

Owner.find({select: ['pets', 'addresses']}) throws the error, but Owner.find({select: ['id', 'pets', 'addresses']}) works fine.

devinivy commented 8 years ago

Ah, that makes sense. Seems like we could fix that and give a proper error. But what does it mean to select a field that represents a collection? You'll want to use populate() instead.

acis commented 8 years ago

I just don't need the other fields :)

sailsbot commented 8 years ago

Thanks for posting, @tjwebb. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

Thanks so much for your help!

particlebanana commented 8 years ago

@tjwebb no longer an issue?

devinivy commented 8 years ago

@particlebanana this is still an issue. It happens when you select no physical attributes– such as only selecting attributes that represent collections.

error: syntax error at or near "SELEC"

dash- commented 8 years ago

This issue is caused by a less-than-ideal approach to joining groups of phrases. It's an attempt to remove a trailing comma, I believe. Instead, we should just build an array of things to be joined in commas, and use the Array.prototype.join method to join them instead of building a string and then attempting to remove the comma.

mikermcneil commented 8 years ago

@devinivy @dash- I believe this is resolved in https://github.com/particlebanana/waterline-query-builder

We'll be using that in the next version of sails-mysql and sails-postgresql (cc @particlebanana)

dash- commented 8 years ago

Cool.

I wish I'd have realized that prior to making waterline-sequel-derby based on waterline-sequel, and prior to making sails-derby based on the former. If waterline-query-builder is more flexible / configurable than its predecessor, I may just be able to move sails-derby over to waterline-query-builder.

The key issue I had with waterline-sequel is that Apache Derby uses single quotes for literals, and the literal quote character wasn't configurable. But I'll check out waterline-query-builder and see if I can maybe just drop waterline-sequel-derby altogether. On Mar 7, 2016 9:25 PM, "Mike McNeil" notifications@github.com wrote:

@devinivy https://github.com/devinivy @dash- https://github.com/dash- I believe this is resolved in https://github.com/particlebanana/waterline-query-builder

We'll be using that in the next version of sails-mysql and sails-postgresql (cc @particlebanana https://github.com/particlebanana)

— Reply to this email directly or view it on GitHub https://github.com/balderdashy/waterline-sequel/issues/58#issuecomment-193615563 .

particlebanana commented 8 years ago

@dash- The new library uses Knex so it's only available for databases supported by that.

mikermcneil commented 8 years ago

@dash- Great work on sails-derby! Sorry for the confusion. Like @particlebanana mentioned, the new SQL builder is only for databases supported by knex; and we're only planning on switching over sails-mysql and sails-postgresql in the immediate term. We'll also still be building some queries (e.g. DDL) without the help of the new sql builder, at least for now.

davemackintosh commented 8 years ago

Is there a fix or workaround for this, it's affecting a few projects I'm working on as well as Multicolour :/

[EDIT] Sounds weird but I fixed the issue by renaming my collection from category to type (in dev, all my models have migrate: "alter".) I tried with a fresh database and it starts first time but won't start again.

Here's the output from npm list --depth=1

rota-api@1.0.0 /www/node/rota/api
├── multicolour@0.3.5
├── multicolour-auth-oauth@1.1.11
├── multicolour-hapi-vantage@1.0.0 (git+https://github.com/Multicolour/multicolour-hapi-vantage.git#2913d46bef974ba05b239f974ae6e5b0fa5ae559)
├── multicolour-server-hapi@1.3.2
├── sails-postgresql@0.11.0
└── waterline@0.11.2

[EDIT 2]

Well.. For some reason, npm installed 0.11.0. Installing sails-postgresql@0.12.1 fixes the issue.

I can only assume it's due to the weird versioning pattern of the module (and several other Waterline based modules) Here's a snippet of npm info sails-postgresql

image