Closed jabuj closed 7 months ago
@ialexi-bl Hi, thanks for reporting
I'm probably going to leave this functionality "as is" as this issue is caused by the additionalProperties: false
constraint being applied to the object type (meaning the union variant fails the prerequisite check (no matching variant) for the given value). If you remove the additionalProperties: false
constraint, calling Clean
will remove these excess properties.
Note that the
Clean
andDefault
functions may return invalid values (and both returnunknown
for this reason) and there is a requirement toCheck
the value before using it. In terms of usage, you should implement Clean with the expectation it will fail, and handle failure cases correctly with Check.
In terms of providing a fix here, auto removing the additionalProperties: false
constraint for object types per variant check would be very expensive (requiring a full clone at a minimum of the type), and not extend well to intersection and other structured types with constraints (from which there may be other caveats to consider). The current implementation tries to keep the Clean algorithm simple and fast, but does make trade offs in places, and doesn't hide some of the issues caused by schema constraints.
The following implements logic to recursively discard the additionalProperties false constraint and implements a custom Clean function.
import { Value, IsObject, IsArray } from '@sinclair/typebox/value'
import { TSchema, Type } from '@sinclair/typebox'
// ----------------------------------------------------------------------------
// Discard (Recursively Discards Properties)
// ----------------------------------------------------------------------------
export function DiscardKey(value: unknown, key: PropertyKey): unknown {
if(!IsObject(value) || IsArray(value)) return value
const { [key]: _, ...rest } = value
return rest
}
export function DiscardKeys(value: unknown, keys: PropertyKey[]) {
return keys.reduce((acc, key) => DiscardKey(acc, key), value)
}
export function DiscardKeysRecursive<T>(value: T, keys: PropertyKey[]): T {
const discarded = DiscardKeys(value, keys)
return (
IsArray(discarded) ? discarded.map(value => DiscardKeysRecursive(value, keys)) :
IsObject(discarded) ? {
...Object.getOwnPropertyNames(discarded).reduce((acc, key) => ({ ...acc, [key]: DiscardKeysRecursive(discarded[key], keys) }), {}),
...Object.getOwnPropertySymbols(discarded).reduce((acc, key) => ({ ...acc, [key]: DiscardKeysRecursive(discarded[key], keys) }), {})
} : discarded
) as never
}
// ----------------------------------------------------------------------------
// Clean (Custom Clean Pipeline)
// ----------------------------------------------------------------------------
export function Clean(schema: TSchema, value: unknown) {
const discarded = DiscardKeysRecursive(schema, ['additionalProperties'])
return Value.Clean(discarded, value)
}
// ----------------------------------------------------------------------------
// Usage
// ----------------------------------------------------------------------------
const T = Type.Union([
Type.Null(),
Type.Object({ x: Type.Number() }, { additionalProperties: false }),
])
const encoded = Clean(T, { x: 1, y: 2 })
console.log(encoded) // { x: 1 }
So the above modifies the schematics prior to calling Clean. It's not ideal, but it is inline with the semantics of how Clean works. You may be interested in the Cast
function however which is loosely the inverse of Clean and will optionally remove properties if the additionalProperties constraint if applied.
import { Value } from '@sinclair/typebox/value'
import { Type } from '@sinclair/typebox'
const T = Type.Union([
Type.Null(),
Type.Object({ x: Type.Number() }, { additionalProperties: false }),
])
const encoded = Value.Cast(T, { x: 1, y: 2 })
console.log(encoded) // { x: 1 }
Will close off this issue for now as things are working as per current design. Hope this helps S
@sinclairzx81, thank you for a quick response. Another idea before leaving this issue: maybe instead of removing additionalProperties: false
constraint recursively, adding some kind of an internal flag to Check
which would ignore additionalProperties
for such cases would be an option?
@ialexi-bl Heya
Another idea before leaving this issue: maybe instead of removing additionalProperties: false constraint recursively, adding some kind of an internal flag to Check which would ignore additionalProperties for such cases would be an option?
The problem is that Clean calls Check internally for Union determination (and Check works entirely off the schematics). So there isn't really an option to conditionally ignore the constraint (The Check function would need to be reimplemented). Also, I'm very hesitant to "mix and match" assertion logic based on additional configuration criteria (i.e. ignoreConstraint
) as the criteria for assertion is configured (encoded) by way of the type itself....this is just a hard rule in TypeBox to tame complexity.
I'll give this some thought, and do agree that excess properties should be omitted irrespective of the constraint, but to achieve, it would require a overhaul of TB's internal logic for union variant matching. The Cast function currently uses a probabilistic match to do union determination (as best as it can), a similar technique may be applicable for Clean...
For now, explicitly omitting the constraint from the type is the recommendation. Hope this brings a bit more insight S
Repro:
Sandbox link
The reason appears to be this line: the call to
Check
fails because of additional properties, so they are not removed. I'm not able to submit a fix PR for now, though, unfortunately.