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

Suggested model property validation additions and/or a different way (see below) #95

Closed
wants to merge 2 commits into from

Conversation

drj-io
Copy link

@drj-io drj-io commented Apr 18, 2018

Hi there, Mike. We are working on building a CRUD model generator for SAILSJS and the model schema validations are throwing things off. In our opinion, any model property validation prevents extending the system with dynamic capabilities. There may be a security reason for validating model properties, but if there's not, we recommend removing the model property validation all together (line 237-241 in /lib/waterline-schema/schema.js). Another option is to allow us to register our own model property types that the validator can validate against. Thank you for considering our ideas as a feature request and a way to make great software, quickly. -- David

p.s. hope you guys are doing well!

david added 2 commits April 18, 2018 14:38
… model schema validations are throwing things off. In our opinion, any model property validation prevents extending the system with dynamic capabilities. There may be a security reason for validating model properties, but if there's not, we recommend removing the model property validation all together (line 237-241 in /lib/waterline-schema/schema.js). Another option is to allow us to register our own model property types that the validator can validate against. Thank you for considering our ideas as a feature request and a way to make great software, quickly. -- David
… model schema validations are throwing things off. In our opinion, any model property validation prevents extending the system with dynamic capabilities. There may be a security reason for validating model properties, but if there's not, we recommend removing the model property validation all together (line 237-241 in /lib/waterline-schema/schema.js). Another option is to allow us to register our own model property types that the validator can validate against. Thank you for considering our ideas as a feature request and a way to make great software, quickly. -- David
@mikermcneil
Copy link
Member

Howdy @drj-io! We've gone back and forth on this a bit, and we ended up using meta as a way to accomplish this. So that's what I'd recommend using in general.

That said, having been down the road it seems you're on, I think I understand why you need label, etc, and I think there might be a more standardized solution for some of them. Here's what I'd suggest:

  • label could you use friendlyName for this? (I realize we'd need to add that one in-- but we're using it across the board for a few things, including in the machine spec, so I'd be down)
  • notEditable could you use readOnly for this? (I realize we'd need to add that one in-- but we're using it across the board for a few things, including in the machine spec, so I'd be down)
  • toolTip, If ≤140 characters, you could you use description for this. Otherwise, if it's longer, markdown-compatible text, consider extendedDescription. Otherwise if neither of those work, I'd say use meta.toolTip
  • 'hideFromTable', recommend using meta.hideFromTable for this
  • 'hideFromForm', recommend using meta.hideFromForm for this
  • 'filterable', recommend using meta.filterable for this
  • 'beforeFormRender', recommend using meta.beforeFormRender for this
  • 'beforeIndexRender', recommend using meta.beforeIndexRender for this

@mikermcneil
Copy link
Member

Also just to give you an answer re:

There may be a security reason for validating model properties

It's not as much security as it is to prevent common errors (we were seeing sooooo many issues-- from the community and in practice on our own projects. Particularly where one attr def accidentally gets nested inside another)

@drj-io
Copy link
Author

drj-io commented Apr 18, 2018

Hey @mikermcneil, great ideas. I think what you propose will work quite well. I didn't realize that meta was meant to be utilitarian. It looks like I can put some objects in there for my controller/model abstractions.

Thanks a lot! I'm starting to head into the sea building my own generator. Here's hoping there are no icebergs on the way. 💨⛵

@drj-io drj-io closed this Apr 18, 2018
danielsharvey added a commit to danielsharvey/sails-docs that referenced this pull request Apr 8, 2020
There are a number of valid attribute properties that are not currently
included in the documentation. Add them.

References:
1. Explanation: balderdashy/waterline-schema#95 (comment)
@danielsharvey
Copy link

Just a note to other that come across this PR, I've added this to the documentation here balderdashy/sails-docs#1308.

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