Skip to content

Commit 3318581

Browse files
hodoulphodoulp
authored
Adsk Contrib - Improve the context variable validation (#1387) (#1395)
Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>
1 parent 7067004 commit 3318581

3 files changed

Lines changed: 171 additions & 32 deletions

File tree

‎src/OpenColorIO/Config.cpp‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1300,21 +1300,35 @@ void Config::validate() const
13001300

13011301
// Only the 'predefined' mode imposes to have all the context variables explicitely defined
13021302
// in the config file. The 'all' mode exclusively relies on the environment variables.
1303-
if (getImpl()->m_context->getEnvironmentMode() == ENV_ENVIRONMENT_LOAD_PREDEFINED)
1303+
if (getMajorVersion() >= 2
1304+
&& getImpl()->m_context->getEnvironmentMode() == ENV_ENVIRONMENT_LOAD_PREDEFINED)
13041305
{
13051306
for (const auto & env : getImpl()->m_env)
13061307
{
1307-
const std::string ctxVariable = std::string("$") + env.first;
1308-
const std::string ctxValue
1309-
= getImpl()->m_context->resolveStringVar(ctxVariable.c_str());
1308+
const std::string ctxValue = env.second;
13101309

13111310
if (ContainsContextVariables(ctxValue))
13121311
{
1313-
std::ostringstream oss;
1314-
oss << "Unresolved context variable '"
1315-
<< env.first << " = "
1316-
<< env.second << "'.";
1317-
throw Exception(oss.str().c_str());
1312+
// When a context variable default value contains another context variable, the
1313+
// only legal case is ENV = $ENV. It means that there is no default value i.e. an
1314+
// system env. variable must exist.
1315+
1316+
const std::string ctxVariable1 = std::string("$") + env.first;
1317+
const std::string ctxVariable2 = std::string("${") + env.first + std::string("}");
1318+
const std::string ctxVariable3 = std::string("%") + env.first + std::string("%");
1319+
1320+
const bool isValid = (ctxVariable1 == ctxValue)
1321+
|| (ctxVariable2 == ctxValue)
1322+
|| (ctxVariable3 == ctxValue);
1323+
1324+
if (!isValid)
1325+
{
1326+
std::ostringstream oss;
1327+
oss << "Unresolved context variable in environment declaration '"
1328+
<< env.first << " = "
1329+
<< env.second << "'.";
1330+
throw Exception(oss.str().c_str());
1331+
}
13181332
}
13191333
}
13201334
}

‎src/OpenColorIO/apphelpers/ColorSpaceHelpers.cpp‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// SPDX-License-Identifier: BSD-3-Clause
22
// Copyright Contributors to the OpenColorIO Project.
33

4+
45
#include <cstring>
56
#include <map>
67
#include <mutex>
@@ -10,9 +11,11 @@
1011
#include <OpenColorIO/OpenColorIO.h>
1112

1213
#include "ColorSpaceHelpers.h"
14+
#include "Logging.h"
1315
#include "Platform.h"
1416
#include "utils/StringUtils.h"
1517

18+
1619
namespace OCIO_NAMESPACE
1720
{
1821

@@ -374,8 +377,8 @@ ColorSpaceMenuHelperRcPtr ColorSpaceMenuHelper::Create(ConstColorSpaceMenuParame
374377
catch (Exception & e)
375378
{
376379
std::ostringstream oss;
377-
oss << "ColorSpaceMenuHelper needs a valid config. Validation failed with: " << e.what();
378-
throw Exception(oss.str().c_str());
380+
oss << "ColorSpaceMenuHelper needs a valid config. Validation warning is: " << e.what();
381+
LogWarning(oss.str().c_str());
379382
}
380383

381384
if (!p->getIncludeColorSpaces() && p->getIncludeRoles())

‎tests/cpu/Config_tests.cpp‎

Lines changed: 143 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -840,9 +840,6 @@ OCIO_ADD_TEST(Config, context_variable)
840840
" VAR1: $VAR1\n" // No default value so the env. variable must exist.
841841
" VAR2: var2\n" // Default value if env. variable does not exist.
842842
" VAR3: env3\n" // Same as above.
843-
" VAR4: env4$VAR1\n" // Same as above and built from another envvar.
844-
" VAR5: env5$VAR2\n" // Same as above and built from another envvar.
845-
" VAR6: env6$VAR3\n" // Same as above and built from another envvar.
846843
"search_path: luts\n"
847844
"strictparsing: true\n"
848845
"luma: [0.2126, 0.7152, 0.0722]\n"
@@ -895,10 +892,6 @@ OCIO_ADD_TEST(Config, context_variable)
895892
OCIO_CHECK_EQUAL(std::string("env2"), config->getCurrentContext()->resolveStringVar("$VAR2"));
896893
OCIO_CHECK_EQUAL(std::string("env3"), config->getCurrentContext()->resolveStringVar("$VAR3"));
897894

898-
OCIO_CHECK_EQUAL(std::string("env4env1"), config->getCurrentContext()->resolveStringVar("$VAR4"));
899-
OCIO_CHECK_EQUAL(std::string("env5env2"), config->getCurrentContext()->resolveStringVar("$VAR5"));
900-
OCIO_CHECK_EQUAL(std::string("env6env3"), config->getCurrentContext()->resolveStringVar("$VAR6"));
901-
902895
std::ostringstream oss;
903896
OCIO_CHECK_NO_THROW(oss << *config.get());
904897
OCIO_CHECK_EQUAL(oss.str(), iss.str());
@@ -910,14 +903,146 @@ OCIO_ADD_TEST(Config, context_variable)
910903
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(iss));
911904
OCIO_CHECK_NO_THROW(config->validate());
912905

913-
// Test a faulty case i.e. the env. variable VAR1 is now missing.
906+
OCIO_CHECK_EQUAL(std::string("env1"), config->getCurrentContext()->resolveStringVar("$VAR1"));
907+
OCIO_CHECK_EQUAL(std::string("var2"), config->getCurrentContext()->resolveStringVar("$VAR2"));
908+
OCIO_CHECK_EQUAL(std::string("env3"), config->getCurrentContext()->resolveStringVar("$VAR3"));
909+
910+
// System env. variable VAR1 is now missing but the context variable VAR1 is still a valid one.
914911

915912
OCIO::Platform::Unsetenv("VAR1");
916913
iss.str(CONFIG);
917914
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(iss));
918-
OCIO_CHECK_THROW_WHAT(config->validate(),
919-
OCIO::Exception,
920-
"Unresolved context variable 'VAR1 = $VAR1'.");
915+
OCIO_CHECK_NO_THROW(config->validate());
916+
917+
OCIO_CHECK_EQUAL(std::string("$VAR1"), config->getCurrentContext()->resolveStringVar("$VAR1"));
918+
OCIO_CHECK_EQUAL(std::string("var2"), config->getCurrentContext()->resolveStringVar("$VAR2"));
919+
OCIO_CHECK_EQUAL(std::string("env3"), config->getCurrentContext()->resolveStringVar("$VAR3"));
920+
}
921+
922+
OCIO_ADD_TEST(Config, context_variable_unresolved)
923+
{
924+
// Test for invalid unresolved context variables.
925+
926+
static const std::string BODY_CONFIG =
927+
"\n"
928+
"search_path: luts\n"
929+
"\n"
930+
"roles:\n"
931+
" default: cs1\n"
932+
" reference: cs1\n"
933+
"\n"
934+
"displays:\n"
935+
" disp1:\n"
936+
" - !<View> {name: view1, colorspace: cs2}\n"
937+
"\n"
938+
"colorspaces:\n"
939+
" - !<ColorSpace>\n"
940+
" name: cs1\n"
941+
"\n"
942+
" - !<ColorSpace>\n"
943+
" name: cs2\n"
944+
" from_reference: !<MatrixTransform> {offset: [0.1, 0.2, 0.3, 0.0]}\n";
945+
946+
{
947+
static const std::string CONFIG =
948+
std::string("ocio_profile_version: 2\nenvironment: {ENV1: $ENV1}\n")
949+
+ BODY_CONFIG;
950+
951+
std::istringstream iss;
952+
iss.str(CONFIG);
953+
954+
OCIO::ConstConfigRcPtr config;
955+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(iss));
956+
OCIO_CHECK_NO_THROW(config->validate());
957+
}
958+
959+
{
960+
static const std::string CONFIG =
961+
std::string("ocio_profile_version: 2\nenvironment:\n ENV1: ${ENV1}\n")
962+
+ BODY_CONFIG;
963+
964+
std::istringstream iss;
965+
iss.str(CONFIG);
966+
967+
OCIO::ConstConfigRcPtr config;
968+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(iss));
969+
OCIO_CHECK_NO_THROW(config->validate());
970+
}
971+
972+
{
973+
static const std::string CONFIG =
974+
std::string("ocio_profile_version: 2\nenvironment: {ENV1: $ENV2}\n")
975+
+ BODY_CONFIG;
976+
977+
std::istringstream iss;
978+
iss.str(CONFIG);
979+
980+
OCIO::ConstConfigRcPtr config;
981+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(iss));
982+
OCIO_CHECK_THROW_WHAT(config->validate(),
983+
OCIO::Exception,
984+
"Unresolved context variable in environment declaration 'ENV1 = $ENV2'.");
985+
}
986+
987+
{
988+
static const std::string CONFIG =
989+
std::string("ocio_profile_version: 2\nenvironment: {ENV1: env, ENV2: $ENV1}\n")
990+
+ BODY_CONFIG;
991+
992+
std::istringstream iss;
993+
iss.str(CONFIG);
994+
995+
OCIO::ConstConfigRcPtr config;
996+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(iss));
997+
OCIO_CHECK_THROW_WHAT(config->validate(),
998+
OCIO::Exception,
999+
"Unresolved context variable in environment declaration 'ENV2 = $ENV1'.");
1000+
}
1001+
1002+
{
1003+
static const std::string CONFIG =
1004+
std::string("ocio_profile_version: 2\nenvironment: {ENV1: env$ENV1}\n")
1005+
+ BODY_CONFIG;
1006+
1007+
std::istringstream iss;
1008+
iss.str(CONFIG);
1009+
1010+
OCIO::ConstConfigRcPtr config;
1011+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(iss));
1012+
OCIO_CHECK_THROW_WHAT(config->validate(),
1013+
OCIO::Exception,
1014+
"Unresolved context variable in environment declaration 'ENV1 = env$ENV1'.");
1015+
}
1016+
1017+
{
1018+
static const std::string CONFIG =
1019+
std::string("ocio_profile_version: 2\nenvironment:\n ENV1: env${ENV2}\n")
1020+
+ BODY_CONFIG;
1021+
1022+
std::istringstream iss;
1023+
iss.str(CONFIG);
1024+
1025+
OCIO::ConstConfigRcPtr config;
1026+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(iss));
1027+
OCIO_CHECK_THROW_WHAT(config->validate(),
1028+
OCIO::Exception,
1029+
"Unresolved context variable in environment declaration 'ENV1 = env${ENV2}'.");
1030+
}
1031+
1032+
{
1033+
static const std::string CONFIG =
1034+
std::string("ocio_profile_version: 2\nenvironment: {ENV1: $ENV1$ENV2}\n")
1035+
+ BODY_CONFIG;
1036+
1037+
std::istringstream iss;
1038+
iss.str(CONFIG);
1039+
1040+
OCIO::ConstConfigRcPtr config;
1041+
OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(iss));
1042+
OCIO_CHECK_THROW_WHAT(config->validate(),
1043+
OCIO::Exception,
1044+
"Unresolved context variable in environment declaration 'ENV1 = $ENV1$ENV2'.");
1045+
}
9211046
}
9221047

