You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by kevdoran <gi...@git.apache.org> on 2017/04/28 21:52:48 UTC

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

GitHub user kevdoran opened a pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85

    MINIFI-275 Bugfix for YAML Configs without Component IDs

    This commit fixes a bug in which loading config YAML that did not
    specify IDs (UUIDs) for components caused an exception. The logic
    previously treated component IDs as required fields by loading them
    without checking for existence. This commit updates the logic to
    generate IDs when they are not specified in the YAML config. As part
    of this fix, the logic for loading connections from YAML config was
    modified to search by name or remote port id in the absence of source
    id or remote id in the YAML config.
    
    For fields that are required, useful error messages were added when
    those fields are missing to assist users in self-diagnosing issues
    related to invalid config files.
    
    Some minor tweaks to the top level .gitignore are included with this
    commit.
    
    Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
    
    In order to streamline the review of the contribution we ask you
    to ensure the following steps have been taken:
    
    ### For all changes:
    - [ ] Is there a JIRA ticket associated with this PR? Is it referenced
         in the commit message?
    
    - [ ] Does your PR title start with MINIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
    
    - [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
    
    - [ ] Is your initial contribution a single, squashed commit?
    
    ### For code changes:
    - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
    - [ ] If applicable, have you updated the LICENSE file?
    - [ ] If applicable, have you updated the NOTICE file?
    
    ### For documentation related changes:
    - [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
    
    ### Note:
    Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/kevdoran/nifi-minifi-cpp bugfix/MINIFI-275-configs-without-component-ids

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/nifi-minifi-cpp/pull/85.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #85
    
----
commit eb7b992872a71b8c01444ef24eeae48121559a5e
Author: Kevin Doran <kd...@gmail.com>
Date:   2017-04-28T21:43:37Z

    MINIFI-275 Bugfix for YAML Configs without Component IDs
    
    This commit fixes a bug in which loading config YAML that did not
    specify IDs (UUIDs) for components caused an exception. The logic
    previously treated component IDs as required fields by loading them
    without checking for existence. This commit updates the logic to
    generate IDs when they are not specified in the YAML config. As part
    of this fix, the logic for loading connections from YAML config was
    modified to search by name or remote port id in the absence of source
    id or remote id in the YAML config.
    
    For fields that are required, useful error messages were added when
    those fields are missing to assist users in self-diagnosing issues
    related to invalid config files.
    
    Some minor tweaks to the top level .gitignore are included with this
    commit.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114367613
  
    --- Diff: libminifi/include/core/yaml/YamlConfiguration.h ---
    @@ -88,10 +102,17 @@ class YamlConfiguration : public FlowConfiguration {
       void parseRemoteProcessGroupYaml(YAML::Node *node,
                                        core::ProcessGroup * parent);
       // Process Provenance Report YAML
    -  void parseProvenanceReportingYaml(YAML::Node *reportNode, core::ProcessGroup * parentGroup);
    +  void parseProvenanceReportingYaml(YAML::Node *reportNode,
    +                                    core::ProcessGroup * parentGroup);
       // Parse Properties Node YAML for a processor
       void parsePropertiesNodeYaml(YAML::Node *propertiesNode,
                                    std::shared_ptr<core::Processor> processor);
    +  // Helper function to get or generate a component's id field
    --- End diff --
    
    I like that. I'll update all existing function comments in YamlConfiguration.h while I'm here


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114378307
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -538,14 +561,62 @@ void YamlConfiguration::parsePropertiesNodeYaml(
           std::string rawValueString = propertyValueNode.as<std::string>();
           if (!processor->setProperty(propertyName, rawValueString)) {
             logger_->log_warn(
    -            "Received property %s with value %s but is not one of the properties for %s",
    -            propertyName.c_str(), rawValueString.c_str(),
    -            processor->getName().c_str());
    +            "Received property %s with value %s but it is not one of the properties for %s",
    +            propertyName,
    +            rawValueString,
    +            processor->getName());
           }
         }
       }
     }
     
    +std::string YamlConfiguration::getOrGenerateId(
    +    YAML::Node *yamlNode,
    +    std::string idField) {
    +  std::string id;
    +  YAML::Node node = yamlNode->as<YAML::Node>();
    +
    +  if (node[idField]) {
    +    if (YAML::NodeType::Scalar == node[idField].Type()) {
    +      id = node[idField].as<std::string>();
    +    } else {
    +      throw std::invalid_argument(
    +          "getOrGenerateId: idField is expected to reference YAML::Node "
    +              "of YAML::NodeType::Scalar.");
    +    }
    +  } else {
    +    uuid_t uuid;
    +    uuid_generate(uuid);
    +    char uuid_str[37];
    +    uuid_unparse(uuid, uuid_str);
    +    id = uuid_str;
    +    logger_->log_debug("Generating random ID: id => [%s]", id);
    +  }
    +  return id;
    +}
    +
    +void YamlConfiguration::checkRequiredField(
    --- End diff --
    
    Oh I see. Nope, not checking for InvalidNode. Will take a look at that and might add some basic handling, but may need to spend some more time thinking about how to best implement that as the [] operator is used throughout this file, not just in this function. So yeah, might open a ticket instead


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114362748
  
    --- Diff: libminifi/include/core/yaml/YamlConfiguration.h ---
    @@ -88,10 +102,17 @@ class YamlConfiguration : public FlowConfiguration {
       void parseRemoteProcessGroupYaml(YAML::Node *node,
                                        core::ProcessGroup * parent);
       // Process Provenance Report YAML
    -  void parseProvenanceReportingYaml(YAML::Node *reportNode, core::ProcessGroup * parentGroup);
    +  void parseProvenanceReportingYaml(YAML::Node *reportNode,
    +                                    core::ProcessGroup * parentGroup);
       // Parse Properties Node YAML for a processor
       void parsePropertiesNodeYaml(YAML::Node *propertiesNode,
                                    std::shared_ptr<core::Processor> processor);
    +  // Helper function to get or generate a component's id field
    +  std::string getOrGenerateId(YAML::Node *yamlNode, std::string idField = "id");
    +  // Get or generate a component's id field
    --- End diff --
    
    Good catch on the comment, and yeah, will switch to const std::string &str


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114543586
  
    --- Diff: libminifi/test/unit/YamlCongifurationTests.cpp ---
    @@ -0,0 +1,182 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +#include <memory>
    +#include <string>
    +#include <core/RepositoryFactory.h>
    +#include "core/yaml/YamlConfiguration.h"
    +#include "../TestBase.h"
    +
    +static const std::shared_ptr<core::Repository> TEST_PROV_REPO = core::createRepository("provenancerepository", true);
    +static const std::shared_ptr<core::Repository> TEST_FF_REPO = core::createRepository("flowfilerepository", true);
    +
    +TEST_CASE("Test YAML Config 1", "[testyamlconfig1]") {
    +
    +  static const std::string TEST_YAML_WITHOUT_IDS = "MiNiFi Config Version: 1\n"
    --- End diff --
    
    @apiri, I'm opening a JIRA now


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114364384
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -491,14 +510,18 @@ void YamlConfiguration::parsePortYaml(YAML::Node *portNode,
     
       YAML::Node inputPortsObj = portNode->as<YAML::Node>();
     
    -  // generate the random UIID
    -  uuid_generate(uuid);
    -
    -  auto portId = inputPortsObj["id"].as<std::string>();
    +  // Check for required fields
    +  checkRequiredField(&inputPortsObj, "name");
       auto nameStr = inputPortsObj["name"].as<std::string>();
    +  checkRequiredField(&inputPortsObj, "id", "The field 'id' is required for "
    +      "the port named '" + nameStr + "' in the YAML Config. If this port "
    +      "is specificy an input port for a NiFi Remote Process Group, the port "
    +      "id should match the id of of the input port in the NiFi configuration. "
    +      "This is a UUID of the format XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX.");
    +  auto portId = inputPortsObj["id"].as<std::string>();
       uuid_parse(portId.c_str(), uuid);
     
    -  port = new minifi::RemoteProcessorGroupPort(nameStr.c_str(), uuid);
    +  port = new minifi::RemoteProcessorGroupPort(nameStr, uuid);
     
       processor = (std::shared_ptr<core::Processor>) port;
    --- End diff --
    
    ok. the same style cast occurs in a few other places in the file, eg, getRoot(...). I assume you prefer just avoiding raw pointers altogether when possible?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114377473
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -491,14 +510,18 @@ void YamlConfiguration::parsePortYaml(YAML::Node *portNode,
     
       YAML::Node inputPortsObj = portNode->as<YAML::Node>();
     
    -  // generate the random UIID
    -  uuid_generate(uuid);
    -
    -  auto portId = inputPortsObj["id"].as<std::string>();
    +  // Check for required fields
    +  checkRequiredField(&inputPortsObj, "name");
       auto nameStr = inputPortsObj["name"].as<std::string>();
    +  checkRequiredField(&inputPortsObj, "id", "The field 'id' is required for "
    +      "the port named '" + nameStr + "' in the YAML Config. If this port "
    +      "is specificy an input port for a NiFi Remote Process Group, the port "
    +      "id should match the id of of the input port in the NiFi configuration. "
    +      "This is a UUID of the format XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX.");
    +  auto portId = inputPortsObj["id"].as<std::string>();
       uuid_parse(portId.c_str(), uuid);
     
    -  port = new minifi::RemoteProcessorGroupPort(nameStr.c_str(), uuid);
    +  port = new minifi::RemoteProcessorGroupPort(nameStr, uuid);
     
       processor = (std::shared_ptr<core::Processor>) port;
    --- End diff --
    
    This will create a true shared pointer object for port and perform a compile time cast. I think the only reason port exists is to call RPG specific functions later, which predates me. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114138536
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -491,14 +510,18 @@ void YamlConfiguration::parsePortYaml(YAML::Node *portNode,
     
       YAML::Node inputPortsObj = portNode->as<YAML::Node>();
     
    -  // generate the random UIID
    -  uuid_generate(uuid);
    -
    -  auto portId = inputPortsObj["id"].as<std::string>();
    +  // Check for required fields
    +  checkRequiredField(&inputPortsObj, "name");
       auto nameStr = inputPortsObj["name"].as<std::string>();
    +  checkRequiredField(&inputPortsObj, "id", "The field 'id' is required for "
    +      "the port named '" + nameStr + "' in the YAML Config. If this port "
    +      "is specificy an input port for a NiFi Remote Process Group, the port "
    +      "id should match the id of of the input port in the NiFi configuration. "
    +      "This is a UUID of the format XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX.");
    +  auto portId = inputPortsObj["id"].as<std::string>();
       uuid_parse(portId.c_str(), uuid);
     
    -  port = new minifi::RemoteProcessorGroupPort(nameStr.c_str(), uuid);
    +  port = new minifi::RemoteProcessorGroupPort(nameStr, uuid);
     
       processor = (std::shared_ptr<core::Processor>) port;
    --- End diff --
    
    we should fix this while we're here. we shouldn't cast a raw pointer to a shared pointer. That can have unintended consequences. I didn't catch this when making changes, apparently. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114375962
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -538,14 +561,62 @@ void YamlConfiguration::parsePropertiesNodeYaml(
           std::string rawValueString = propertyValueNode.as<std::string>();
           if (!processor->setProperty(propertyName, rawValueString)) {
             logger_->log_warn(
    -            "Received property %s with value %s but is not one of the properties for %s",
    -            propertyName.c_str(), rawValueString.c_str(),
    -            processor->getName().c_str());
    +            "Received property %s with value %s but it is not one of the properties for %s",
    +            propertyName,
    +            rawValueString,
    +            processor->getName());
           }
         }
       }
     }
     
    +std::string YamlConfiguration::getOrGenerateId(
    +    YAML::Node *yamlNode,
    +    std::string idField) {
    +  std::string id;
    +  YAML::Node node = yamlNode->as<YAML::Node>();
    +
    +  if (node[idField]) {
    +    if (YAML::NodeType::Scalar == node[idField].Type()) {
    +      id = node[idField].as<std::string>();
    +    } else {
    +      throw std::invalid_argument(
    +          "getOrGenerateId: idField is expected to reference YAML::Node "
    +              "of YAML::NodeType::Scalar.");
    +    }
    +  } else {
    +    uuid_t uuid;
    +    uuid_generate(uuid);
    +    char uuid_str[37];
    +    uuid_unparse(uuid, uuid_str);
    +    id = uuid_str;
    +    logger_->log_debug("Generating random ID: id => [%s]", id);
    +  }
    +  return id;
    +}
    +
    +void YamlConfiguration::checkRequiredField(
    --- End diff --
    
    Sorry, the double reply was likely confusing. I'm good with the function itself. The code that I'm looking at in our third party directory returns a Node
    template <typename Key>
      const Node operator[](const Key& key) const;
    
    The exception I reference is below and throws an InvalidNode if the node is invalid some how. Is this also handled?
    `template <typename Key>
    inline const Node Node::operator[](const Key& key) const {
      if (!m_isValid)
        throw InvalidNode();
      EnsureNodeExists();
      detail::node* value = static_cast<const detail::node&>(*m_pNode)
                                .get(detail::to_value(key), m_pMemory);
      if (!value) {
        return Node(ZombieNode);
      }
      return Node(*value, m_pMemory);
    }`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114138730
  
    --- Diff: libminifi/include/core/yaml/YamlConfiguration.h ---
    @@ -88,10 +102,17 @@ class YamlConfiguration : public FlowConfiguration {
       void parseRemoteProcessGroupYaml(YAML::Node *node,
                                        core::ProcessGroup * parent);
       // Process Provenance Report YAML
    -  void parseProvenanceReportingYaml(YAML::Node *reportNode, core::ProcessGroup * parentGroup);
    +  void parseProvenanceReportingYaml(YAML::Node *reportNode,
    +                                    core::ProcessGroup * parentGroup);
       // Parse Properties Node YAML for a processor
       void parsePropertiesNodeYaml(YAML::Node *propertiesNode,
                                    std::shared_ptr<core::Processor> processor);
    +  // Helper function to get or generate a component's id field
    --- End diff --
    
    Can you use a more verbose comment block here? We're moving away from single line comments here. JavaDoc is better and will have a more positive impact when we generate docs than these single line function comments. Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp issue #85: MINIFI-275 Bugfix for YAML Configs without Compon...

Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:

    https://github.com/apache/nifi-minifi-cpp/pull/85
  
    Thanks for the review and feedback @phrocker!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp issue #85: MINIFI-275 Bugfix for YAML Configs without Compon...

Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:

    https://github.com/apache/nifi-minifi-cpp/pull/85
  
    reviewing


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114376703
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -538,14 +561,62 @@ void YamlConfiguration::parsePropertiesNodeYaml(
           std::string rawValueString = propertyValueNode.as<std::string>();
           if (!processor->setProperty(propertyName, rawValueString)) {
             logger_->log_warn(
    -            "Received property %s with value %s but is not one of the properties for %s",
    -            propertyName.c_str(), rawValueString.c_str(),
    -            processor->getName().c_str());
    +            "Received property %s with value %s but it is not one of the properties for %s",
    +            propertyName,
    +            rawValueString,
    +            processor->getName());
           }
         }
       }
     }
     
    +std::string YamlConfiguration::getOrGenerateId(
    +    YAML::Node *yamlNode,
    +    std::string idField) {
    +  std::string id;
    +  YAML::Node node = yamlNode->as<YAML::Node>();
    +
    +  if (node[idField]) {
    +    if (YAML::NodeType::Scalar == node[idField].Type()) {
    +      id = node[idField].as<std::string>();
    +    } else {
    +      throw std::invalid_argument(
    +          "getOrGenerateId: idField is expected to reference YAML::Node "
    +              "of YAML::NodeType::Scalar.");
    +    }
    +  } else {
    +    uuid_t uuid;
    +    uuid_generate(uuid);
    +    char uuid_str[37];
    +    uuid_unparse(uuid, uuid_str);
    +    id = uuid_str;
    +    logger_->log_debug("Generating random ID: id => [%s]", id);
    +  }
    +  return id;
    +}
    +
    +void YamlConfiguration::checkRequiredField(
    --- End diff --
    
    While it's touching the same code, I believe this may be out of the scope of this PR and ticket. We can simply create another ticket for this case. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114469266
  
    --- Diff: libminifi/test/unit/YamlCongifurationTests.cpp ---
    @@ -0,0 +1,182 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +#include <memory>
    +#include <string>
    +#include <core/RepositoryFactory.h>
    +#include "core/yaml/YamlConfiguration.h"
    +#include "../TestBase.h"
    +
    +static const std::shared_ptr<core::Repository> TEST_PROV_REPO = core::createRepository("provenancerepository", true);
    +static const std::shared_ptr<core::Repository> TEST_FF_REPO = core::createRepository("flowfilerepository", true);
    +
    +TEST_CASE("Test YAML Config 1", "[testyamlconfig1]") {
    +
    +  static const std::string TEST_YAML_WITHOUT_IDS = "MiNiFi Config Version: 1\n"
    --- End diff --
    
    @kevdoran @phrocker if there isn't one, can we be sure to capture this in a JIRA issue?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp issue #85: MINIFI-275 Bugfix for YAML Configs without Compon...

Posted by apiri <gi...@git.apache.org>.
Github user apiri commented on the issue:

    https://github.com/apache/nifi-minifi-cpp/pull/85
  
    @kevdoran looks good here!  thanks for fixing this up and associated test cases.
    
    will get this merged.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114138828
  
    --- Diff: libminifi/include/core/yaml/YamlConfiguration.h ---
    @@ -88,10 +102,17 @@ class YamlConfiguration : public FlowConfiguration {
       void parseRemoteProcessGroupYaml(YAML::Node *node,
                                        core::ProcessGroup * parent);
       // Process Provenance Report YAML
    -  void parseProvenanceReportingYaml(YAML::Node *reportNode, core::ProcessGroup * parentGroup);
    +  void parseProvenanceReportingYaml(YAML::Node *reportNode,
    +                                    core::ProcessGroup * parentGroup);
       // Parse Properties Node YAML for a processor
       void parsePropertiesNodeYaml(YAML::Node *propertiesNode,
                                    std::shared_ptr<core::Processor> processor);
    +  // Helper function to get or generate a component's id field
    +  std::string getOrGenerateId(YAML::Node *yamlNode, std::string idField = "id");
    +  // Get or generate a component's id field
    --- End diff --
    
    Let's make a habit of using const std::string &str


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114137974
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -538,14 +561,62 @@ void YamlConfiguration::parsePropertiesNodeYaml(
           std::string rawValueString = propertyValueNode.as<std::string>();
           if (!processor->setProperty(propertyName, rawValueString)) {
             logger_->log_warn(
    -            "Received property %s with value %s but is not one of the properties for %s",
    -            propertyName.c_str(), rawValueString.c_str(),
    -            processor->getName().c_str());
    +            "Received property %s with value %s but it is not one of the properties for %s",
    +            propertyName,
    +            rawValueString,
    +            processor->getName());
           }
         }
       }
     }
     
    +std::string YamlConfiguration::getOrGenerateId(
    +    YAML::Node *yamlNode,
    +    std::string idField) {
    +  std::string id;
    +  YAML::Node node = yamlNode->as<YAML::Node>();
    +
    +  if (node[idField]) {
    +    if (YAML::NodeType::Scalar == node[idField].Type()) {
    +      id = node[idField].as<std::string>();
    +    } else {
    +      throw std::invalid_argument(
    +          "getOrGenerateId: idField is expected to reference YAML::Node "
    +              "of YAML::NodeType::Scalar.");
    +    }
    +  } else {
    +    uuid_t uuid;
    +    uuid_generate(uuid);
    +    char uuid_str[37];
    +    uuid_unparse(uuid, uuid_str);
    +    id = uuid_str;
    +    logger_->log_debug("Generating random ID: id => [%s]", id);
    +  }
    +  return id;
    +}
    +
    +void YamlConfiguration::checkRequiredField(
    --- End diff --
    
    Perhaps I should rephrase this to say that we can also get an InvalidNode exception. I think this occurs when the node exists check occurs, and when the current node isn't valid. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114374896
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -491,14 +510,18 @@ void YamlConfiguration::parsePortYaml(YAML::Node *portNode,
     
       YAML::Node inputPortsObj = portNode->as<YAML::Node>();
     
    -  // generate the random UIID
    -  uuid_generate(uuid);
    -
    -  auto portId = inputPortsObj["id"].as<std::string>();
    +  // Check for required fields
    +  checkRequiredField(&inputPortsObj, "name");
       auto nameStr = inputPortsObj["name"].as<std::string>();
    +  checkRequiredField(&inputPortsObj, "id", "The field 'id' is required for "
    +      "the port named '" + nameStr + "' in the YAML Config. If this port "
    +      "is specificy an input port for a NiFi Remote Process Group, the port "
    +      "id should match the id of of the input port in the NiFi configuration. "
    +      "This is a UUID of the format XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX.");
    +  auto portId = inputPortsObj["id"].as<std::string>();
       uuid_parse(portId.c_str(), uuid);
     
    -  port = new minifi::RemoteProcessorGroupPort(nameStr.c_str(), uuid);
    +  port = new minifi::RemoteProcessorGroupPort(nameStr, uuid);
     
       processor = (std::shared_ptr<core::Processor>) port;
    --- End diff --
    
    I don't see the cast you are referring to in getRoot. I only see the use of the unique_ptr, which  okay as that is currently the only way to create a unique pointer. What I'm referencing is the explicit cast of a pointer to a shared_ptr object, which could have negative consequences at some point. If this is an endemic issue we can create a ticket, but I didn't see another case of this specific cast. The appropriate thing to do is std::make_shared<minifi::RemoteProcessorGroupPort>(nameStr,uuid);


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114375744
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -491,14 +510,18 @@ void YamlConfiguration::parsePortYaml(YAML::Node *portNode,
     
       YAML::Node inputPortsObj = portNode->as<YAML::Node>();
     
    -  // generate the random UIID
    -  uuid_generate(uuid);
    -
    -  auto portId = inputPortsObj["id"].as<std::string>();
    +  // Check for required fields
    +  checkRequiredField(&inputPortsObj, "name");
       auto nameStr = inputPortsObj["name"].as<std::string>();
    +  checkRequiredField(&inputPortsObj, "id", "The field 'id' is required for "
    +      "the port named '" + nameStr + "' in the YAML Config. If this port "
    +      "is specificy an input port for a NiFi Remote Process Group, the port "
    +      "id should match the id of of the input port in the NiFi configuration. "
    +      "This is a UUID of the format XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX.");
    +  auto portId = inputPortsObj["id"].as<std::string>();
       uuid_parse(portId.c_str(), uuid);
     
    -  port = new minifi::RemoteProcessorGroupPort(nameStr.c_str(), uuid);
    +  port = new minifi::RemoteProcessorGroupPort(nameStr, uuid);
     
       processor = (std::shared_ptr<core::Processor>) port;
    --- End diff --
    
    I updated my earlier comment. You're right, this is a one-off not an endemic issue.
    
    is this cast ok:
    
    `std::shared_ptr<core::Processor> processor = NULL;
    std::shared_ptr<minifi::RemoteProcessorGroupPort> port = NULL;
    port = new minifi::RemoteProcessorGroupPort(nameStr, uuid);
    processor = (std::shared_ptr<core::Processor>) port;
    `


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114371075
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -538,14 +561,62 @@ void YamlConfiguration::parsePropertiesNodeYaml(
           std::string rawValueString = propertyValueNode.as<std::string>();
           if (!processor->setProperty(propertyName, rawValueString)) {
             logger_->log_warn(
    -            "Received property %s with value %s but is not one of the properties for %s",
    -            propertyName.c_str(), rawValueString.c_str(),
    -            processor->getName().c_str());
    +            "Received property %s with value %s but it is not one of the properties for %s",
    +            propertyName,
    +            rawValueString,
    +            processor->getName());
           }
         }
       }
     }
     
    +std::string YamlConfiguration::getOrGenerateId(
    +    YAML::Node *yamlNode,
    +    std::string idField) {
    +  std::string id;
    +  YAML::Node node = yamlNode->as<YAML::Node>();
    +
    +  if (node[idField]) {
    +    if (YAML::NodeType::Scalar == node[idField].Type()) {
    +      id = node[idField].as<std::string>();
    +    } else {
    +      throw std::invalid_argument(
    +          "getOrGenerateId: idField is expected to reference YAML::Node "
    +              "of YAML::NodeType::Scalar.");
    +    }
    +  } else {
    +    uuid_t uuid;
    +    uuid_generate(uuid);
    +    char uuid_str[37];
    +    uuid_unparse(uuid, uuid_str);
    +    id = uuid_str;
    +    logger_->log_debug("Generating random ID: id => [%s]", id);
    +  }
    +  return id;
    +}
    +
    +void YamlConfiguration::checkRequiredField(
    --- End diff --
    
    Hey @phrocker,
    
    If I follow you correctly, the concern is about the conditional on line 610?
    
    operator[] returns a bool for the version of yaml-cpp we are using:
    https://github.com/jbeder/yaml-cpp/wiki/Tutorial
    
    In older versions of the yaml-cpp api, it did in fact throw an exception as you describe:
    https://github.com/jbeder/yaml-cpp/wiki/How-To-Parse-A-Document-(Old-API)#optional-keys
    
    As to the function in general, it puts the logic in one place for generating helpful error messages and eliminated the need for if/else or try/catch blocks throughout the code. The idea is you would call this prior to using a required field; if the field is not present, a runtime error with a useful user-facing error message is generated, otherwise you are safe to use the field without a try/catch or if/else after calling checkRequiredField(...)
    
    Does that answer your question?
    
    I would like input on the function interface (aside from the types, which I am changing to const std::string &str).. I ended up tacking on a couple optional fields solely for the use in the error message generation, but it does seem like a lot to ask the caller just for an assertion check. Let me know if you see a more elegant way to achieve the desired functionality here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114363142
  
    --- Diff: libminifi/test/unit/YamlCongifurationTests.cpp ---
    @@ -0,0 +1,182 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +#include <memory>
    +#include <string>
    +#include <core/RepositoryFactory.h>
    +#include "core/yaml/YamlConfiguration.h"
    +#include "../TestBase.h"
    +
    +static const std::shared_ptr<core::Repository> TEST_PROV_REPO = core::createRepository("provenancerepository", true);
    +static const std::shared_ptr<core::Repository> TEST_FF_REPO = core::createRepository("flowfilerepository", true);
    +
    +TEST_CASE("Test YAML Config 1", "[testyamlconfig1]") {
    +
    +  static const std::string TEST_YAML_WITHOUT_IDS = "MiNiFi Config Version: 1\n"
    --- End diff --
    
    will do


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114137427
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -538,14 +561,62 @@ void YamlConfiguration::parsePropertiesNodeYaml(
           std::string rawValueString = propertyValueNode.as<std::string>();
           if (!processor->setProperty(propertyName, rawValueString)) {
             logger_->log_warn(
    -            "Received property %s with value %s but is not one of the properties for %s",
    -            propertyName.c_str(), rawValueString.c_str(),
    -            processor->getName().c_str());
    +            "Received property %s with value %s but it is not one of the properties for %s",
    --- End diff --
    
    is this with the formatter applied?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114138774
  
    --- Diff: libminifi/include/core/yaml/YamlConfiguration.h ---
    @@ -88,10 +102,17 @@ class YamlConfiguration : public FlowConfiguration {
       void parseRemoteProcessGroupYaml(YAML::Node *node,
                                        core::ProcessGroup * parent);
       // Process Provenance Report YAML
    -  void parseProvenanceReportingYaml(YAML::Node *reportNode, core::ProcessGroup * parentGroup);
    +  void parseProvenanceReportingYaml(YAML::Node *reportNode,
    +                                    core::ProcessGroup * parentGroup);
       // Parse Properties Node YAML for a processor
       void parsePropertiesNodeYaml(YAML::Node *propertiesNode,
                                    std::shared_ptr<core::Processor> processor);
    +  // Helper function to get or generate a component's id field
    +  std::string getOrGenerateId(YAML::Node *yamlNode, std::string idField = "id");
    +  // Get or generate a component's id field
    --- End diff --
    
    I'm not sure this comment matches. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114137644
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -538,14 +561,62 @@ void YamlConfiguration::parsePropertiesNodeYaml(
           std::string rawValueString = propertyValueNode.as<std::string>();
           if (!processor->setProperty(propertyName, rawValueString)) {
             logger_->log_warn(
    -            "Received property %s with value %s but is not one of the properties for %s",
    -            propertyName.c_str(), rawValueString.c_str(),
    -            processor->getName().c_str());
    +            "Received property %s with value %s but it is not one of the properties for %s",
    +            propertyName,
    +            rawValueString,
    +            processor->getName());
           }
         }
       }
     }
     
    +std::string YamlConfiguration::getOrGenerateId(
    +    YAML::Node *yamlNode,
    +    std::string idField) {
    +  std::string id;
    +  YAML::Node node = yamlNode->as<YAML::Node>();
    +
    +  if (node[idField]) {
    +    if (YAML::NodeType::Scalar == node[idField].Type()) {
    +      id = node[idField].as<std::string>();
    +    } else {
    +      throw std::invalid_argument(
    +          "getOrGenerateId: idField is expected to reference YAML::Node "
    +              "of YAML::NodeType::Scalar.");
    +    }
    +  } else {
    +    uuid_t uuid;
    +    uuid_generate(uuid);
    +    char uuid_str[37];
    +    uuid_unparse(uuid, uuid_str);
    +    id = uuid_str;
    +    logger_->log_debug("Generating random ID: id => [%s]", id);
    +  }
    +  return id;
    +}
    +
    +void YamlConfiguration::checkRequiredField(
    --- End diff --
    
    Why/how do we need this? operator[] or YamlNode throws an InvalidNode. Do you catch this? I did not see that. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114137733
  
    --- Diff: libminifi/test/unit/YamlCongifurationTests.cpp ---
    @@ -0,0 +1,182 @@
    +/**
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +#include <memory>
    +#include <string>
    +#include <core/RepositoryFactory.h>
    +#include "core/yaml/YamlConfiguration.h"
    +#include "../TestBase.h"
    +
    +static const std::shared_ptr<core::Repository> TEST_PROV_REPO = core::createRepository("provenancerepository", true);
    +static const std::shared_ptr<core::Repository> TEST_FF_REPO = core::createRepository("flowfilerepository", true);
    +
    +TEST_CASE("Test YAML Config 1", "[testyamlconfig1]") {
    +
    +  static const std::string TEST_YAML_WITHOUT_IDS = "MiNiFi Config Version: 1\n"
    --- End diff --
    
    You can add this into the resource directory -- I'm renaming this to resources in a subsequent PR, but there's no need to maintain this text here. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114137339
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -491,14 +510,18 @@ void YamlConfiguration::parsePortYaml(YAML::Node *portNode,
     
       YAML::Node inputPortsObj = portNode->as<YAML::Node>();
     
    -  // generate the random UIID
    -  uuid_generate(uuid);
    -
    -  auto portId = inputPortsObj["id"].as<std::string>();
    +  // Check for required fields
    +  checkRequiredField(&inputPortsObj, "name");
       auto nameStr = inputPortsObj["name"].as<std::string>();
    +  checkRequiredField(&inputPortsObj, "id", "The field 'id' is required for "
    +      "the port named '" + nameStr + "' in the YAML Config. If this port "
    +      "is specificy an input port for a NiFi Remote Process Group, the port "
    --- End diff --
    
    typo


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by phrocker <gi...@git.apache.org>.
Github user phrocker commented on a diff in the pull request:

    https://github.com/apache/nifi-minifi-cpp/pull/85#discussion_r114377308
  
    --- Diff: libminifi/src/core/yaml/YamlConfiguration.cpp ---
    @@ -491,14 +510,18 @@ void YamlConfiguration::parsePortYaml(YAML::Node *portNode,
     
       YAML::Node inputPortsObj = portNode->as<YAML::Node>();
     
    -  // generate the random UIID
    -  uuid_generate(uuid);
    -
    -  auto portId = inputPortsObj["id"].as<std::string>();
    +  // Check for required fields
    +  checkRequiredField(&inputPortsObj, "name");
       auto nameStr = inputPortsObj["name"].as<std::string>();
    +  checkRequiredField(&inputPortsObj, "id", "The field 'id' is required for "
    +      "the port named '" + nameStr + "' in the YAML Config. If this port "
    +      "is specificy an input port for a NiFi Remote Process Group, the port "
    +      "id should match the id of of the input port in the NiFi configuration. "
    +      "This is a UUID of the format XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX.");
    +  auto portId = inputPortsObj["id"].as<std::string>();
       uuid_parse(portId.c_str(), uuid);
     
    -  port = new minifi::RemoteProcessorGroupPort(nameStr.c_str(), uuid);
    +  port = new minifi::RemoteProcessorGroupPort(nameStr, uuid);
     
       processor = (std::shared_ptr<core::Processor>) port;
    --- End diff --
    
    No that cast is not okay. That should be
    ```
    port = std::make_shared<minifi::RemoteProcessorGroupPort>(nameStr,uuid);
    processor = std::static_pointer_cast<core::Processor>(port);
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] nifi-minifi-cpp pull request #85: MINIFI-275 Bugfix for YAML Configs without...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/nifi-minifi-cpp/pull/85


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---