ClassCompiler: Validate field types -> Icinga type names correctly

[yhabteab]

When the classcompiler is validating/transforming field types -> Icinga type names, it is currently returning
Icinga Number type for field type of bool, which is actually wrong. This PR ensures to always transform
into the correct Icinga type names.

[julianbrost]

There are two uses of FieldTypeToIcingaName:

1. icinga2/tools/mkclass/classcompiler.cpp

   Lines 313 to 358 in 3a8abdc

   	/* GetFieldInfo */
   	m_Header << "\t" << "Field GetFieldInfo(int id) const override;" << std::endl;
   	m_Impl << "Field TypeImpl<" << klass.Name << ">::GetFieldInfo(int id) const" << std::endl
   	<< "{" << std::endl;
   	if (!klass.Parent.empty())
   	m_Impl << "\t" << "int real_id = id - " << klass.Parent << "::TypeInstance->GetFieldCount();" << std::endl
   	<< "\t" << "if (real_id < 0) { return " << klass.Parent << "::TypeInstance->GetFieldInfo(id); }" << std::endl;
   	if (!klass.Fields.empty()) {
   	m_Impl << "\t" << "switch (";
   	if (!klass.Parent.empty())
   	m_Impl << "real_id";
   	else
   	m_Impl << "id";
   	m_Impl << ") {" << std::endl;
   	size_t num = 0;
   	for (const Field& field : klass.Fields) {
   	std::string ftype = FieldTypeToIcingaName(field, false);
   	std::string nameref;
   	if (field.Type.IsName)
   	nameref = "\"" + field.Type.TypeName + "\"";
   	else
   	nameref = "nullptr";
   	m_Impl << "\t\t" << "case " << num << ":" << std::endl
   	<< "\t\t\t" << "return {" << num << ", \"" << ftype << "\", \"" << field.Name << "\", \"" << (field.NavigationName.empty() ? field.Name : field.NavigationName) << "\", " << nameref << ", " << field.Attributes << ", " << field.Type.ArrayRank << "};" << std::endl;
   	num++;
   	}
   	m_Impl << "\t\t" << "default:" << std::endl
   	<< "\t\t";
   	}
   	m_Impl << "\t" << "throw std::runtime_error(\"Invalid field ID.\");" << std::endl;
   	if (!klass.Fields.empty())
   	m_Impl << "\t" << "}" << std::endl;
   	m_Impl << "}" << std::endl << std::endl;

   
   This affects the GetFieldInfo function in the generated code. This one has a few users, but the change only affects the value of the TypeName member, which has fewer users (git grep '\bTypeName\b', excluding matches in tools/mkclass/ which use a different TypeName than the one in use in code generated by mkclass):

   1. icinga2/lib/base/object.cpp

      Lines 140 to 151 in 3a8abdc

      	try {
      	SetField(fid, value);
      	} catch (const boost::bad_lexical_cast&) {
      	Field fieldInfo = type->GetFieldInfo(fid);
      	Type::Ptr ftype = Type::GetByName(fieldInfo.TypeName);
      	BOOST_THROW_EXCEPTION(ScriptError("Attribute '" + field + "' cannot be set to value of type '" + value.GetTypeName() + "', expected '" + ftype->GetName() + "'", debugInfo));
      	} catch (const std::bad_cast&) {
      	Field fieldInfo = type->GetFieldInfo(fid);
      	Type::Ptr ftype = Type::GetByName(fieldInfo.TypeName);
      	BOOST_THROW_EXCEPTION(ScriptError("Attribute '" + field + "' cannot be set to value of type '" + value.GetTypeName() + "', expected '" + ftype->GetName() + "'", debugInfo));
      	}
      	}

      
      Used in an error message, so probably even better to use the proper type name.

   2. icinga2/lib/cli/clicommand.cpp

      Lines 63 to 65 in 3a8abdc

      	if (strcmp(field.TypeName, "int") != 0 && strcmp(field.TypeName, "double") != 0
      	&& strcmp(field.TypeName, "bool") != 0 && strcmp(field.TypeName, "String") != 0)
      	continue;

      
      The whole functions seems to be unused but doesn't check for "Number"/"Boolean" anyway.

   3. icinga2/lib/icinga/macroprocessor.cpp

      Line 148 in 3a8abdc

      if (strcmp(fieldInfo.TypeName, "Timestamp") == 0)

      
      Only checks for "Timestamp", so also unaffected by the change.

   4. icinga2/lib/remote/typequeryhandler.cpp

      Line 129 in 3a8abdc

      fieldInfo->Set("type", field.TypeName);

      
      Changes the returned type information in /v1/types, probably also better to include the correct type name.

