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

fields lowercase #106

Closed
GoNode5 opened this issue Apr 9, 2013 · 35 comments
Closed

fields lowercase #106

GoNode5 opened this issue Apr 9, 2013 · 35 comments

Comments

@GoNode5
Copy link

GoNode5 commented Apr 9, 2013

I used the Mysql World-database as example to test Orm. All the examples for fields are lowercase. Testing with find returned an empty array but the count was correct. It took me a long time to find out that the field defs in MySql table where mixed case. The empty array was not really helping... It also could suggest the find was empty? An error that the fieldname was not found would have been more helpfull?

@dresende
Copy link
Owner

dresende commented Apr 9, 2013

I think you can define the model properties in any case you want. I'm not sure if it will look for the column as you defined it. I'm going to check.

@GoNode5
Copy link
Author

GoNode5 commented Apr 9, 2013

var  Country = db.define("country", {
        Code    : { type: "text", size: 3 },
        Name    : { type: "text", size: 52 }

});   

Country.find( {Name:orm.like("n%") }, function (err, countries) {
console.log(countries[0].Name);
// console output: Netherlands Antilles

var  Country = db.define("country", {
        code    : { type: "text", size: 3 },
        name    : { type: "text", size: 52 }

});   

Country.find( {name:orm.like("n%") }, function (err, countries) {
console.log(countries[0].name);
// console output: null

@dresende
Copy link
Owner

dresende commented Apr 9, 2013

I did not express correctly. When I said you can define the properties as you want, I meant as you need. Your example shows that the driver you're using is case sensitive. I never thought about that. Since ORM does not inspect tables before querying, it doesn't know the properties case. What database are you using?

@dresende
Copy link
Owner

dresende commented Apr 9, 2013

Is it postgresql?

@dresende
Copy link
Owner

dresende commented Apr 9, 2013

I think it is. I just found that when escaping property names in queries, postgresql is case sensitive. If I don't escape them, it's insensitive... fuck.

@dresende
Copy link
Owner

dresende commented Apr 9, 2013

I can't do nothing about this, unless I add an option to perform queries without quoting identifiers (table names and column names). This will probably bite me in the future, I would prefer to keep it as it is. You have to define your properties as you have them in database. I don't like having ucfirst() on columns but if you like it, you probably also like it in db.define().

@GoNode5
Copy link
Author

GoNode5 commented Apr 9, 2013

i wrote I used the Mysql ... ucfirst is not needed.... I would prefer rather an error message.

Querie's seems to be case inseniitive. in both cases the sql is the same
(orm/mysql) SELECT * FROM country WHERE name LIKE 'n%'

somehow the resultset is not matched... could you not make this always lowercase

@dresende
Copy link
Owner

dresende commented Apr 9, 2013

I don't seem to have a problem with mysql. Are you sure err is null ? When I tested with postgresql it returned me an error.

@dresende
Copy link
Owner

dresende commented Apr 9, 2013

Please post mysql server version and orm version.

@GoNode5
Copy link
Author

GoNode5 commented Apr 9, 2013

mysql Ver 14.14 Distrib 5.6.10, for Win64 (x86_64)
2.0.8 (did a fresh new install to a new dir / npm install orm

var orm = require("orm");

orm.connect("mysql://_:_@localhost/world", function (err, db) {
if (err) throw err;

var  Country = db.define("country", {
        code    : { type: "text", size: 3 },
        name    : { type: "text", size: 52 }
});

var Country2 = db.define("country", {
Code : { type: "text", size: 3 },
Name : { type: "text", size: 52 }
});

Country.find( {Name:orm.like("n%") }, function (err, countries) {
console.log(countries[0].name);
console.log(countries[0].Name);
});
Country2.find( {Name:orm.like("n%") }, function (err, countries) {
console.log(countries[0].name);
console.log(countries[0].Name);
});

});

node-test

@dxg
Copy link
Collaborator

dxg commented Apr 10, 2013

I can't do nothing about this, unless I add an option to perform queries without quoting identifiers (table names and column names). This will probably bite me in the future, I would prefer to keep it as it is.

It will, when someone decides to have a field called select or any one of these.

@GoNode5: Not really sure what's in your databse, so I did some testing of my own:

var orm = require("orm");

orm.connect("mysql://user:pass@localhost/database", function (err, db) {
  if (err) throw err;

  var Country = db.define("country", {
    code : { type: "text", size: 3  },
    name : { type: "text", size: 52 }
  });
  var Country2 = db.define("country", {
    Code : { type: "text", size: 3  },
    Name : { type: "text", size: 52 }
  });

  var which = Country2;

  if (which == Country)
    var data = [{name: 'nelly'}, {name: 'norbert'}];
  else
    var data = [{Name: 'nelly'}, {Name: 'norbert'}];

  which.drop(function(err) {
    which.sync(function(err) {
      if (err) throw err;

      which.create(data, function(err) {
        if (err) throw err;

        Country.find( { name:orm.like("n%") }, function (err, countries) {
          console.log(countries[0].name, countries[0].Name);

          Country2.find( { Name:orm.like("n%") }, function (err, countries) {
            console.log(countries[0].name, countries[0].Name);
            db.close();
          });
        });
      });
    });
  });
});
which = Country;
> nelly undefined
> nelly undefined

which = Country2;
> null undefined
> null undefined

This is a caching issue; if you disable cache on Country2 it outputs correctly:

var Country2 = db.define("country", {
  Code : { type: "text", size: 3  },
  Name : { type: "text", size: 52 }
}, {cache: false});

null undefined
undefined 'nelly'

There are only two hard things in Computer Science: cache invalidation and naming things.

In this case, the key for the cache is the table name.

@GoNode5
Copy link
Author

GoNode5 commented Apr 10, 2013

@dxg I really dont understand the cache thing I have to study your example:

I installed MySql locally, included was an database called: World, wich include an allready created and filled table Country: CREATE TABLE country (
Code char(3) NOT NULL DEFAULT '',
Name char(52) NOT NULL DEFAULT '',
You can have mixed case fieldname's in Mysql apperantly... I took the ORM example where fieldnames all are lowercase. These leads to the null showed... The sql query apperently don't care about mixed case (SELECT * FROM country WHERE name LIKE 'n%' showed by node-mysql ) so why do I get an null ?

@dxg
Copy link
Collaborator

dxg commented Apr 10, 2013

Ignore the caching thing; it's not the issue you've hit..

I finally understand. I also recreated your table, and got the same problem you did with mysql.

ORM is case sensitive. Some databases are/arent.
This is where the confusion comes from.

@dresende: from what I can see, the data from the database is being discarded here due to different case.
If the user calls Model.sync() there won't be a problem, but if the table is pre-existing, and with different column case, it will continue to trip people up..
I'm not sure of a good solution, will have a think.

EDIT:

SELECT `name` FROM country WHERE `name` LIKE 'n%';

name: Norway

SELECT `Name` FROM country WHERE `Name` LIKE 'n%';

Name: Norway

Hmm so the database is returning the right thing. The issue lies either in the mysql driver or ORM. Will continue investigating tomorrow..

@GoNode5
Copy link
Author

GoNode5 commented Apr 10, 2013

@dxg thnx... well explained your english is much better... As said before, I think its a mismatch between the results set and the fields in the model. Although Mysql use mixcased you can't have 2 fields like Test and test... So like the query that only use lowercase (SELECT * FROM country WHERE name LIKE 'n%' ) you should lowercase the match between the result fields and the model fields... then it doesn't matter wether the model or sql or table/colums uses mixcase

@dresende
Copy link
Owner

Mysql driver returns the columns right, as were defined when creating the table. ORM is case sensitive in this case because js object keys are case sensitive. I'm not sure it will be easy to make it insensitive.

@GoNode5
Copy link
Author

GoNode5 commented Apr 10, 2013

with Country2.find( { Name:orm.like("n%") }
you first internally create an sql statement (where column names seems to be lowercase) right?
then you get an result from mysql (where column names are case sensative) right?

somewhere the model is filled with the resultset right? on that part you can temporary lowercase to match the resultset-columns with the model columns?

@dresende
Copy link
Owner

It could be whether after getting the result from mysql (or other driver) or when filling properties in every instance. It's probably better to have this behaviour active only with a setting as it adds more code and probably reduces performance.

@dxg
Copy link
Collaborator

dxg commented Apr 10, 2013

Existing table:

CREATE TABLE `country` (
  `id` int(11) unsigned NOT NULL AUTO_INCREMENT,
  `Code` varchar(3) DEFAULT NULL,
  `Name` varchar(52) DEFAULT NULL,
  PRIMARY KEY (`id`)
);
var Country = db.define("country", {
  code : { type: "text", size: 3  },
  name : { type: "text", size: 52 }
});

The following returns a null name since orm is case sensitive:

Country.find( { name:orm.like("n%") }, function (err, countries) {
  console.log(countries[0].name);
});
SELECT * FROM `country` WHERE `name` LIKE 'n%'

The following returns the name as expected:

Country.find( { name:orm.like("n%") }, {only: ['name']}, function (err, countries) {
  console.log(countries[0].name);
});
SELECT `name` FROM `country` WHERE `name` LIKE 'n%'

When you specify the column name in the select, it comes back with the case you asked for. Otherwise it comes back with it's original case.

If all select queries included a list of the models properties, then this would work in mysql.
I checked postgres, and whilst it doesn't work the same way (quoted column names are case sensitive as you mentioned above), it will throw an error:
ERROR: column "name" does not exist which the user will see :)

I think adding a list of the models properties to all selects is a good solution. Thoughts?

@dresende
Copy link
Owner

I'm not sure. It's good and bad. If you have a subset of the properties, query should have only what you need (pro), but if you have a big table the query will be very long (con).

@dxg
Copy link
Collaborator

dxg commented Apr 11, 2013

There's some more discussion about the issue here: http://stackoverflow.com/questions/65512/which-is-faster-best-select-or-select-column1-colum2-column3-etc with various pros/cons listed.

@dresende
Copy link
Owner

I like the pros, never thought about some of them.

@dxg: do you think the lib should select just the defined properties?

@GoNode5
Copy link
Author

GoNode5 commented Apr 11, 2013

pls select only columns that needed by default... most of the time you dont need all columns... I see it like master/detail

@dresende
Copy link
Owner

Don't worry, I'm going to change this.

@dxg
Copy link
Collaborator

dxg commented Apr 11, 2013

Yeah, defined properties [normal+association].

I noticed that sometimes hasOne association properties aren't created:

var Country = db.define("country", {
  code : { type: "text", size: 3  },
  name : { type: "text", size: 52 }
});
Country.hasOne('world', ...);
c = new Country();
console.log(c.code)
console.log(c.world_id)

will print out:

null
undefined

since association properties arent created when you call new (but they are when you call find/get, or if you call new and supply world_id).

It sounds like models should have a list of all defined properties to make this more consistent and less error prone, rather than compiling the list each time.
It also sounds like you'd need such a list to implement the select col1, col2.. behaviour, so we should win all round :)

@dresende
Copy link
Owner

Yes, I have a change with that list. I'm just fixing this list to have associations.

@dresende
Copy link
Owner

Please test this change when you can.

@GoNode5
Copy link
Author

GoNode5 commented Apr 12, 2013

    var  Country = db.define("country", {
            code    : { type: "text", size: 3 },
            name    : { type: "text", size: 52 }
    })

Country.find( {Name:orm.like("n%") }, function (err, countries) {
 console.log(err);

{ [Error: ER_BAD_FIELD_ERROR: Unknown column 'id' in 'field list'] code: 'ER_BAD
_FIELD_ERROR', index: 0 }

@GoNode5
Copy link
Author

GoNode5 commented Apr 12, 2013

cant you give the model, like methods, an search/find option where you can define searches...

default will be all/*

find: {
            shortList: {code+name};
            longlist: {code+name+language};
        }

@dresende
Copy link
Owner

@GoNode5 : you should define the model properties as you have them in the table. In your case, at least name should be Name.

cant you give the model, like methods, an search/find option where you can define searches...

What do you mean by this? I did not understand your example.

@GoNode5
Copy link
Author

GoNode5 commented Apr 12, 2013

error is about 'id' ? Changing from 'name' to 'Name' didnt help. (btw there is no id field, primary key is Code in the country table)

  var  Country = db.define("country", {
            Code    : { type: "text", size: 3 },
            Name    : { type: "text", size: 52 }
    });
{ [Error: ER_BAD_FIELD_ERROR: Unknown column 'id' in 'field list'] code: 'ER_BAD
_FIELD_ERROR', index: 0 }

find would be a property of the model where you could predefine column(s) to use in find, instead of define them for every Find

Model.find( [columns /pre defined finds from model / or select all*][ conditions ] [, options ] [

@dresende
Copy link
Owner

If primary key is Code, you're doing it wrong. Your Country model should be defined like this:

var Country = db.define("country", {
    Name : { type: "text", size: 52 }
}, {
    id : "Code"
});

@GoNode5
Copy link
Author

GoNode5 commented Apr 12, 2013

sorry forget the syntax... now its working the actual query has 2x Code?

    var  Country = db.define("country", {
            Code    : { type: "text", size: 3 },
            Name    : { type: "text", size: 52 },
            Region    : { type: "text", size: 26 },
            Continent : [ "Asia", "Europe", "North America ", "Africa", "Oceania","Antartica",   "South America" ],         
    }, {
    id : "Code"
    });

Country.find( {name:orm.like("n%") }, function (err, countries) {
 console.log(countries[0].Code);
 console.log(countries[0].Name);
 console.log(JSON.stringify(countries[0]));
})

(orm/mysql) SELECT Code, Code, Name, Region, Continent FROM country
WHERE name LIKE 'n%'
ANT
Netherlands Antilles
{"Code":"ANT","Name":"Netherlands Antilles","Region":"Caribbean","Continent":"No
rth America"}

@dresende
Copy link
Owner

You added id: "Code" but you forgot to remove Code from properties.

@GoNode5
Copy link
Author

GoNode5 commented Apr 13, 2013

it's very confusing... I would rather have:

Code : { type: "text", size: 3,primary-key, unique, required},
Name : { type: "text", size: 52, required },

@dresende
Copy link
Owner

Primary keys are not defined in the properties. The primary key is auto included by ORM. If you add it to the properties it will be duplicated and probably fail a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants