Label is not working with other constraints

[alimozdemir]

Hello,

I have noticed an issue about naming label in error messages. Here is the example

Let's say I have a property in my schema

    "priceAmount": {
      "type": "integer",
      "title": "Price Amount"
    },

So this field is required and error message is like Price Amount is a required field.

    "priceAmount": {
      "type": "integer",
      "title": "Price Amount",
      "minimum": 100,
      "maximum": 500,
    },

But when I add minimum maximum properties to the definition it ignores the title field.

And, error became priceAmount is a required field.

Any ideas?

[kristianmandrup]

I recommend adding a custom error function, then you have full control and have access to title. See the Readme.

[alimozdemir]

Yes that example is not working, have you tried it recently? btw there is a bug inside of the library custom function should be the last solution :)

[alimozdemir]

Btw, the label method that library uses is the same functionality that I want, I don't want it to customize it. It seems like it is overriding label of yup.

[kristianmandrup]

Looking at the code in mixed.js:

  get mixedEnabled() {
    return (
      this.mixedConfig.enabled || [
        "oneOf",
        "notOneOf",
        "when",
        "nullable",
        "isType",
        "label",
        "const",
        "refValueFor",
      ]
    );
  }

  label() {
    const value = this.value;
    const label = value.title || value.label;
    this.base = (label && this.base.label(label)) || this.base;
    return this;
  }

This means that the method label() will be called, which should set label on yup to either title or label if set, in that precedence order.

I believe the bug you point to might originate in constraints/base.js. The constraints minimum and maximum for Number type is handled by Range constraint, which inherits from Constraint which inherits from constraints/Base class.

  addConstraints(method, names = []) {
    names.map(name => {
      const value = this.validateAndTransform(name);
      this.addConstraint(name, { method, value });
    });
    return this;
  }

The issue could be that it passes in method in this.addConstraint(name, { method, value });

    constraintName = constraintName || propName;
    method = method || constraintName;

    // ...

    const constraintErrMsg = this.validationErrorMessage(constraintName);
    const errErrMsg = errName && this.validationErrorMessage(errName);

    const errFn = constraintErrMsg || errErrMsg;

The errFn is then used later and passed to yup as the error handler such as in presentConstraintValue

    const newBase = constraintFn(constraintValue, errFn);
    return newBase;

However I see constraints such as email using this as well with even more props. I guess ideally you would want it not to pass any error handler to yup and let yup handle the error message displayed based on whether yup.label has been set and if not defaulting to the prop name?

  email() {
    if (!this.isEmail) return this;
    const constraintName = this.constraintNameFor("email", "format");
    const method = "email";
    this.addConstraint("email", {
      constraintValue: true,
      constraintName,
      method,
      errName: method,
    });
    return this;
  }

  constraintNameFor(...names) {
    return names.find((name) => this.constraints[name]);
  }

[kristianmandrup]

I pushed a new branch err-msg-label which should add more debugging info if you enable detailed logging for the specific property causing an issue. That should greatly help track it down.

[alimozdemir]

Btw, thank you for your detailed answer, It was helpful

Okay, I have been trying to understand the source code and I found something about range constraint.

in the src/types/number/index.js there is a method

  range() {
    this.rangeConstraint.add();
  }

I have added following console logs

  range() {
    console.log('before range', this.base.describe());
    this.rangeConstraint.add();
    console.log('after range', this.base.describe());
  }

before range {
  meta: undefined,
  label: 'My age',
  type: 'number',
  oneOf: [],
  notOneOf: [],
  tests: [ { name: 'required', params: undefined } ]
}

after range {
  meta: undefined,
  label: undefined,
  type: 'number',
  oneOf: [],
  notOneOf: [],
  tests: [
    { name: 'required', params: undefined },
    { name: 'min', params: [Object] }
  ]
}

And here is the log, it seems like it is losing the label.

[kristianmandrup]

Very interesting. I've never been focused on the labels. Would be good to add some u it tests for that, especially for the constraints such as Range, since hey are handled slightly differently than those that are defined as methods in the basic types.

Ideally the whole project should be reengineered with a modular approach using plugins for "plug and play".

I tried that a few times but never managed to complete it.

[alimozdemir]

Could it be related with inheritance? Somehow maybe it is losing super.base and losing the mixed rules. What is your first insight?

Btw I don't think this is just a label issue, I will try this with other things also.

