False validation errors with the yaml-cpp adapter

[jacquev6]

Hello and thank you for this library!

I get false validation errors for schemas with a type constraint for boolean, number and integer when using the yaml-cpp adapter. See code below to reproduce the issue.

I believe this is because the isBool, isDouble, isInteger, isNumber and isString methods in yaml_cpp_adapter.hpp return literal trues and falses.

Code to reproduce the issue:

#include <valijson/adapters/yaml_cpp_adapter.hpp>
#include <valijson/schema_parser.hpp>
#include <valijson/validator.hpp>

void validate(const std::string& schema_string, const std::string& document_string) {
  std::istringstream schema_iss(schema_string);
  YAML::Node schema_node = YAML::Load(schema_iss);
  valijson::SchemaParser schema_parser;
  valijson::adapters::YamlCppAdapter schema_adapter(schema_node);
  valijson::Schema schema;
  schema_parser.populateSchema(schema_adapter, schema);

  std::istringstream document_iss(document_string);
  YAML::Node document_node = YAML::Load(document_iss);
  valijson::adapters::YamlCppAdapter document_adapter(document_node);

  valijson::Validator validator;
  valijson::ValidationResults results;
  if (validator.validate(schema, document_adapter, &results)) {
    std::cout << "OK";
  } else {
    std::cout << "VALIDATION ERROR:";
    valijson::ValidationResults::Error error;
    while (results.popError(error)) {
      std::cout << "\n -";
      for (const std::string &contextElement : error.context) {
          std::cout << " " << contextElement;
      }
      std::cout << ": " << error.description;
    }
  }
  std::cout << std::endl;
}

int main() {
  // OK
  validate("type: string", "\"bonjour\"");
  validate("type: array", "[]");
  validate("type: object", "{}");
  // <root>: Value type not permitted by 'type' constraint.
  validate("type: boolean", "true");
  validate("type: integer", "42");
  validate("type: number", "49.3");
}

Thank you!

[jacquev6]

Maybe you could try the conversion and catch the BadConversion but I'm not familiar enough with either yaml-ccp or valijson to be sure that's the right way. Something like:

    bool isInteger() const
    {
        try {
            m_value.as<int64_t>();
            return true;
        } catch (const YAML::BadConversion&) {
            return false;
        }
    }

[sergiorussia]

@tristanpenman hi. thanks again for the library, great work. we've hit this bug too (yaml > valijson), maybe u could take a look again? there's a chance you simply lost it in a daily fuss :) there's linked w/a too, maybe it's worth trying/looking at?

[tristanpenman]

Hi @sergiorussia thanks for the reminder on this one.

I'll play around with the YamlCpp adapter using the work around you've linked.

I have an idea that might help in the mean time. When adding support for Boost Property Trees, I ran into a similar issue where all scalar keys are treated as strings. To address that, I added support for 'weak typing' in the Validator class. You can enable this at construction time:

valijson::Validator validator(valijson::kWeakTypes);

Let me know if this helps at all.

[sergiorussia]

@tristanpenman yeah, it worked in our case. what about performance? i assume it will be ok but as we use validation for every incoming request / client response it would be great to know what option is better - weak typing or possible future w/a (mentioned or your version, whatever)

[tristanpenman]

@tristanpenman yeah, it worked in our case. what about performance? i assume it will be ok but as we use validation for every incoming request / client response it would be great to know what option is better - weak typing or possible future w/a (mentioned or your version, whatever)

Performance should be similar in most cases, as the adapter base class uses functionality in the standard library to attempt any type conversions. This seems to be what yamlcpp does under the hood too.

For context, here's the code that checks whether a value validates as an integer using weak typing:

    bool maybeInteger() const override
    {
        if (m_value.isInteger()) {
            return true;
        } else if (maybeString()) {
            std::string s;
            if (m_value.getString(s)) {
                std::istringstream i(s);
                int64_t x;
                char c;
                if (!(i >> x) || i.get(c)) {
                    return false;
                }
                return true;
            }
        }

        return false;
    }

[sergiorussia]

unfortunately team says that weak types could hide potential bugs with types in future and insist on using strict types.

we use valijson to validate incoming request over (yaml) openapi spec, so with weak types it is possible that validation over eg "boolean" "true" will pass but consequent actual code could fail.

here's simplified failing spec example:

SomeSchema:
  type: object
  properties:
    someEnum:
      type: integer
      enum:
        - 1
        - 2

in this particular case valijson with yaml adapter rejects valid json like {"someEnum":1} as it treats enumerators as strings (however it seems easy pass "type" info here https://github.com/tristanpenman/valijson/blob/master/include/valijson/schema_parser.hpp#L702-L704 nah, seems the proper way is to fix yaml adapter)

tl;dr: weak types w/a works but too permissive for now, it's better to fix yaml adapter. seems that suggested w/a is really just a temp w/a. better way would be to track nodes "type" in adapter and then cast scalar schema values to that type