9231048
OCIO_ADD_TEST(Config, context_variable_with_sanity_check)
@@ -994,26 +1119,23 @@ OCIO_ADD_TEST(Config, context_variable_with_sanity_check)
9941119
}
9951120

9961121
{
997-
// Update $CS2 to use $TOTO. That's still a self-contained context because
998-
// $TOTO exists.
1122+
// Update $CS2 to use $TOTO. That's an invalid case because a context variable value can
1123+
// only be a string (i.e. "VAR: env") or the context variable itself (i.e. "VAR: $VAR").
9991124
OCIO_CHECK_NO_THROW(cfg->addEnvironmentVar("CS2", "$TOTO"));
10001125
OCIO_CHECK_EQUAL(cfg->getNumEnvironmentVars(), 2);
1001-
OCIO_CHECK_NO_THROW(cfg->validate());
1002-
1003-
// Note that the default value of the context variable is unresolved.
1004-
OCIO_CHECK_EQUAL(std::string(cfg->getEnvironmentVarDefault("CS2")), std::string("$TOTO"));
1126+
OCIO_CHECK_THROW_WHAT(cfg->validate(),
1127+
OCIO::Exception,
1128+
"Unresolved context variable in environment declaration 'CS2 = $TOTO'.");
10051129
}
10061130

10071131
{
1008-
// Remove $TOTO from the context. That's a faulty case because $CS2 is still used
1009-
// but resolved using $TOTO so, the environment is not self-contained. Sanity check
1010-
// must throw in that case.
1132+
// Remove $TOTO from the context. That's an invalid case as explained above.
10111133
OCIO_CHECK_NO_THROW(cfg->addEnvironmentVar("TOTO", nullptr));
10121134
OCIO_CHECK_EQUAL(cfg->getNumEnvironmentVars(), 1);
10131135

10141136
OCIO_CHECK_THROW_WHAT(cfg->validate(),
10151137
OCIO::Exception,
1016-
"Unresolved context variable 'CS2 = $TOTO'.");
1138+
"Unresolved context variable in environment declaration 'CS2 = $TOTO'.");
10171139
OCIO_CHECK_THROW_WHAT(cfg->getProcessor("cs1", "disp1", "view1", OCIO::TRANSFORM_DIR_FORWARD),
10181140
OCIO::Exception,
10191141
"The specified file reference '$CS2' could not be located");

0 commit comments

Comments
 (0)