Bug/2948 wrong max one service path response #3034

arigliano · 2017-11-06T16:30:26Z

This PR fixes #2948 Issue

It was implemented a new toJson() method in SubscriberError, in order to correctly print out the error response, depending on which serviceRoutine version calls either render() or toJson() method.
Fixed 400 httpStatusCode in both serviceRoutines

Implemented toJson() method in SubscribeContextResponse, SubscribeError and SubscriptionId

…into bug/2948_wrong_max_one_service_path_response

- Fixed HttpStatusCode of the response - New test for the fixed issue added - New entry in CHANGES_NEXT_RELEASE added

fgalan · 2017-11-06T16:45:05Z

src/lib/ngsi/SubscriptionId.cpp

+*/
+std::string SubscriptionId::toJson(RequestType container, bool comma)
+{
+  std::string xString = string;


We really choose a really bad name to store id string in Subscriptions.h... :)

NTC (just a bit surprised of my/our self :)

This was probably me, smells KZ ... :-)
I don't find it that bad though :-D

NTC

Just for fun... blame shows this:

Very ancient code, probably from first commit in public repo... The origin of this variable is lost in History :D

99% sure it's mine :-)

fgalan · 2017-11-06T16:46:05Z

src/lib/ngsi/SubscriptionId.h

@@ -47,6 +47,7 @@ typedef struct SubscriptionId
  const char*   c_str(void) const;
  bool          isEmpty(void);
  std::string   render(RequestType container,bool comma);
+  std::string   toJson(RequestType container,bool comma);


Style: whitespace after ,

(I understand you take render() as reference, but it is also wrongly styled... could you please fix that line also, pls?)

Fixed in 7480bca

fgalan · 2017-11-06T16:46:27Z

src/lib/ngsi10/SubscribeContextResponse.cpp

+*/
+std::string SubscribeContextResponse::toJson(void)
+{
+  std::string out     = "";


Style: excesive whitepace here?

[ Depends on what comes after. The '=' may be aligned with the '=' on the following line. If not, then yes ]

Correct. In this case neither L66 (above) or L68 (below) has assignation stamente, thus my comment about excesive whitepace.

Fixed in 7480bca

fgalan · 2017-11-06T16:49:54Z

Good work! Only minor comments.

Btw, do the branch in this PR pass make test and make style_check?

kzangeli · 2017-11-06T17:12:16Z

src/lib/ngsi/SubscribeError.cpp

+    }
+    out += ",";
+    out += JSON_PROP("affectedItems") + "[" + JSON_STR(subscriptionId.toJson(requestType, true)) + "]";
+ }


Indentation (one space seems to be missing)

Fixed in 7480bca

kzangeli · 2017-11-06T17:12:25Z

src/lib/ngsi/SubscribeError.cpp

+           (subscriptionId.get() != "000000000000000000000000") &&
+           (subscriptionId.get() != ""))
+  {
+	out += ",";


Indentation

Fixed in 7480bca

kzangeli · 2017-11-06T17:15:54Z

...Test/cases/2948_wrong_max_one_service_path_response/wrong_max_one_service_path_response.test

+
+#
+# 01. Create subscription for E1 with multiple service paths header
+


This empty line to be removed

Fixed in 7480bca

kzangeli · 2017-11-06T17:16:22Z

...Test/cases/2948_wrong_max_one_service_path_response/wrong_max_one_service_path_response.test

+#
+
+echo "01. Create subscription for E1 with multiple service paths header"
+echo "=============================================================="


A few more '='s, to be as long as the preceding line

Also, the description is not very correct.
Suggestion:

01. Attempt to create subscription for E1 with multiple service paths header, and see it fail

Fixed in 7480bca

kzangeli · 2017-11-06T17:40:09Z

What Fermin said, good work!

fgalan · 2017-11-14T10:12:06Z

@arigliano no news on this PR since a week ago... Are the feedback comments clear? Do you need any extra clarification? Don't hesitate to ask if you need, please.

…into bug/2948_wrong_max_one_service_path_response

arigliano · 2017-11-15T11:39:28Z

Sorry for the late. With 7480bca commit, i've fixed the style issues.

make test passed
make style_check gives me a lot of errors regarding code parts that I did not touch.
Starting in verbose mode,make style_check_v gives me the following:

style_check.sh: 111032 lines in 682 files in dir

0001 errors of category 'Do not leave a blank line after "public:"'
0001 errors of category 'Found C++ system header after other header'
0002 errors of category 'Extra space after ( in function call'
0003 errors of category 'Found C system header after other header'
0003 errors of category 'Missing space before ('
0003 errors of category 'Streams are highly discouraged'
0004 errors of category 'Single-argument constructors should be marked explicit'
0005 errors of category 'Line contains only semicolon'
0005 errors of category 'Missing spaces around ='
0006 errors of category 'Closing ) should be moved to the previous line'
0006 errors of category 'Include the directory when naming'
0008 errors of category 'Should have a space between // and comment'
0009 errors of category 'already included at'
0010 errors of category 'More than one command on the same line'
0011 errors of category 'Found C system header after C++ system header'
0013 errors of category '#endif line should be'
0013 errors of category '#ifndef header guard has wrong style'
0015 errors of category 'Do not use namespace using-directives'
0019 errors of category 'Blank line at the start of a code block'
0027 errors of category 'Missing space after ,'
0033 errors of category 'Labels should always be indented at least one space'
0038 errors of category 'Blank line at the end of a code block'
0044 errors of category 'Is this a non-const reference'
0045 errors of category 'Add #include'
0047 errors of category 'At least two spaces is best between code and comments'
0108 errors of category 'Tab found; better to use spaces'
0120 errors of category 'Extra space before ( in function call'
0133 errors of category 'Use int16/int64/etc'
0180 errors of category 'Line ends in whitespace'
0213 errors of category 'Weird number of spaces at line-start'
0249 errors of category 'Lines should be <= 120 characters long'

New style-guide errors:
-----------------------------------------------------
src/lib/common/JsonHelper.cpp:77: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] src/lib/common/string.cpp:433: Complex multi-line /*...*/-style comment found. Lint may give bogus warnings. Consider replacing these with //-style comments, with #if 0...#endif, or with more clearly structured multi-line comments. [readability/multiline_comment] [5] src/lib/common/string.cpp:1055: Consider using gmtime_r(...) instead of gmtime(...) for improved thread safety. [runtime/threadsafe_fn] [2] src/lib/common/tag.cpp:191: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] src/lib/ngsiNotify/Notifier.cpp:1: Complex multi-line /*...*/-style comment found. Lint may give bogus warnings. Consider replacing these with //-style comments, with #if 0...#endif, or with more clearly structured multi-line comments. [readability/multiline_comment] [5] src/lib/ngsiNotify/Notifier.cpp:0: One or more unexpected \r (^M) found;better to use only a \n [whitespace/newline] [1] src/lib/rest/httpRequestSend.cpp:0: One or more unexpected \r (^M) found;better to use only a \n [whitespace/newline] [1]
-----------------------------------------------------
style_check.sh: found 1381 errors (982 mayor, 392 minor errors) in 111032 lines of source code in 682 files (1.24% style-guide-incompatibilities)

