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

Flexible attribute properties patch #96

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gstreetmedia
Copy link

This small change allows a developer to define model attribute properties in anyway they wish. I believe this implementation provides better flexibility in model attribute definitions while maintaining backwards compatibility for those who rely upon the attribute whitelist (likely unknowingly).

attributeWhitelist : "flexible"

can be defined on a per model basis or with config/model.js to make this change globally.

Allow the user to define whatever properties they would like on an attribute, while still maintaining standard attribute validation.
Adds a flag: attributeWhitelist to model to bypass attribute property validation. While this is fine, it doesn't allow for custom properties to be added to attributes.  This might be helpful to some users, but others might want to make distinct model schemas the contain isMobilePhone, or JSON Schema style snytax for an enum, etc.
@mikermcneil
Copy link
Member

@gstreetmedia hey there! Totally understand where you're coming from- luckily we provide a way to do this now (#95). The conversation in that PR also covers a bit about the reasoning

@gstreetmedia
Copy link
Author

gstreetmedia commented May 25, 2018

meta seems like a good solution for some things. I could also see offering up additional items that would merged into the "whitelist" Sort of saying, yes I know you have your whitelist, but I have my own too. So validate that.

So maybe instead of

attributeWhitelist : flexible

something like

attributeWhitelist : ['isMobilePhone', 'isDivisibleBy',.....]

This would address the issue you outlined, where there were a lot of errors by not validating attribute properties, while still allowing developers to provide an additional whitelist of things they know to be good.

Sudo coded to something like

var validProperties = require('../../accessible/valid-attribute-properties');
....
var validPropertiesToCheck = _.merge(validProperties, schemaCollection.attributeWhitelist)
...
if (_.indexOf(validPropertiesToCheck, propertyName) < 0) {

There are also a lot of validations in the chriso/validator library not covered in waterline. I'd be happy to add some of those and submit a PR. isMoblePhone, isFloat, isCurrency, isBase64...lots of good stuff that would make waterline better.

@danielsharvey
Copy link

Further to @mikermcneil's comments above, the solution is probably to use meta property?

To aide others, I've added this to the documentation here balderdashy/sails-docs#1308.

Can probably close this PR?

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

Successfully merging this pull request may close these issues.

3 participants