MINIFICPP-710 - Fix errors in Property StringToInt conversion #472

arpadboda · 2019-01-10T17:16:57Z

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 MINIFICPP-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?
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.

arpadboda · 2019-01-10T17:17:37Z

libminifi/test/unit/PropertyTests.cpp

@@ -107,3 +107,28 @@ TEST_CASE("Test Trimmer Left", "[testTrims]") {
  REQUIRE(test.c_str()[1] == ' ');
 }

+TEST_CASE("Test int conversion", "[testStringToInt]") {


Most of these would fail without this change.

Almost all of these are invalid and shouldn't be accepted, though (not the changes -- the actual entries like - 2 GB ). This was the point of https://issues.apache.org/jira/browse/MINIFICPP-707. I don't want to trounce on this PR, but the reason I created the ticket was that some of these SHOULD be prevented from being used at all in configs, fail early. Therefore it's not as simple as doing what is done here. There needs to be more thought put into this, this was the primary reason I created the ticket versus implementing.

I didn't think my last comment was entirely helpful so I went through and tried to parse out the parts that are appropriate and SHOULD be accepted. In retrospect I think I was being a bit terse jumping between tasks. Sorry if that comment didn't provide much insight into my thought process.

Hopefully I was more clear on what we can immediately accept and what I think needs more discussion. I'm totally okay if that discussion yields pushing some of the concerns down the road if it makes sense.

arpadboda · 2019-01-10T17:18:11Z

libminifi/src/c2/protocols/RESTProtocol.cpp

@@ -129,7 +129,7 @@ const C2Payload RESTProtocol::parseJsonResponse(const C2Payload &payload, const
  } catch (...) {
  }
 #endif
-  return std::move(C2Payload(payload.getOperation(), state::UpdateState::READ_COMPLETE, true));


These generated copy-elision warnings in clang, so removed.

Good catch. I rarely build with clang these days...

phrocker

-1

phrocker · 2019-01-11T00:24:25Z

libminifi/test/unit/PropertyTests.cpp

+  REQUIRE(uint32_var == expected_value);
+
+  int32_t int32_var = 0;
+  REQUIRE(Property::StringToInt("3GB", int32_var) == false);  // Doesn't fit


I'll have to look at the changes more closely but we shouldn't be worrying about int32, we're almost exclusively uint64 and when we're not ( such as someone saying a type is int ), we should be okay up-converting. It's possible that someone sets a type, but I don't think we can say at this prospect if the type fits or doesn't fit

One of the things that the big PR left as a todo was to write tests with conversions, so I will readily admit that there may be some cleanup.

If we only aim for uint64, I think we should consider having this function only for uint_64t instead of the current template solution and provide possibility to safely cast for smaller types (like I did in AssignIfFits).

As this would introduce a breaking change, not sure when it can be done properly.

phrocker · 2019-01-11T00:24:58Z

libminifi/test/unit/PropertyTests.cpp

+  int32_t int32_var = 0;
+  REQUIRE(Property::StringToInt("3GB", int32_var) == false);  // Doesn't fit
+
+  REQUIRE(Property::StringToInt("-1 G", int32_var) == true);


We should almost exclusively not allow signed Byte values in string to int. It should always be positive.

Okay, thanks, will change.

phrocker · 2019-01-11T00:26:10Z

libminifi/test/unit/PropertyTests.cpp

+  REQUIRE(Property::StringToInt("-1 G", int32_var) == true);
+  REQUIRE(int32_var == -1 * 1000 * 1000 * 1000);
+
+  REQUIRE(Property::StringToInt("-1G", uint32_var) == false);  // Negative to uint


This shouldn't be allowed. Whether this barrier exists here or elsewhere is what I think we should discuss. It's about the end to end correctness, so not sure if the appropriate action is to fix the signedness issues or correct the overall issue that 707 was hoping to look into. I fear that resolving any issues like signed values without looking into solving the whole picture takes us to an unknown state. Thoughts?

Agreed, however as you see, it's not allowed, this conversion fails.

Although I think the current implementation is still more forgiving than it should be according to the whole picture, so let's try to explore that before going ahead with this.

phrocker · 2019-01-11T00:33:57Z

libminifi/include/core/TypedValues.h

+
+  //Signed
+  template<typename T, typename std::enable_if<std::is_signed<T>::value, T>::type* = nullptr>
+  static bool AssignIfFits(int64_t base, uint64_t mult, T& output) {


These I like and think we should use this.

phrocker · 2019-01-11T00:36:29Z

libminifi/include/core/TypedValues.h

-    auto ival = std::strtoll(cvalue, &pEnd, 0);
+
+    T ival = 0;
+    if(!StringToNum(cvalue, &pEnd, ival)) {


This is only applicable if we reach the null terminator and it is not a byte string

Not sure I get the point, this does the same strto(u)ll call inside that was here before.
The only change is the return value, which comes from AssignIfFits.

phrocker · 2019-01-11T00:38:21Z

libminifi/include/core/TypedValues.h

+  //Unsigned
+  template<typename T, typename std::enable_if<!std::is_signed<T>::value, T>::type* = nullptr>
+  static bool StringToNum(const char* str_begin, char **str_end, T& output) {
+    if(str_begin[0] == '-') {


can't assume that it's zero, and that it's non-empty

Good shout, thanks!
This also means adding a tectcase to check with empty and invalid strings to make sure it handles errors gracefully.

phrocker · 2019-01-11T00:38:37Z

libminifi/include/core/TypedValues.h

+    }
+    return false;
+  }
+


Does this code compile in windows? I'm not sure that it will.

Appveyor showed that it didn't. Do you have access to windows? I can probably help with that since I do.

Yes, please! I can check Appveyor err messages (those made me find the fix), but that's not so efficient in longer term.

phrocker · 2019-01-11T00:52:28Z

libminifi/test/unit/PropertyTests.cpp

+  REQUIRE(Property::StringToInt("-2", uint64_var) == false);  // Negative shouldn't be converted to uint
+
+  uint32_t uint32_var = 0;
+  REQUIRE(Property::StringToInt("3  GB", uint32_var) == true);  // Test skipping of spaces, too


Some of these tests exist in yaml if I recall correctly ( but I didn't write them ), if you're interested.

phrocker · 2019-01-11T00:56:30Z

libminifi/test/unit/PropertyTests.cpp

+  using org::apache::nifi::minifi::core::Property;
+
+  uint64_t uint64_var = 0;
+  REQUIRE(Property::StringToInt("-2", uint64_var) == false);  // Negative shouldn't be converted to uint


I realize 710 kind of had a different focus, but your comment on the prior PR in which you mentioned this was to open 707 in hopes of opening a discussion for better dealing with this. For example, it may be the case where sometimes a negative value makes sense AND the conversion make sense. I will do some research, as I know these cases exist. In others, that is NOT true.

While the above test is totally valid from the perspective of 710, the hope was that we could discuss and more intelligently handle these cases, perhaps with the validators themselves. What is your perspective?

Sure, let's focus on the long term solution.

Just scratched this as I spotted errors in the code, however as you correctly pointed out, my change doesn't include the integration approach.

phrocker · 2019-01-11T01:07:48Z

libminifi/include/core/TypedValues.h

+  template<typename T, typename std::enable_if<std::is_signed<T>::value, T>::type* = nullptr>
+  static bool StringToNum(const char* str_begin, char **str_end, T& output) {
+    int64_t val = std::strtoll(str_begin, str_end, 0);
+    return AssignIfFits<T>(val, 1, output);


why have any multiplication at all if it is 1? same with the one, below.

Just to reuse that code. The goal here was to make sure that the value just read fits target in case that's smaller than 64 bits.

szaszm · 2020-12-16T15:26:01Z

Closing this because of inactivity

arpadboda commented Jan 10, 2019

View reviewed changes

phrocker requested changes Jan 10, 2019

View reviewed changes

phrocker reviewed Jan 11, 2019

View reviewed changes

MINIFICPP-710 - Fix errors in Property StringToInt conversion

f97f943

arpadboda force-pushed the MINIFICPP-710 branch from 8a1db8e to f97f943 Compare January 20, 2019 16:21

szaszm closed this Dec 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINIFICPP-710 - Fix errors in Property StringToInt conversion #472

MINIFICPP-710 - Fix errors in Property StringToInt conversion #472

arpadboda commented Jan 10, 2019

arpadboda Jan 10, 2019

phrocker Jan 10, 2019 •

edited

phrocker Jan 11, 2019 •

edited

arpadboda Jan 10, 2019

phrocker Jan 11, 2019

phrocker left a comment

phrocker Jan 11, 2019

phrocker Jan 11, 2019

arpadboda Jan 12, 2019

phrocker Jan 11, 2019

arpadboda Jan 12, 2019

phrocker Jan 11, 2019 •

edited

arpadboda Jan 12, 2019

phrocker Jan 11, 2019

phrocker Jan 11, 2019

arpadboda Jan 12, 2019

phrocker Jan 11, 2019

arpadboda Jan 12, 2019

phrocker Jan 11, 2019

phrocker Jan 11, 2019

arpadboda Jan 12, 2019 •

edited

phrocker Jan 11, 2019

arpadboda Jan 11, 2019

phrocker Jan 11, 2019 •

edited

arpadboda Jan 12, 2019

phrocker Jan 11, 2019 •

edited

arpadboda Jan 11, 2019

szaszm commented Dec 16, 2020

MINIFICPP-710 - Fix errors in Property StringToInt conversion #472

MINIFICPP-710 - Fix errors in Property StringToInt conversion #472

Conversation

arpadboda commented Jan 10, 2019

For all changes:

For code changes:

For documentation related changes:

Note:

Choose a reason for hiding this comment

phrocker Jan 10, 2019 • edited

Choose a reason for hiding this comment

phrocker Jan 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phrocker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phrocker Jan 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arpadboda Jan 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phrocker Jan 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phrocker Jan 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szaszm commented Dec 16, 2020

phrocker Jan 10, 2019 •

edited

phrocker Jan 11, 2019 •

edited

phrocker Jan 11, 2019 •

edited

arpadboda Jan 12, 2019 •

edited

phrocker Jan 11, 2019 •

edited

phrocker Jan 11, 2019 •

edited