Bad error reporting for value objects

[marcospassos]

In our code base, we have several value objects for representing IDs, like WorkspaceId, OrganizationId, etc. When mapping a value where such ids were expected, Valinor yields poor unuseful messages like:

Cannot be empty and must be filled with a value matching type ?.

I understand that exposing the class name isn't a good idea for security reasons. However, currently, there is no way to handle such cases:

• The originally expected type isn't retained, so we can't just inspect the parameters
• All messages are flagged as @internal, so we can't rely on it to check if it's one of these cases to override the message
• There's no feature that allows providing labels for such type so Valinor can take care of it

I think Valinor should take advantage of the ICU select construct to remove those confusing parts:

{expected_type, select,
  `?` {Cannot be empty}
   other {Cannot be empty and must match type {expected_type}}
}

The docs suggest looking at the message codes, but comparing code with random integers like 1655449641 leads to a bad DX in terms of readability and maintainability. I'd rather prefer relying on instanceof checks or ultimately in constants.

[romm]

Hi @marcospassos, thanks for the report and the suggestion.

I've been thinking about a solution to avoid using ? in the message body but didn't come to something yet. When I get some time I'll work on this again.

In the meantime, would it be ok for you to use a custom formatter that could look like this:

final class TypeSanitizerFormatter implements MessageFormatter
{
    private const MAP = [
        WorkspaceId::class => 'string',
        OrganizationId::class => 'string',
    ];

    public function format(NodeMessage $message): NodeMessage
    {
        $nodeType = $message->node()->type();

        if (isset(self::MAP[$nodeType])) {
            $message = $message->withParameter('expected_type', self::MAP[$nodeType]);
        }

        return $message;
    }
}

Please keep me updated. 🙂

[marcospassos]

Just an update here: I'm afraid the rabbit hole is a bit deeper.

That's what we're using right now to achieve what we need:

try {
    return $mapper->map($type, $source);
} catch (MappingError $exception) {
    $flattener = new MessagesFlattener($exception->node());
    $errors = $flattener->errors();
    $iterator = $errors->getIterator();
    /** @var NodeMessage $message */
    $message = $iterator->current();

    throw new MappingException(
        self::formatError($message)->toString(),
        $message->node()->path(),
        $exception,
    );
}

// ...

private static function formatError(NodeMessage $message): NodeMessage
{
    return match ($message->code()) {
        // CuyZ\Valinor\Mapper\Tree\Exception\MissingNodeValue
        '1655449641' => $message
            ->withBody('Missing value at "{node_path}".')
            ->withParameter('node_path', $message->node()->path()),
        // CuyZ\Valinor\Mapper\Tree\Exception\InvalidNodeValue
        '1630946163',
        // CuyZ\Valinor\Mapper\Tree\Exception\InvalidTraversableKey
        '1618736242',
        // CuyZ\Valinor\Mapper\Tree\Exception\ShapedArrayElementMissing
        '1631613641',
        // CuyZ\Valinor\Mapper\Tree\Exception\SourceMustBeIterable
        '1618739163' => $message
            ->withBody('Invalid value at "{node_path}".')
            ->withParameter('node_path', $message->node()->path()),
        // CuyZ\Valinor\Mapper\Tree\Exception\ValueNotAcceptedByScalarType
        '1655030601' => $message
            ->withBody('Invalid value at "{node_path}".')
            ->withParameter('node_path', $message->node()->path()),
        default => $message,
    };
}

There's room for DX improvement here, IMHO.

[marcospassos]

@romm, do you see any more maintainable way of achieving the same result?

[romm]

Hi @marcospassos sorry for the late answer.

Could you take a look at #244? It introduces new helpers that would help you achieve what you want in a better way:

try {
    return $mapper->map($type, $source);
} catch (MappingError $exception) {
    $errors = Messages::flattenFromNode($error->node())
        ->errors()
        ->formatWith(new CallbackMessageFormatter(
            fn (NodeMessage $message) => match ($message->code()) {
                // CuyZ\Valinor\Mapper\Tree\Exception\MissingNodeValue
                '1655449641' => $message->withBody('Missing value at "{node_path}".'),
                // CuyZ\Valinor\Mapper\Tree\Exception\InvalidNodeValue
                '1630946163',
                // CuyZ\Valinor\Mapper\Tree\Exception\InvalidTraversableKey
                '1618736242',
                // CuyZ\Valinor\Mapper\Tree\Exception\ShapedArrayElementMissing
                '1631613641',
                // CuyZ\Valinor\Mapper\Tree\Exception\SourceMustBeIterable
                '1618739163',
                // CuyZ\Valinor\Mapper\Tree\Exception\ValueNotAcceptedByScalarType
                '1655030601' => $message->withBody('Invalid value at "{node_path}".'),
                default => $message,
            }
        ));

    // …
}

WDYT?

[marcospassos]

That's a bit better, but the main problem relies on obscure codes.

[romm]

I see. I'll give it some thought, maybe use more explicit codes, like invalid_node_value or something like that.

I'll keep you updated here.

[marcospassos]

Enums or constants are the way to go IMHO