1602 / jugglingdb

Multi-database ORM for nodejs: redis, mongodb, mysql, sqlite3, postgresql, arango, in-memory...
http://1602.github.io/jugglingdb/
2.05k stars 241 forks source link

Select specific cols(issue #143) #384

Closed jvskelton closed 10 years ago

jvskelton commented 10 years ago
jvskelton commented 10 years ago

*correction jvskelton/mysql-adapter

1602 commented 10 years ago

I would suggest to implement it in a different way: in mysql adapter, adapter.all method you can check for filter.attributes and replace * in query with the list of attributes when filter.attributes is present, no reason for adding another core method which will work only for limited set of adapters, especially in this particular case when it can be implemented by extending existing feature.

jvskelton commented 10 years ago

My only concern with that however, would be the fact that currently, I return either an array of data(if one attributed is given) or an array of object literals(if more the one attribute is given), which means the return type would be different. I felt that returning a model instance with potentially several undefined values, along with the model specific functions attached was a bit redundant(not to mention could cause problems if that instance was saved). What are your thoughts on that?

anatoliychakkaev commented 10 years ago

I think that feature makes sense. But this is nothing just pure jugglingdb feature, it should call adapter.all method and return plain objects / strings. No changes in adapters required at all. So, to conclude, there are two features: 1) in mysql adater: hook up 'attributes' property 2) in jugglingdb core: new method which always returns plain objects or just value depending on what you want. this method should work with 'attributes' feature of adapter (when adapter supports it), or just do it manually, as _.pick works in undercore.js

On 28 February 2014 16:29, jvskelton notifications@github.com wrote:

My only concern with that however, would be the fact that currently, I return either an array of data(if one attributed is given) or an array of object literals(if more the one attribute is given), which means the return type would be different. I felt that returning a model instance with potentially several undefined values, along with the model specific functions attached was a bit redundant(not to mention could cause problems if that instance was saved). What are your thoughts on that?

Reply to this email directly or view it on GitHubhttps://github.com/1602/jugglingdb/pull/384#issuecomment-36368033 .

jvskelton commented 10 years ago

@anatoliychakkaev This is a bit delayed, but just to clarify, you do or do not want a new method in the jugglinddb core(which is currently how it works). I believe the last comment conflicts with your first comment. Thanks.

1602 commented 10 years ago
  1. new method in jugglingdb which will call .all with {attributes:[]} property and also ensure that returned dataset only includes required attributes (removing all unexpected attributes).
  2. in adapter: hook up attributes property in adapter.all method

my comments are not in conflict. first comments meaning is that we don't need additional method - all you need is just add support of 'attributes' clause in adapter.all method. second comment suggests to add core method for convenience and cross-adapters compatibility, so that this feature will work in the same way in all adapters not only adapters with 'attributes' clause support in adapter.all method. but you have to filter object manually in case if it contains unexpected attributes. hope that makes sense.

jvskelton commented 10 years ago

Ah, ok that makes sense, thank you for clarifying.