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

Wish there was even more detailed error handling in sanctuary-def #274

Open
voxbono opened this issue Nov 24, 2019 · 4 comments
Open

Wish there was even more detailed error handling in sanctuary-def #274

voxbono opened this issue Nov 24, 2019 · 4 comments

Comments

@voxbono
Copy link

voxbono commented Nov 24, 2019

I have a use case where I have a RecordType like this:

const newProductRequestBodyType = $.RecordType({
    product_number: $.NonEmpty ($.String),
    name: $.NonEmpty ($.String),
    category: $.NonEmpty ($.String),
    sub_category: $.NonEmpty ($.String),
    supplier: $.NonEmpty ($.String),
    unit: $.NonEmpty ($.String),
    volume_per_unit: $.NonEmpty ($.String)
});

And I use this function to validate it:

const isValidObjectByType = recordType => tagBy(is(recordType));

newProductRequestBodyType is a post request, where I want to validate that each field is valid. Is there a simple way, to provide an error message for each field, so that I can show the right error message to the form on the client?
The Left just gives me the sent object back, but it doesn't say why the validation of the Recordtype failed, if that makes sense?

It would be super-useful if the Left contained more details about the error.... Maybe even something I define myself when I create my RecordType.

(This issue was first mentioned on Gitter).

@davidchambers davidchambers transferred this issue from sanctuary-js/sanctuary Nov 24, 2019
@dotnetCarpenter
Copy link

dotnetCarpenter commented Oct 4, 2021

This feature is much more popular than I initial thought. #220 and #219 are also about this.

Part I

@davidchambers posted a proof-of-concept pure function on gitter that provides good error messages in English. The drawbacks being that it's painful to translate the error messages into user facing messages.

As mention in both #219 and #220, there already exist an exposed (though supposedly private) method to validate types on each type. My initial thought was to translate the internal (to sanctuary-def) error message into 3 well-define error messages.

As far as I can tell there is only 5 possible outcome of an validation of any type (unless we bake in interconnected rules, which we will not).

  1. The value is undefined/null.
  2. Properties are missing from the value.
  3. Properties are extraneous (which we will ignore as is custom in JS land).
  4. Properties are wrong.
  5. A value is correct.

Currently we have Type ~> validate -> Env -> a.

This comment is going to be long so I'm splitting it into several comments...

@dotnetCarpenter
Copy link

dotnetCarpenter commented Oct 4, 2021

Part II

My first attempts was to create a pure function that is separate from sanctuary-def, so its immediately available to try out in projects without a new release of sanctuary-def. Having well-defined error messages/objects will ease translation of said errors to public facing error messages. E.g. "Start date is not a valid date"/"Start dato er ikke en godkendt dato" or "Start date is missing"/"Start dato mangler" etc.

Example:

const BarFoo = $.NamedRecordType
  ('BarFoo')
  ('')
  ([])
  ({
    foo: $.String,
    bar: $.NonEmpty (
      $.NullaryType
        ('DateIso')
        ('')
        ([$.String])
        (x => {
          console.debug (`test ran with x = ${x}`);
          return /^\d{4}-[0-1]\d-[0-3]\d$/.test (x);
        })
    ),
    fez: $.Number,
  });

Examining the return types from Type.validate, I get the following:

// 1.1
BarFoo.validate ($.env) (null)
// -> Left ({"propPath": [], "value": null})

// 1.2
BarFoo.validate ($.env) (undefined)
// -> Left ({"propPath": [], "value": undefined})

// 2.1
BarFoo.validate ($.env) ({foo: 'blue'})
// -> Left ({"propPath": [], "value": {"foo": "blue"}})

// 2.2
BarFoo.validate ($.env) ({foo: 'blue', bar: '2020-10-10'})
// -> Left ({"propPath": [], "value": {"bar": "2020-10-10", "foo": "blue"}})

