`Value.Convert` to a `Union` no longer checks destination type validity

[colatkinson]

I believe this was introduced in version 0.32.16 (#791).

My uneducated guess as to the exact change is https://github.com/sinclairzx81/typebox/pull/791/files#diff-214384b10c038c24c4e088acf140367f701f11c4e1aee7573118fafcfe1d0e7bR242-R244.

For some context, my use case is to deserialize an object where all the values may be strings, convert them to their schema-specified type (if possible), and then use TypeCompiler.Compile().Check() to ensure validity.

I've written up a simple script that demonstrates the change in behavior:

import { Value } from "@sinclair/typebox/value"
import { Type } from "@sinclair/typebox"

const main = () => {
    const schema1 = Type.Union([ Type.String(), Type.Number({ minimum: 0 }) ])
    const schema2 = Type.Union([ Type.Number({ minimum: 0 }), Type.String() ])

    for (const val of ["1", "10", "asdf", "-1"]) {
        const res1 = Value.Convert(schema1, val)
        const res2 = Value.Convert(schema2, val)
        console.log(`${val}:`, res1, typeof res1, "/", res2, typeof res2)
    }
}

main()

Under the old behavior, conversion would greedily use the first valid Union member's type, whereas this is no longer the case as the reduce() goes through all entries in the anyOf. Thus the two reversed schemas -- this change is otherwise irrelevant.

Old output:

1: 1 string / 1 number
10: 10 string / 10 number
asdf: asdf string / asdf string
-1: -1 string / -1 string

New output:

1: 1 number / 1 string
10: 10 number / 10 string
asdf: asdf string / asdf string
-1: -1 number / -1 string

Note the final lines in both. With the old behavior, "-1" is left as a string regardless of ordering, since Check() fails due to the minimum. With the new behavior, it is, depending on ordering, converted to a number -- even though this isn't valid per the schema.

I'm of course uncertain how much of this is intended behavior vs accidents of implementation, but Convert() generating invalid data seems undesirable IMO. Of course if there's an alternative blessed approach to data conversion please let me know! And of course if there is any additional information I can provide, I'd be happy to do so.

[sinclairzx81]

@colatkinson Hi, thanks for reporting.

Yes, this looks like an oversight to me. The updates to Convert were intended to resolve a union/intersect (or distributive type) conversion issue that was submitted on https://github.com/sinclairzx81/typebox/issues/787. However the Union implementation was updated inline with the Intersect which applied a "best guess" conversion where it probably shouldn't have.

The update omitted the test for the "best possible" conversion (and early return) with the update attempting to rely a bit too much on the potential for Convert to return invalid values. I did mean to review this before publishing, but the update went through with some other changes on 0.32.16. Apologies.

I've published an update on 0.32.19 which reverts the Union logic specifically but retains the other conversion logic for 0.32.16. If you want to grab the latest off NPM and test things out your side, can close up this issue.

Cheers! S

[colatkinson]

Pulled the latest and it works like a charm. Thanks for the fast response! I'll close this out.