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

Autoincrement property is too mysql specific #5812

Closed
dtoubelis opened this issue Mar 29, 2015 · 6 comments
Closed

Autoincrement property is too mysql specific #5812

dtoubelis opened this issue Mar 29, 2015 · 6 comments

Comments

@dtoubelis
Copy link

I'm working on a waterline-cassandra adapter at https://github.com/dtoubelis/sails-cassandra and it looks to me that autoIncrement property is too restrictive. I think the autoIncrement mechanism is too MySQL specific. Oracle and Postgres can use sequences, another option is using UUID. The problem is that autoIncrement attribute also enforces the field type to integer, however this is not the case with UUIDs and potentially sequences that can support values larger than 32-bit integer. So, here are few suggestions:

  1. First of all, prevent autoIncrement attribute enforcing integer type and leave it either up to developer or to the adapter to provide the correct type. Same is true for enforcing unique: true. Even though it seems harmless, it unnecessarily interferes with developer's intent.
  2. The second suggestion is to replace autoincrement feature with something more generic (maybe similar to Hibernate), for example:
attributes: {
  id: {
    type: `string`,
    primaryKey: true,
    autoGenerated: {
      generator: 'native'    // 'uuid', 'sequence', etc. depending on what the adapter provides
    }
  },
  ...
}

and make autoIncrement: true synonym to autoGenerated: {generator: 'native'} for backward compatibility.

This would make the interface more generic and more compatible with other data sources.

P.S.: The autoGenerated is not necessarily has to be property of a primary key. I think that any attribute should be allowed to have this property.

@dmarcelino
Copy link
Member

Hi @dtoubelis, regarding the attribute id specifically you can configure your adapter to have pkFormat of type string (example). This is only true for id (or for whatever attribute you set as primaryKey). I think your suggestions make a lot of sense. 👍

@devinivy
Copy link

I like it! This would apply nicely to the sails-dynamodb adapter too, among others. What do you think of adding this to the 0.11 milestone, @particlebanana ?

@dtoubelis
Copy link
Author

@dmarcelino: I actually have pkFormat set to string in my adapter but it seems that autoIncrement supercedes this definition and forces it to integer anyway.

@devinivy
Copy link

And that can be seen here: https://github.com/balderdashy/waterline/blob/7aa74cb8fd61407c2bc6391cab7db81932af4256/lib/waterline/core/schema.js#L126-L129

That said, I have had some luck implementing UUIDs using autoIncrement on the sails-dynamodb adapter. I suppose that may be because of the restricted way I'm using the adapter. We should look into it.

@sailsbot
Copy link

Thanks for posting, @dtoubelis. I'm a repo bot-- nice to meet you!

It has been 60 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

  • review our contribution guide to make sure this submission meets our criteria (only verified bugs with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
  • create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to the original issue. This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

@dtoubelis
Copy link
Author

Any plans on implementing this?

@johnabrams7 johnabrams7 transferred this issue from balderdashy/waterline May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants