Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HPCC-19216 Dynamically add <Properties> to ESPService and ESPBinding sections #11034

Merged
merged 1 commit into from May 3, 2018

Conversation

Projects
None yet
6 participants
@kenrowland
Copy link
Contributor

commented Apr 17, 2018

Add support to dynamically add sections to
ESPService and ESPBinding sections in the environment

Signed-off-by: Ken Rowland kenneth.rowland@lexisnexisrisk.com

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Testing:

@hpcc-jirabot

This comment has been minimized.

Copy link

commented Apr 17, 2018

@kenrowland

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2018

@rpastrana @afishbeck Please review

This PR adds event processing for element creating that inserts XML (properties and authentication features) into dependent sections, namely ESPService and ESPBinding.

It also adds dependent attribute values. This handles default http and https port numbers for ESPBindings.

@kenrowland kenrowland force-pushed the kenrowland:HPCC-19216 branch from 2ca63e5 to 53b00ed Apr 17, 2018

@afishbeck
Copy link
Member

left a comment

Generally looks ok. Some comments. There may be places where you should be using the "override" declaration when overriding virtual functions. Didn't take the time to track down specific ones.

{
if (!m_itemAttribute.empty())
{
std::shared_ptr<EnvironmentValue> pItemAttr = pEventNode->getAttribute(m_itemAttribute);

This comment has been minimized.

Copy link
@afishbeck

afishbeck Apr 24, 2018

Member

Should cases like these be "unique_ptr" rather than shared_ptr? If so, there are probably many cases throughout.

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 24, 2018

Author Contributor

In most cases the called method returns a shared_ptr since because the object already exists and most likely has a reference. Converting to a unique_ptr in these cases would not follow the architecture since unique ownership is not the intent. I will, however, keep an eye out for situations where a unique_ptr would suffice, such as where the method creates the object and persistence is not necessary.


EnvironmentLoader() { }
virtual ~EnvironmentLoader() { }
//void addPath(const std::shared_ptr<EnvironmentNode> pNode);

This comment has been minimized.

Copy link
@afishbeck

afishbeck Apr 24, 2018

Member

Not really a fan of keeping commented out code... are these functions going to be readded at some point?

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Methods were moved during some refactoring. I neglected to remove the commented lines. Lines now removed.

@@ -73,6 +77,11 @@ class DECL_EXPORT SchemaItem : public std::enable_shared_from_this<SchemaItem>
void setHidden(bool hidden) { m_hidden = hidden; }
bool isHidden() const { return m_hidden; }

void setParent(const std::shared_ptr<SchemaItem> &parent) { m_pParent = parent; }
std::shared_ptr<const SchemaItem> findSchemaRoot() const;

This comment has been minimized.

Copy link
@afishbeck

afishbeck Apr 24, 2018

Member

I understand the implementation "walks" the parent nodes, but conceptually this doesn't seem like a "find" function,.. more like a "getSchemaRoot()" or similiar name.

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

True, it is a get, find was carried over because it was written while I was implementing the find functionality. I changed it to getSchemaRoot. Similar change made to finding the root node in the environment.

void setItemAttributeName(const std::string &name) { m_itemAttribute = m_matchAttribute = name; }
void setMatchAttributeName(const std::string &name) { m_matchAttribute = name; }
virtual bool handleEvent(const std::string &envType, std::shared_ptr<EnvironmentNode> pEventNode);

This comment has been minimized.

Copy link
@afishbeck

afishbeck Apr 24, 2018

Member

Trivial, but lots of extra space.

virtual ~InsertEnvironmentDataCreateEvent() {}
void setEnvironmentInsertData(const std::string &envData) { m_envData = envData; }
void setMatchPath(const std::string &path) { m_matchPath = path; }
void setItemAttributeName(const std::string &name) { m_itemAttribute = m_matchAttribute = name; }

This comment has been minimized.

Copy link
@afishbeck

afishbeck Apr 24, 2018

Member

Seems like you are relying on the order these methods may be called to change the state. i.e. what if I did:

setMatchAttributeName("match");
setItemAttributeName("item");

?

Unless that's intentional, then in case setMatchAttributeName was already called I would do something like:
void setItemAttributeName(const std::string &name)
{
m_itemAttribute = name;
if (m_matchAttribute.empty())
m_matchAttribute = name;
}


return pNewEnvNode;

/*pParentNode->addChild(pNewEnvNode);

This comment has been minimized.

Copy link
@afishbeck

afishbeck Apr 24, 2018

Member

this block of comment code is particularly ugly.

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Once again, code that had been refactored. Commented section removed.

//
// Send a create event now that it's been added to the environment
pCfgItem->findSchemaRoot()->processEvent("create", pNewEnvNode);

This comment has been minimized.

Copy link
@afishbeck

afishbeck Apr 24, 2018

Member

Wonder about the name of these events being "create". Can you explain the meaning of create... when the comment above makes it sound like an "added".

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

The comment is informing the reader that the create event is not sent until after it has been added to the environment. The new node was "created", hence the event naming, but I just wanted to be clear that the event wasn't being processed until after adding it to the environment.

if (insertLimitType == "attribute")
{
std::string attributeName = pSchemaItem->getProperty("insertLimitData");
std::shared_ptr<SchemaValue> pSchemaValue = pSchemaItem->getAttribute(attributeName);

This comment has been minimized.

Copy link
@afishbeck

afishbeck Apr 24, 2018

Member

Again lots of variables that I think probably could be unique_ptr.

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

These methods return shared pointers to existing objects.

#include "SchemaItem.hpp"
#include <vector>

struct InsertItemLimitChoice

This comment has been minimized.

Copy link
@afishbeck

afishbeck Apr 24, 2018

Member

Do you use struct just to avoid a "public:" declaration?

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

I suppose yes. It has been a style I have used that when a small simply all public class is necessary, I just use a struct to indicate that it's all public. I also use it when instantiated objects are treated more like data, such as items in a vector.

@@ -27,4 +27,4 @@ class Cws_config2SoapBindingEx : public Cws_config2SoapBinding


private:
};
};

This comment has been minimized.

Copy link
@afishbeck

afishbeck Apr 24, 2018

Member

missing carriage return.

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Fixed.

@afishbeck

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

@stuartort someone else should also review. Especially details of c++ and std library usage.

@rpastrana
Copy link
Member

left a comment

@kenrowland several comments/questions. Noticed commented out code throughout. Remove 190env file


bool CreateEnvironmentEvent::handleEvent(const std::string &eventType, std::shared_ptr<EnvironmentNode> pEventNode)
{
return pEventNode->getSchemaItem()->getItemType() == m_itemType;

This comment has been minimized.

Copy link
@rpastrana

rpastrana Apr 24, 2018

Member

are you guaranteed to get non null from getSchemaItem?

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Yes, all nodes in the environment have a schema item, even if no schema is defined a default is created and assigned.

};


class CreateEnvironmentEvent : public EnvironmentEvent

This comment has been minimized.

Copy link
@rpastrana

rpastrana Apr 24, 2018

Member

this class name sounds like a method name, is this an EnvironmentEventCreator?

This comment has been minimized.

Copy link
@afishbeck

afishbeck Apr 24, 2018

Member

Or an EnvironmentCreateEvent?

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

The classes are handlers for events that occur in the environment. Perhaps adding 'Handler' to the class names is appropriate. Classes refactored with new naming convention.

@@ -0,0 +1,97 @@
/*##############################################################################
HPCC SYSTEMS software Copyright (C) 2018 HPCC Systems�.

This comment has been minimized.

Copy link
@rpastrana

rpastrana Apr 24, 2018

Member

second to last character seems invalid

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Fixed.

#include "Exceptions.hpp"


void EnvironmenLoader::addPath(const std::shared_ptr<EnvironmentNode> pNode)

This comment has been minimized.

Copy link
@rpastrana

rpastrana Apr 24, 2018

Member

I can't find where this class is declared (note: the name is missing a 'T'). Is this file being compiled?

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

It is no longer in use. It was not removed when it should have been based on some refactoring. It has been removed.

protected:

std::shared_ptr<SchemaItem> m_pSchemaItem;
//std::shared_ptr<EnvironmentNode> m_pEnvNode;

This comment has been minimized.

Copy link
@rpastrana

rpastrana Apr 24, 2018

Member

As mentioned above, let's clean up commented out code

ESPrequest FindNodesRequest
{
string sessionId;
string startingNodeId(""); // optional stataring node for the find, "" for none

This comment has been minimized.

Copy link
@rpastrana

rpastrana Apr 24, 2018

Member

spelling

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Fixed.

@@ -310,6 +329,20 @@ ESPresponse [exceptions_inline] GetTreeResponse
};


ESPrequest FindNodesRequest

This comment has been minimized.

Copy link
@rpastrana

rpastrana Apr 24, 2018

Member

It might be more common in hpcc code to call this FetchNodesRequest

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Unless there is a compelling reason, I will leave this as "find". A change to fetch will percolate all the way through the config manager.

ESPMethod
[
description("Find nodes matching the indicated path")
] FindNodes(FindNodesRequest, FindNodesResponse);

This comment has been minimized.

Copy link
@rpastrana

rpastrana Apr 24, 2018

Member

FetchNodes?

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

See previous comment. Leaving as find to keep same paradigm throughout all of config manager.

@@ -0,0 +1,1111 @@
<?xml version="1.0" encoding="utf-8"?>

This comment has been minimized.

Copy link
@rpastrana

rpastrana Apr 24, 2018

Member

what's this file used for? is it here for testing?

@@ -0,0 +1,6093 @@
<?xml version="1.0" encoding="UTF-8"?>
<!-- Edited with ConfigMgr on ip on 2018-03-23T18:29:29 -->

This comment has been minimized.

Copy link
@rpastrana

rpastrana Apr 24, 2018

Member

This file seems out of place in this folder, it prob shouldn't be checked in at all

@kenrowland kenrowland force-pushed the kenrowland:HPCC-19216 branch 2 times, most recently from c22fc58 to b8806f1 Apr 24, 2018

@afishbeck

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

@kenrowland if you wait until the end of code review to rebase it will make things much easier for the reviewers.

@kenrowland
Copy link
Contributor Author

left a comment

Comments addressed. New commit will follow shortly.


bool CreateEnvironmentEvent::handleEvent(const std::string &eventType, std::shared_ptr<EnvironmentNode> pEventNode)
{
return pEventNode->getSchemaItem()->getItemType() == m_itemType;

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Yes, all nodes in the environment have a schema item, even if no schema is defined a default is created and assigned.

@@ -0,0 +1,97 @@
/*##############################################################################
HPCC SYSTEMS software Copyright (C) 2018 HPCC Systems�.

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Fixed.

};


class CreateEnvironmentEvent : public EnvironmentEvent

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

The classes are handlers for events that occur in the environment. Perhaps adding 'Handler' to the class names is appropriate. Classes refactored with new naming convention.

#include "Exceptions.hpp"


void EnvironmenLoader::addPath(const std::shared_ptr<EnvironmentNode> pNode)

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

It is no longer in use. It was not removed when it should have been based on some refactoring. It has been removed.


return pNewEnvNode;

/*pParentNode->addChild(pNewEnvNode);

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Once again, code that had been refactored. Commented section removed.

}


void XMLEnvironmentLoader::parse(const pt::ptree &envTree, const std::shared_ptr<SchemaItem> &pConfigItem, std::shared_ptr<EnvironmentNode> &pEnvNode) const

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

The intent is that you use this method when you have an instance of the object, not to be able to call it otherwise. In the future there could be use of class member variables.

@@ -310,6 +329,20 @@ ESPresponse [exceptions_inline] GetTreeResponse
};


ESPrequest FindNodesRequest

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Unless there is a compelling reason, I will leave this as "find". A change to fetch will percolate all the way through the config manager.

ESPrequest FindNodesRequest
{
string sessionId;
string startingNodeId(""); // optional stataring node for the find, "" for none

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Fixed.

ESPMethod
[
description("Find nodes matching the indicated path")
] FindNodes(FindNodesRequest, FindNodesResponse);

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

See previous comment. Leaving as find to keep same paradigm throughout all of config manager.

@@ -27,4 +27,4 @@ class Cws_config2SoapBindingEx : public Cws_config2SoapBinding


private:
};
};

This comment has been minimized.

Copy link
@kenrowland

kenrowland Apr 26, 2018

Author Contributor

Fixed.

@kenrowland kenrowland force-pushed the kenrowland:HPCC-19216 branch from 47aee9f to e104667 Apr 26, 2018

@kenrowland

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

@afishbeck , @rpastrana
Changes have been made. Please review latest commit.

Thanks.

@rpastrana
Copy link
Member

left a comment

@kenrowland one minor issue

@@ -1,6 +1,6 @@
/*##############################################################################
HPCC SYSTEMS software Copyright (C) 2018 HPCC Systems®.
HPCC SYSTEMS software Copyright (C) 2018 HPCC Systems.

This comment has been minimized.

Copy link
@rpastrana

rpastrana Apr 30, 2018

Member

this cr symbol looks odd

This comment has been minimized.

Copy link
@kenrowland

kenrowland May 1, 2018

Author Contributor

Fixed. I believe the issue is when I create a file in Windows, and push it. I think the character is different between Linux and Windows.

@rpastrana

This comment has been minimized.

Copy link
Member

commented May 1, 2018

Sending back to @kenrowland

@richardkchapman

This comment has been minimized.

Copy link
Member

commented May 2, 2018

@rpastrana Are you ok with this now? If so, will need squashing

@rpastrana

This comment has been minimized.

Copy link
Member

commented May 2, 2018

@kenrowland I just noticed the name change didn't happen, let's stick with the more standard hpcc conventions (fetch vs find).

@kenrowland

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2018

@rpastrana Renamed APIs to FetchXXX. Please review and approve.
@afishbeck Please approve if no further comments

@afishbeck
Copy link
Member

left a comment

@kenrowland looks ok

@kenrowland

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2018

@rpastrana If no further comments, please approve. Thanks

@rpastrana

This comment has been minimized.

Copy link
Member

commented May 3, 2018

@kenrowland latest commit looks reasonable, you can squash your commits.

HPCC-19216 Dynamically add <Properties> section from buildset
Add support to dynamically add <Properties> sections to
ESPService and ESPBinding sections in the environment

Signed-off-by: Ken Rowland <kenneth.rowland@lexisnexisrisk.com>

@kenrowland kenrowland force-pushed the kenrowland:HPCC-19216 branch from bd7f7ee to 67b5147 May 3, 2018

@kenrowland

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2018

@richardkchapman Please merge

@richardkchapman richardkchapman merged commit c0b91a7 into hpcc-systems:master May 3, 2018

@HPCCSmoketest

This comment has been minimized.

Copy link
Contributor

commented May 3, 2018

Automated Smoketest:
OS: centos 7.2.1511 (Linux 3.10.0-327.28.3.el7.x86_64)
Sha: 67b5147
Build: success
Install hpccsystems-platform-community_6.5.0-trunk0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test total passed failed errors timeout
unittest 88 88 0 0 0
wutoolTest(Dali) 19 19 0 0 0
wutoolTest(Cassandra) 19 19 0 0 0

Regression test result:

phase total pass fail
setup (hthor) 11 11 0
setup (thor) 11 11 0
setup (roxie) 11 11 0
test (hthor) 784 784 0
test (thor) 715 715 0
test (roxie) 828 828 0

HPCC Stop: OK
HPCC Uninstall: OK
Time stats:

Prep time Build time Package time Install time Start time Test time Stop time Summary
5 sec (00:00:05) 188 sec (00:03:08) 56 sec (00:00:56) 6 sec (00:00:06) 21 sec (00:00:21) 1171 sec (00:19:31) 17 sec (00:00:17) 1464 sec (00:24:24)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.