2. icinga2/tools/mkclass/classcompiler.cpp

   Lines 509 to 553 in 3a8abdc

   	std::string ftype = FieldTypeToIcingaName(field, true);
   	if (ftype == "Value") {
   	m_Impl << "\t" << "if (" << valName << ".IsObjectType<Function>()) {" << std::endl
   	<< "\t\t" << "Function::Ptr func = " << valName << ";" << std::endl
   	<< "\t\t" << "if (func->IsDeprecated())" << std::endl
   	<< "\t\t\t" << "Log(LogWarning, \"" << klass.Name << "\") << \"Attribute '" << field.Name << R"(' for object '" << dynamic_cast<ConfigObject *>(this)->GetName() << "' of type '" << dynamic_cast<ConfigObject *>(this)->GetReflectionType()->GetName() << "' is set to a deprecated function: " << func->GetName();)" << std::endl
   	<< "\t" << "}" << std::endl << std::endl;
   	}
   	if (field.Type.IsName) {
   	if (field.Type.ArrayRank > 0) {
   	m_Impl << "\t\t\t" << "if (!" << valName << ".IsEmpty() && ";
   	m_Impl << "!" << valName << ".IsString())" << std::endl
   	<< "\t\t\t\t" << "BOOST_THROW_EXCEPTION(ValidationError(dynamic_cast<ConfigObject *>(this), { \""
   	<< field.Name << R"(" }, "It is not allowed to specify '" + )" << valName << R"( + "' of type '" + )"
   	<< valName << ".GetTypeName()" << R"( + "' as ')" << field.Type.TypeName
   	<< "' name. Expected type of '" << field.Type.TypeName << "' name: 'String'.\"));" << std::endl;
   	}
   	m_Impl << "\t\t\t" << "if (";
   	if (field.Type.ArrayRank > 0)
   	m_Impl << valName << ".IsEmpty() || ";
   	else
   	m_Impl << "!" << valName << ".IsEmpty() && ";
   	m_Impl << "!utils.ValidateName(\"" << field.Type.TypeName << "\", " << valName << "))" << std::endl
   	<< "\t\t\t\t" << "BOOST_THROW_EXCEPTION(ValidationError(dynamic_cast<ConfigObject *>(this), { \"" << field.Name << R"(" }, "Object '" + )" << valName << R"( + "' of type ')" << field.Type.TypeName
   	<< "' does not exist.\"));" << std::endl;
   	} else if (field.Type.ArrayRank > 0 && (ftype == "Number" || ftype == "Boolean")) {
   	m_Impl << "\t" << "try {" << std::endl
   	<< "\t\t" << "Convert::ToDouble(" << valName << ");" << std::endl
   	<< "\t" << "} catch (const std::invalid_argument&) {" << std::endl
   	<< "\t\t" << "BOOST_THROW_EXCEPTION(ValidationError(dynamic_cast<ConfigObject *>(this), { \"" << field.Name << R"(", "Array element '" + " << valName << " + "' of type '" + " << valName << ".GetReflectionType()->GetName() + "' is not valid here; expected type ')" << ftype << "'.\"));" << std::endl
   	<< "\t" << "}" << std::endl;
   	}
   	if (field.Type.ArrayRank > 0) {
   	m_Impl << "\t\t" << "}" << std::endl
   	<< "\t" << "}" << std::endl;
   	}
   	m_Impl << "}" << std::endl << std::endl;

   
   The following is the only case where returning "Boolean" or "Number" would make a difference:
   

   icinga2/tools/mkclass/classcompiler.cpp

   Line 540 in 3a8abdc

   } else if (field.Type.ArrayRank > 0 && (ftype == "Number" || ftype == "Boolean")) {

   
   But this is dead code at the moment anyways, i.e. no *.ti file defines an array of Number/Boolean anyways, so you can even put abort() into that branch and things still compile. Thus, no change in behavior here.

[julianbrost]

Observable changes by this PR (highlighted in bold in my previous comment) are totally fine (even an improvement), so let's do this.