// 3.1
BarFoo.validate ($.env) ({foo: 'blue', bar: '2020-10-10', fez: 2, lorem:"zzz"})
// -> Right ({"bar": "2020-10-10", "fez": 2, "foo": "blue", "lorem": "zzz"})
// test ran with x = 2020-10-10

// 3.2
BarFoo.validate ($.env) ({foo: 'blue', bar: '2020-10-10', fez: 2, lorem:"zzz", ipsum: 123})
// -> Right ({"bar": "2020-10-10", "fez": 2, "foo": "blue", "ipsum": 123, "lorem": "zzz"})
// test ran with x = 2020-10-10

// 4.1
BarFoo.validate ($.env) ({foo: 'blue', bar: '2020-29-10', fez: 2})
// -> Left ({"propPath": ["bar", "$1"], "value": "2020-29-10"})
// test ran with x = 2020-29-10

// 4.2
BarFoo.validate ($.env) ({foo: 'blue', bar: '2020-29-10', fez: 'lol'})
// -> Left ({"propPath": [], "value": {"bar": "2020-29-10", "fez": "lol", "foo": "blue"}})

// 5
BarFoo.validate ($.env) ({foo: 'blue', bar: '2020-10-10', fez: 2})
// -> Right ({"bar": "2020-10-10", "fez": 2, "foo": "blue"})
// test ran with x = 2020-10-10

Initially it looks like converting the internal error objects to well-define error objects is straight forward. I think we can ignore extraneous properties in 3 and 5 is obviously correct.

However there is ambiguity between multiple properties being wrong (4.2) and 2, one or more properties are missing. Also in both 2 and 4.2, it is not enough to examine the return value of BarFoo.validate ($.env) (a). One have to go through the properties of a and match that with the value property of the error object.

In that case it would probably be better to leverage the fieldType._test function Value -> Boolean.

For our Type BarFoo the function is available through types

Type ~> Array Type ~> (Value -> Boolean)

Which brings me to part III.

@dotnetCarpenter
Copy link

dotnetCarpenter commented Oct 4, 2021

Part III

The original proof-of-concept function from @davidchambers:

//    validate :: Type -> a -> Either (Array String) a
const validate = t => x => {
  if (x == null) {
    return Left ([show (x) + ' is not a member of ‘' + show (t) + '’']);
  }
  const missingFields = t.keys.filter (
    k => !(Object.prototype.propertyIsEnumerable.call (x, k))
  );
  if (missingFields.length > 0) {
    return Left (missingFields.map (k => show (k) + ' field is missing'));
  }
  for (const fieldName of t.keys) {
    const fieldType = t.types[fieldName];
    const fieldValue = x[fieldName];
    if (!(fieldType._test ([]) (fieldValue))) {
      return Left ([
        'Value of ' + show (fieldName) + ' field, ' + show (fieldValue) +
        ', is not a member of ‘' + show (fieldType) + '’',
      ]);
    }
  }
  return Right (x);
};

This works perfectly for none-nested types.

//    FooBar :: Type
const FooBar = $.NamedRecordType
  ('FooBar')
  ('')
  ([])
  ({foo: $.String,
    bar: $.Number});

eq (validate (FooBar) (null))
   (Left (['null is not a member of ‘FooBar’']));

eq (validate (FooBar) (undefined))
   (Left (['undefined is not a member of ‘FooBar’']));

eq (validate (FooBar) ({}))
   (Left (['"bar" field is missing', '"foo" field is missing']));

eq (validate (FooBar) ({foo: null}))
   (Left (['"bar" field is missing']));

eq (validate (FooBar) ({foo: null, bar: null}))
   (Left (['Value of "bar" field, null, is not a member of ‘Number’']));

eq (validate (FooBar) ({foo: null, bar: 42}))
   (Left (['Value of "foo" field, null, is not a member of ‘String’']));

eq (validate (FooBar) ({foo: 'blue', bar: 42}))
   (Right ({foo: 'blue', bar: 42}));

But this solution won't work for BarFoo.bar because the test is never run.

