clarkie / dynogels

DynamoDB data mapper for node.js. Originally forked from https://github.com/ryanfitz/vogels
Other
490 stars 110 forks source link

Table name defaults to lower case of name #125

Open moraneva opened 6 years ago

moraneva commented 6 years ago

Hi there,

I was just curious if there is a good reason for the table name of a model to default to all lower case? I was dealing with a table that had upper case characters and it was a pain trying to figure out what the issue was. Are there some sort of standards somewhere around dynamo table naming? I missed that part in the documentation and it was confusing to me why the table name was being overridden by the library.

Thanks!

clarkie commented 6 years ago

Hi @moraneva,

As far as I know there isn't a "standard" when it comes to naming dynamodb tables. You can modify the table name using a helper:

const eventTable = dynogels.define('event', {
  hashKey: 'id',
  timestamps: true,
  schema,
  indexes,
  tableName: name => name.toUpperCase(),
});

are you suggesting we use the first argument of define as the table name? I'd be happy to see a PR if you're up for it?

Thanks

Clarkie

moraneva commented 6 years ago

Cool, didn't now that you could modify that in the config. Specifically I'm talking about https://github.com/clarkie/dynogels/blob/master/lib/index.js#L67

  const tableName = name.toLowerCase();

  const table = new Table(tableName, schema, serializer, internals.loadDocClient(), log);

So if I didn't set the table name in the schema, it uses the first argument of define which is cast to lower case here. Is there a reason for doing that casting in the define? Because the table name definitely uses this if there is nothing defined in the schema:

Table.prototype.tableName = function () {
  if (this.schema.tableName) {
    if (_.isFunction(this.schema.tableName)) {
      return this.schema.tableName.call(this);
    } else {
      return this.schema.tableName;
    }
  } else {
    return this.config.name;
  }
};
cdhowie commented 6 years ago

Personally, I explicitly state the name of the table as a string in the config.

const FooTable = dynogels.define('FooTable', {
    tableName: 'FooTable',
    // ...
});

I found that dynogels tries (tried?) to pluralize the table, for example, and that the best way to ensure that I know exactly what the table will be named is to explicitly name it. This is my current recommendation as a best practice.

IMO, tableName should default to exactly the name of the model, but this is a backwards-incompatible change.

moraneva commented 6 years ago

@cdhowie yeah good point, didn't even think about the compatibility of it. Maybe it could be a feature for the next major release?

cdhowie commented 6 years ago

I'm fully on board with that. I think that having dynogels munge the table name in any way violates the principle of least surprise. (To be fair, this functionality was inherited from vogels.)