[kristianmandrup]

Very interesting. I've never been focused on the labels. Would be good to add some u it tests for that, especially for the constraints such as Range, since hey are handled slightly differently than those that are defined as methods in the basic types.

Ideally the whole project should be reengineered with a modular approach using plugins for "plug and play".

I tried that a few times but never managed to complete it.

[kristianmandrup]

Could well be related to inheritance, an anti pattern... Composition over inheritance

[kristianmandrup]

Why not just add a custom ErrorHandler for now so you complete control the error message logic?

[alimozdemir]

The example (error handler) in the readme does not make sense, and I couldn't applied that. Could you give me a working example?

btw, isn't possible to find a solution for this bug?

[kristianmandrup]

I will look into it tmrw or later this week

[kristianmandrup]

The following fix (in master) should let you override the current buggy behavior

https://github.com/kristianmandrup/schema-to-yup#Custompipelinesteps

I will then look to fix the constraints bug - most likely due to delegation/inheritance as you pointed out

[alimozdemir]

Ah perfect! I will try it when it is released 1.12.3.

Thank you for your detailed answers

[kristianmandrup]

1.12.3 includes pipeline customization options for pre and post convert steps. Also documented in the Readme.

[alimozdemir]

I just tested it, it is working! thank you for the workaround :) let me know if you need any help about inheritance issue

[kristianmandrup]

Great 😃 Yeah, I'd love some help with that if you have the time

[kristianmandrup]

I think I found the issue.

For number/index.js

class YupNumber extends YupMixed {
  constructor(obj) {
    super(obj);
    this.type = this.baseType;
    this.base = this.validatorInstance;
    this.rangeConstraint = createRangeConstraint(this);
  }

  range() {
    this.rangeConstraint.add();
  }

  addConstraint(propName, opts) {
    if (!this.constraintBuilder) {
      throw new Error(
        `[YupMixed] addConstraint: Missing constraintBuilder in ${this.constructor.name}`
      );
    }
    const constraint = this.constraintBuilder.addConstraint(propName, opts);
    if (constraint) {
      this.base = constraint;
    }
    return this;
  }

The key part is:

    const constraint = this.constraintBuilder.addConstraint(propName, opts);
    if (constraint) {
      this.base = constraint;
    }

Constraint base class delegates addConstraint to type handler binding this for each to the type handler via bind(typeHandler)

class Constraint extends TypeMatcher {
  constructor(typeHandler, map) {
    super(typeHandler.config);
    this.map = map || this.$map || {};
    this.typeHandler = typeHandler;
    this.delegates.map(name => {
      const delegate = typeHandler[name];
      if (!delegate) {
        this.error(`missing delegate: ${name}`, {
          typeHandler
        });
      }
      this[name] = this.isFunctionType(delegate)
        ? delegate.bind(typeHandler)
        : delegate;
    });
  }

  get delegates() {
    return ["constraints", "addConstraint", "constraintsAdded"];
  }

  addConstraints(method, names = []) {
    names.map(name => {
      const value = this.validateAndTransform(name);
      this.addConstraint(name, { method, value });
    });
    return this;
  }

  add() {
    const $map = this.map;
    Object.keys($map).map(yupMethod => {
      const names = this.entryNames($map[yupMethod]);
      this.addConstraints(yupMethod, names);
    });
  }

But essentially range constraints handler for Number looks like

  range() {
    this.rangeConstraint.add();
  }

Whereas a typical constraint looks sth like:

  maxLength() {
    const { maxLength } = this.constraints;
    const errMsg =
      this.validationErrorMessage("maxLength") ||
      this.validationErrorMessage("max");
    const newBase = maxLength && this.base.max(maxLength, errMsg);
    this.base = newBase || this.base;
    return this;
  }

Crucially with

    const newBase = maxLength && this.base.max(maxLength, errMsg);
    this.base = newBase || this.base;

So if the add method of Constraint returned the new base

  add() {
    const $map = this.map;
    Object.keys($map).map(yupMethod => {
      const names = this.entryNames($map[yupMethod]);
      this.addConstraints(yupMethod, names);
    });
    return this.typeHandler.base
  }

And range then set the base accordingly

  range() {
    const newBase = this.rangeConstraint.add();
    this.base = newBase || this.base;
  }

It should most likely work the same as for the normal cases.