-
Notifications
You must be signed in to change notification settings - Fork 152
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
Allows for other auto increment scenarios other than numeric #278
Allows for other auto increment scenarios other than numeric #278
Conversation
@gstreetmedia thanks! |
…jobs/368925613). This is because #278 actually breaks normal usage (because logical types are not made accessible to the adapter in the per-table DDL spec passed into the define() adapter method- instead, another approach must be used)
@gstreetmedia for future reference, worth noting that this approach actually breaks normal usage (because we don't have access to the attributes logical |
I'm glad you revisited this. Almost as soon as I sent of the PR, I realized this should be handled based on columnType. Your solution looks great. For what its worth, the mysql adapter seems to make this same assumption that autoIncrement is numeric. From the documentation:
Strictly speaking, that isn't currently the case, as both mysql and postgres adapters may still modify the columnType if you set "autoIncrement", "unique", "notNull" to true. If a complex columnType is present, perhaps there is an easy way to bypass the logic that modifies the columnType (e.g. the addition of UNIQUE, NOT NULL, AUTO_INCREMENT)? As a side note, i'd love to see a "beforeValidate" callback in Waterline, along with global lifecycle callbacks in /config/models.js. This would allow for the creation of all kinds handy things in a more global manner. In our specific case, we could have inserted a code generated uuid during beforeValidate and left the columnType set to string. I know it's possible to just sub in and "id" during create, but if you have 100's of Model.create({id:uuid.v4()}) instances it becomes challenging to maintain. |
good point, headed out atm, but if you happen to think of this, would welcome a PR to the docs in this area |
Currently the id for a model requires that it be either "required" or "autoIncrement". However, when setting autoIncrement the value in columnType is ignored. The assumption seems to be that autoIncrement can only be done numerically. This small change allows the developer to determine how the autoincrement should work.