validateAll returns error when parseData returns std::nullopt

[bedrich-schindler]

We have following code (simplified for the purpose of example):

// ...
libyang::Context *context;
// ...

// ...
YangModule::YangModule() {
    this->context = new libyang::Context(std::nullopt, libyang::ContextOptions::SetPrivParsed);
}
// ...

/*
 * Validates the data tree against the schema.
 *
 * @param dataTreeJson The data tree to validate
 * @return `true` if the data tree is valid, `false` otherwise
 */
bool YangModule::validateData(const std::string &dataTreeJson) const {
    // Errors are automatically truncated every function call
    this->clearErrors();

    try {
        std::optional<libyang::DataNode> dataNode = this->context->parseDataMem(
            dataTreeJson,
            libyang::DataFormat::JSON,
            libyang::ParseOptions::ParseOnly,
            libyang::ValidationOptions::Present
        );

        validateAll(dataNode, libyang::ValidationOptions::Present);
    } catch (...) {
        return false;
    }

    // Errors are automatically truncated every function call
    return this->getErrors().empty();
}

If parseDataMem (respectively parseData in new version) returns std::nullopt instead of DataNode, and you pass it to the validateAll, libyang returns following error libyang[0]: Invalid argument tree (lyd_validate_all()).

I can provide example schema later, but the problem might be that std::optional<DataNode> Context::parseData returns std::nullopt when !tree. Colleague that use libyang (not libyang-cpp) says it works fine in C:

lyd_parse_data_mem(ctx, "{}", LYD_JSON, LYD_PARSE_ONLY, 0, &dnode);
// lyd_parse_data_mem(ctx, "", LYD_XML, LYD_PARSE_ONLY, 0, &dnode);
lyd_validate_all(&dnode, ctx, LYD_VALIDATE_PRESENT, NULL);

If he passes NULL instead of empty object, it the returns the same error as I mentioned before.

We think that C++ implementation is not correct.

[jktjkt]

Interesting. The main reason why the validateAll wrapper takes an optional<DataNode> & and not just a const DataNode & is that it might modify the node in-place, possibly even setting it to nullopt. The other reason is that a nullopt might be a valid "tree" on some models. The proper fix will probably be to extend validateAll to accept both an optional node, and a libyang context. Right now we're setting ctx to NULL and are relying on the C library to infer the context from the data tree, which it obviously cannot do if the tree is NULL as well.

However, the real point is that explicitly requesting validation like this should not be needed in the first place. If I read the C docs correctly, lyd_parse_XX performs validation already, so validateAll is intended to be called only when you're building your data trees programmatically.

[syyyr]

The solution in this specific case would probably be just removing the libyang::ParseOptions::ParseOnly flag.

Adding an optional Context argument is probably pretty easy yeah. Something like an std::optional<Context> argument to validateAll

[bedrich-schindler]

Thank you for quick response. We somehow ignored the fact that validateAll is not necessary. If I change our code from:

    try {
        std::optional<libyang::DataNode> dataNode = this->context->parseDataMem(
            dataTreeJson,
            libyang::DataFormat::JSON,
            libyang::ParseOptions::ParseOnly,
            libyang::ValidationOptions::Present
        );

        if (!dataNode.has_value()) {
            return false;
        }

        validateAll(dataNode, libyang::ValidationOptions::Present);
    } catch (...) {
        return false;
    }

to this:

    try {
        std::optional<libyang::DataNode> dataNode = this->context->parseDataMem(
            dataTreeJson,
            libyang::DataFormat::JSON,
            libyang::ParseOptions::Strict,
            libyang::ValidationOptions::Present
        );
    } catch (...) {
        return false;
    }

it almost works. I just have to change libyang::ParseOptions::ParseOnly to libyang::ParseOptions::Strict otherwise it does not validate anything. But with Strict options, it almost validates "empty data" as expected. I just need to figure out why the catch does not catch error as in previous case, whole code crashes instead and error is outputted in the standard output instead - before, I called parseData, validateAll and I found those error messages with getErrors function. But it might be problem in our WebAssemly code.

However, should't the mentioned problem be fixed? Empty data are still valid data, so if parseData returns std::nullopt, validateAll should be able it (if C code is able to do it), should't it?