I don't know if it is expected, but the errors in the part of code that I have modified are no longer there

kzangeli · 2017-11-15T12:16:59Z

Try 'make style'.
Speaking from memory: I think you are executing style check on the entire code, all dirs.
Not all dirs are prepared still. Another make target, called 'style' (I think) uses a special script called 'style check from makefile'. Well, something like that :)

arigliano · 2017-11-15T14:12:57Z

Ok thanks. I've performed '''make style''' and everything passed with no mayor error.

fgalan · 2017-11-15T14:36:11Z

LGTM. Thx!

Passing the ball to @kzangeli for final LGTM

kzangeli · 2017-11-15T18:50:41Z

LGTM

arigliano added 3 commits October 31, 2017 17:10

Fixes Issue telefonicaid#2948 - Wrong "max one service-path..." Response

fd41125

Implemented toJson() method in SubscribeContextResponse, SubscribeError and SubscriptionId

Merge branch 'master' of https://github.com/telefonicaid/fiware-orion …

8d81141

…into bug/2948_wrong_max_one_service_path_response

FIX Issue telefonicaid#2948 - Update

70855cb

- Fixed HttpStatusCode of the response - New test for the fixed issue added - New entry in CHANGES_NEXT_RELEASE added

fgalan mentioned this pull request Nov 6, 2017

Wrong "max one service-path allowed for subscriptions" in NGSIv2 subscription operation #2948

Closed

fgalan reviewed Nov 6, 2017

View reviewed changes

kzangeli reviewed Nov 6, 2017

View reviewed changes

Ermanno Mastrosimone and others added 2 commits November 15, 2017 10:26

Merge branch 'master' of https://github.com/telefonicaid/fiware-orion …

5a81b4d

…into bug/2948_wrong_max_one_service_path_response

Style fixed

7480bca

fgalan merged commit 59b051c into telefonicaid:master Nov 15, 2017

fgalan mentioned this pull request Nov 16, 2017

FIX broken 0675 test #3041

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/2948 wrong max one service path response #3034

Bug/2948 wrong max one service path response #3034

arigliano commented Nov 6, 2017

fgalan Nov 6, 2017

kzangeli Nov 6, 2017

fgalan Nov 6, 2017

kzangeli Nov 6, 2017

fgalan Nov 6, 2017

arigliano Nov 15, 2017

fgalan Nov 6, 2017

kzangeli Nov 6, 2017

fgalan Nov 6, 2017

arigliano Nov 15, 2017

fgalan commented Nov 6, 2017

kzangeli Nov 6, 2017

arigliano Nov 15, 2017

kzangeli Nov 6, 2017

arigliano Nov 15, 2017

kzangeli Nov 6, 2017

arigliano Nov 15, 2017

kzangeli Nov 6, 2017

kzangeli Nov 6, 2017 •

edited

Loading

arigliano Nov 15, 2017

kzangeli commented Nov 6, 2017

fgalan commented Nov 14, 2017 •

edited

Loading

arigliano commented Nov 15, 2017

kzangeli commented Nov 15, 2017

arigliano commented Nov 15, 2017

fgalan commented Nov 15, 2017

kzangeli commented Nov 15, 2017


		#
		# 01. Create subscription for E1 with multiple service paths header

Bug/2948 wrong max one service path response #3034

Bug/2948 wrong max one service path response #3034

Conversation

arigliano commented Nov 6, 2017

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

Choose a reason for hiding this comment

fgalan commented Nov 6, 2017

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

kzangeli Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzangeli commented Nov 6, 2017

fgalan commented Nov 14, 2017 • edited Loading

arigliano commented Nov 15, 2017

kzangeli commented Nov 15, 2017

arigliano commented Nov 15, 2017

fgalan commented Nov 15, 2017

kzangeli commented Nov 15, 2017

kzangeli Nov 6, 2017 •

edited

Loading

fgalan commented Nov 14, 2017 •

edited

Loading