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

Unexpected default error message for anchor's maxLength rule #4611

Open
Rua-Yuki opened this issue Dec 19, 2018 · 4 comments
Open

Unexpected default error message for anchor's maxLength rule #4611

Rua-Yuki opened this issue Dec 19, 2018 · 4 comments
Labels
bug helpful info or workaround orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc.

Comments

@Rua-Yuki
Copy link

Waterline version: 0.13.6
Node version: 10.14.2
NPM version: 6.4.1
Operating system: macOS 10.13.3


There appears to be a very small issue with the default message reported by anchor's maxLength rule. Specifically, with the changes introduced to improve validation error messages (sailshq/anchor@18e0e7f) in v1.4.0, it appears that the length overage is displayed as a negative number.

For example, it's possible to receive something like the following:

Could not use specified `lenTestField`.  Violated one or more validation rules:
  • Value was -4 characters longer than the configured maximum length (2)

The specific issue portion is the "-X characters longer" phrase, which doesn't make much sense.


This issue can be reproduced easily given the following model definition and usage code:

/**
 * @file ValidationTestModel.js
 */

module.exports = {

  attributes: {
    lenTestField: {
      type: 'string',
      maxLength: 2,
    },
  }

};
/**
 * Short and simple promise-based code that can be executed in the `sails console`,
 * for your convenience.
 */

ValidationTestModel.create({
    lenTestField: 'potato',
}).catch(err => {
    sails.log.error('Woops:', err);
});

The errant line of code appears to be:
https://github.com/sailshq/anchor/blob/18e0e7fd144f0246cb7cdad5d8501681dc52b41a/lib/rules.js#L262

The specific culprit being that the overage calculation arithmetic is backwards, with maxLength-x.length written rather than x.length-maxLength on two occasions.

As a side-effect of always being negative, the pluralisation logic intended to select between outputting the singular "character" or plural "characters" will not function as expected. Reordering both operations as mentioned above will solve this pluralisation issue bit as well.


My apologies for the long and pedantic issue report!

(Also I wasn't certain if I should file this under the main sails repo or this one – sorry if I've goofed up in this regard!)

@sailsbot
Copy link

@Rua-Yuki Thanks for posting, we'll take a look as soon as possible.


For help with questions about Sails, click here. If you’re interested in hiring @sailsbot and her minions in Austin, click here.

Rua-Yuki referenced this issue in Rua-Yuki/anchor Dec 19, 2018
This resolves a simple issue where the overage amount displayed for the
maxLength rule’s default error message was always displayed as a
negative number, for example: “Value was -4 characters longer than the
configured maximum length”

See the corresponding issue for more information:
https://github.com/balderdashy/waterline/issues/1589
@Rua-Yuki
Copy link
Author

Sidenote / Gotcha: Due to npm's respect for semantic versioning this of course won't affect every project on Waterline 0.13.6. Only projects which have had the latest sailshq/anchor pulled in will be affected by the issue.

@johnabrams7
Copy link
Contributor

johnabrams7 commented Feb 18, 2019

@Rua-Yuki Thanks for taking the time to explore this error and providing a simple solution to the discrepancy in the code. PR located here.

@raqem raqem transferred this issue from balderdashy/waterline Mar 6, 2019
@balderdashy balderdashy deleted a comment from sailsbot Mar 6, 2019
@raqem raqem added bug orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc. and removed needs cleanup labels Mar 6, 2019
@raqem
Copy link
Contributor

raqem commented Mar 6, 2019

Hi @Rua-Yuki,
Thank you for making a pull request to fix the issue!
We are currently in the process of consolidating all of Sails related issues into one repo to help keep better track of issues.
Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug helpful info or workaround orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc.
Development

No branches or pull requests

4 participants