validate (BarFoo) ({foo: 'blue', bar: '2020-10-10', fez: 2})
// -> Right ({"bar": "2020-10-10", "fez": 2, "foo": "blue"})

validate (BarFoo) ({foo: 'blue', bar: '2020-29-10', fez: 2})
// -> Right ({"bar": "2020-29-10", "fez": 2, "foo": "blue"})

A wrong date for our very simple test is accepted.

I could add test for super types to validate:

//    validate :: Type -> a -> Either (Array String) a
const validate = t => x => {
  if (x == null) {
    return S.Left ([S.show (x) + ' is not a member of ‘' + S.show (t) + '’']);
  }
  const missingFields = t.keys.filter (
    k => !(Object.prototype.propertyIsEnumerable.call (x, k))
  );
  if (missingFields.length > 0) {
    return S.Left (missingFields.map (k => S.show (k) + ' field is missing'));
  }
  for (const fieldName of t.keys) {
    const fieldType = t.types[fieldName];
    const fieldValue = x[fieldName];
    if (!(fieldType._test (fieldType.supertypes) (fieldValue))) {
      return S.Left ([
        'Value of ' + S.show (fieldName) + ' field, ' + S.show (fieldValue) +
        ', is not a member of ‘' + S.show (fieldType) + '’',
      ]);
    }
  }
  return S.Right (x);
};

But the issue is not that validate didn't test super types since $.NonEmpty doesn't have a super type. The issue is that we do not call _test on nested types. E.g. BarFoo.bar.types.$1._test().

ERRATA: It turns out that fieldType._test (fieldType.supertypes) (fieldValue) is not how you test super types. I still do not know how to do that...

@dotnetCarpenter
Copy link

dotnetCarpenter commented Oct 4, 2021

Part IV

I believe the following change to make validate a recursive function should work, though not thoroughly tested.

//    validate :: Type -> a -> Either (Array String) a
const validate = t => x => {
  if (x == null) {
    return S.Left ([S.show (x) + ' is not a member of ‘' + S.show (t) + '’']);
  }
  const missingFields = t.keys.filter (
    k => !(Object.prototype.propertyIsEnumerable.call (x, k))
  );
  if (missingFields.length > 0) {
    return S.Left (missingFields.map (k => S.show (k) + ' field is missing from ' + S.show (t)));
  }
  for (const fieldName of t.keys) {
    const fieldType = t.types[fieldName];
    const fieldValue = x[fieldName];
    if (!(fieldType._test ($.env) (fieldValue))) {
      return S.Left ([
        'Value of ' + S.show (fieldName) + ' field, ' + S.show (fieldValue) +
        ', is not a member of ‘' + S.show (fieldType) + '’',
      ]);
    }
    if (fieldType.keys.length) {
      return validate (fieldType)
                      (S.foldMap (Object)
                                 (k => ({ [k]: x[fieldName] }))
                                 (fieldType.keys));
    }
  }
  return S.Right (x);
};

I realize that my version relies on Sanctuary which will create a cyclic dependency but if the idea works out, we can convert it to an Z (sanctuary-type-classes) implementation later.

validate (BarFoo) ({foo: 'blue', bar: '2020-10-10', fez: 2})
// -> Right ({"$1": "2020-10-10"})
// test ran with x = 2020-10-10

validate (BarFoo) ({foo: 'blue', bar: '2020-29-10', fez: 2})
// -> Left (["Value of \"$1\" field, \"2020-29-10\", is not a member of ‘DateIso’"])
// test ran with x = 2020-29-10

Unfortunately I'm running out of time and are AFK the following week, so won't have time to do the next obvious steps.

  1. Which is thoroughly test validate with different types and values.
  2. Change the return type Either (Array String) a to Either (Array Error) a to facilitate translation or accept a StrMap of translations.
  3. Implement validate in different projects and iterate over solutions.

But for sure there will be a part V and VI! ;)

@davidchambers What do you think?

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

No branches or pull requests

2 participants