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

Table name defaults to lower case of name #125

Open
moraneva opened this issue Dec 20, 2017 · 5 comments
Open

Table name defaults to lower case of name #125

moraneva opened this issue Dec 20, 2017 · 5 comments

Comments

@moraneva
Copy link

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
Copy link
Owner

clarkie commented Dec 21, 2017

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
Copy link
Author

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
Copy link
Collaborator

cdhowie commented Dec 21, 2017

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
Copy link
Author

@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
Copy link
Collaborator

cdhowie commented Dec 21, 2017

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.)

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