Problem with columns named 'length' #76

Closed
richtera opened this Issue Feb 15, 2013 · 6 comments

Comments

Projects
None yet
3 participants

length conflicts with Object.length since the code does object[columnName] = ...
I am attempting to map columns named "length" with "$length" but not sure how many places this would need to be done. Has anyone else run into this problem?
I added this to the routine reading the metadata.
if (colName === 'length')
colName = '$length'; // Alias the column name to prevent conflict with Object.length

Collaborator

pekim commented Feb 17, 2013

The same problem will arise if a result set has a column named 'metadata', '0', '1' or any integer less than the number of columns.

Renaming the problematic column names (by prefixing them with a '$') in tedious isn't necessarily a good solution. It makes problems less likely, but doesn't make them go away entirely.

Consider a (somewhat crazy, but valid) query such as this.

select    1 as [length],
          2 as [$length],
          3 as [metadata],
          4 as [$metadata],
          5 as [0],
          6 as[$0]

Renaming the troublesome columns in tedious isn't going to deal with it in an acceptable manner. The renamed first column will have the same name as the second column.

The root cause of the problem is that I made a bad decision with the API.

I can think of two ways to address the problem.

  1. Do not change the code. Update the documentation to make clear that 'length', 'metadata' and integers are not supported as column names. The workaround would be to alias the columns in the query.
    • Pro: It's easy just to update the documentation.
    • Pro: Very low risk.
    • Pro: No API change.
    • Con: Queries have to alias problematic column names.
  2. Change the API:
    The columns object exposed by the row event would be restructured to something like.
    columns = {
        byName: {
          length: {metadata:{...}, value: ...},
          "$length": {metadata:{...}, value: ...},
          "metadata": {metadata:{...}, value: ...},
          ...
        },
        byIndex: [
          {metadata:{...}, value: ...},
          {metadata:{...}, value: ...},
          {metadata:{...}, value: ...},
          ...
        ]
    }
* Pro: Column names do not have to be aliased in queries.
* Pro: It's probably the right thing to do.
* Con: The API changes will affect almost users of tedious.

I was using the api to import tables so I was not able to alias columns
since I was relying to get the metadata from tedious. For my use case a
flag to turn off named aliases of columns and only using indexed would be
adequate. But obj['length'] = value crashes under node no matter what, so
even the other byName structure will cause the same problem. Only using
separate get() and set() functions can get around the problem and that's
probably a larger overhead than using objects as hashes.
Andy

Sent from my iPhone

On Feb 17, 2013, at 4:15 AM, Mike D Pilsbury notifications@github.com
wrote:

The same problem will arise if a result set has a column named 'metadata',
'0', '1' or any integer less than the number of columns.

Renaming the problematic column names (by prefixing them with a '$') in
tedious isn't necessarily a good solution. It makes problems less likely,
but doesn't make them go away entirely.

Consider a (somewhat crazy, but valid) query such as this.

select 1 as [length],
2 as [$length],
3 as [metadata],
4 as [$metadata],
5 as [0],
6 as[$0]

Renaming the troublesome columns in tedious isn't going to deal with it in
an acceptable manner. The renamed first column will have the same name as
the second column.

The root cause of the problem is that I made a bad decision with the API.

I can think of two ways to address the problem.

Do not change the code. Update the documentation to make clear that
'length', 'metadata' and integers are not supported as column names. The
workaround would be to alias the columns in the query.
- Pro: It's easy just to update the documentation.
- Pro: Very low risk.
- Pro: No API change.
- Con: Queries have to alias problematic column names.
2.

Change the API:
The columns object exposed by the row event would be restructured to
something like.

columns = {
byName: {
length: {metadata:{...}, value: ...},
"$length": {metadata:{...}, value: ...},
"metadata": {metadata:{...}, value: ...},
...
},
byIndex: [
{metadata:{...}, value: ...},
{metadata:{...}, value: ...},
{metadata:{...}, value: ...},
...
]
}

- Pro: Column names do not have to be aliased in queries.
  - Pro: It's probably the right thing to do.
  - Con: The API changes will affect almost users of tedious.


Reply to this email directly or view it on
GitHubhttps://github.com/pekim/tedious/issues/76#issuecomment-13683067.

Collaborator

pekim commented Feb 17, 2013

Using a simple object, instead of an array should be fine.

> var o = {length: 'qaz', '1': 'one'};
undefined
> o.length
'qaz'
> o['1']
'one'
> 

However it would mean an undesirable breaking change to the API.

I see what you mean about a flag to disable population of the row's values by name. That would be a reasonable way to work around the problem.
I'll look in to implementing it.

Now I understand what you were saying by separating the array from the named items you can fix the problem.
Sorry I didn't understand in your previous e-mail. Yes that would work.
Andy

On Feb 17, 2013, at 9:23 AM, Mike D Pilsbury notifications@github.com wrote:

Using a simple object, instead of an array should be fine.

var o = {length: 'qaz', '1': 'one'};
undefined
o.length
'qaz'
o['1']
'one'

However it would mean an undesirable breaking change to the API.

I see what you mean about a flag to disable population of the row's values by name. That would be a reasonable way to work around the problem.
I'll look in to implementing it.


Reply to this email directly or view it on GitHub.

Hmmmm. Maybe if you initialize columns as an object rather than array? An object can still have indices 0, 1, 2, 3.
An then there is no conflict with length since it's not an array class.
Andy

On Feb 17, 2013, at 9:23 AM, Mike D Pilsbury notifications@github.com wrote:

Using a simple object, instead of an array should be fine.

var o = {length: 'qaz', '1': 'one'};
undefined
o.length
'qaz'
o['1']
'one'

However it would mean an undesirable breaking change to the API.

I see what you mean about a flag to disable population of the row's values by name. That would be a reasonable way to work around the problem.
I'll look in to implementing it.


Reply to this email directly or view it on GitHub.

@patriksimek patriksimek self-assigned this Feb 11, 2014

@patriksimek patriksimek added the change label Mar 10, 2014

@patriksimek patriksimek added this to the v0.2.0 milestone Mar 10, 2014

patriksimek added a commit that referenced this issue Mar 31, 2014

#76 #126 - Results (rows and column metadata) are now simple arrays
Optionaly key-value collections (useColumnNames)
Collaborator

patriksimek commented Apr 1, 2014

Fixed in upcoming release 0.2.0.

@patriksimek patriksimek closed this Apr 1, 2014

momow pushed a commit to momow/tedious that referenced this issue Oct 15, 2014

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