From 609a9491203137626ebae5333dd94084f8140529 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 28 Feb 2023 15:36:41 +0000 Subject: [PATCH] records.yaml: Make sure when a string field is set to NULL then this gets replicated into the records and not get lost as null field which is not the intention of null strings. NULL string fields should still make the internal records as null value. --- src/records/RecUtils.cc | 2 +- src/records/RecYAMLDecoder.cc | 30 +++++++++++---- tests/gold_tests/records/records_yaml.test.py | 38 +++++++++++++++++++ 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/src/records/RecUtils.cc b/src/records/RecUtils.cc index cb9ec5a5371..e229a41f6f0 100644 --- a/src/records/RecUtils.cc +++ b/src/records/RecUtils.cc @@ -357,7 +357,7 @@ RecDataSetFromString(RecDataT data_type, RecData *data_dst, const char *data_str data_src.rec_float = atof(data_string); break; case RECD_STRING: - if (data_string && (strlen(data_string) == 4) && strncmp((data_string), "NULL", 4) == 0) { + if (data_string && (strlen(data_string) == 4) && strncasecmp((data_string), "NULL", 4) == 0) { data_src.rec_string = nullptr; } else { // It's OK to cast away the const here, because RecDataSet will copy the string. diff --git a/src/records/RecYAMLDecoder.cc b/src/records/RecYAMLDecoder.cc index 6015bd686ea..002361c85fb 100644 --- a/src/records/RecYAMLDecoder.cc +++ b/src/records/RecYAMLDecoder.cc @@ -90,24 +90,39 @@ SetRecordFromYAMLNode(CfgNode const &field, swoc::Errata &errata) rec_type = found->type; data_type = found->value_type; } else { - std::string field_name = field.node.as(); - // Not registered in ATS, could be a plugin or an invalid(not registered) records. // Externally registered records should have the type set in each field (!!int, !!float, etc), otherwise we will not be able to // deduce the type and we could end up doing a bad type cast at the end. So we say if there is no type(tag) specified, then // we ignore it. auto [dtype, e] = detail::try_deduce_type(field.value_node); if (!e.empty()) { - errata.note(ERRATA_WARN, "Ignoring field '{}' at Line {}. Not registered and {}", field_name, field.node.Mark().line + 1, e); + errata.note(ERRATA_WARN, "Ignoring field '{}' at Line {}. Not registered and {}", field.node.as(), + field.node.Mark().line + 1, e); // We can't continue without knowing the type. return; } data_type = dtype; // field tags found. } - std::string nvalue = field.value_node.as(); - std::string value_str = RecConfigOverrideFromEnvironment(record_name.c_str(), nvalue.c_str()); - RecSourceT source = (nvalue == value_str ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV); + // It could happen that a field was set to null. We only care for string type, we want + // this to be explicitly set so the librecords can deal with this. For non strings we + // will use the default value. + // + if (YAML::NodeType::Null == field.value_node.Type()) { + switch (data_type) { + case RecDataT::RECD_INT: + case RecDataT::RECD_FLOAT: + errata.note(ERRATA_DEBUG, "Field '{}' set to null. Default value will be used", field.node.as()); + return; + default:; + } + } + + std::string field_value = field.value_node.as(); // in case of a string, the library will give us the literal + // 'null' which is exactly what we want. + + std::string value_str = RecConfigOverrideFromEnvironment(record_name.c_str(), field_value.c_str()); + RecSourceT source = (field_value == value_str ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV); if (source == REC_SOURCE_ENV) { errata.note(ERRATA_DEBUG, "'{}' was override with '{}' using an env variable", record_name, value_str); @@ -191,7 +206,8 @@ flatten_node(CfgNode const &field, RecYAMLNodeHandler handler, swoc::Errata &err } } break; case YAML::NodeType::Sequence: - case YAML::NodeType::Scalar: { + case YAML::NodeType::Scalar: + case YAML::NodeType::Null: { field.append_field_name(); handler(field, errata); } break; diff --git a/tests/gold_tests/records/records_yaml.test.py b/tests/gold_tests/records/records_yaml.test.py index cc1d736e39c..ed56a9b0200 100644 --- a/tests/gold_tests/records/records_yaml.test.py +++ b/tests/gold_tests/records/records_yaml.test.py @@ -157,3 +157,41 @@ def check_response(resp: Response): 'proxy.config.diags.debug.tags: filemanager', 'Config should show a different tag' ) + + +ts2.Disk.records_config.append_to_document( + ''' + dns: + resolv_conf: NULL + nameservers: null + local_ipv6: Null + ssl: + client: + cert: + filename: ~ + ''' +) + + +tr3 = Test.AddTestRun("test null string") +tr3.Processes.Default.Command = 'traffic_ctl config get proxy.config.dns.resolv_conf proxy.config.dns.local_ipv6 proxy.config.dns.nameservers proxy.config.ssl.client.cert.filename' +tr3.Processes.Default.Env = ts2.Env +tr3.Processes.Default.ReturnCode = 0 + +# Make sure it's what we want. +tr3.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.dns.resolv_conf: null', + 'should be set to null' +) +tr3.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.dns.nameservers: null', + 'should be set to null' +) +tr3.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.ssl.client.cert.filename: null', + 'should be set to null' +) +tr3.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.dns.local_ipv6: null', + 'should be set to null' +)