Add JsonVectorHelper class and some related refactors #3329

fgalan · 2018-10-03T11:20:45Z

This PR is part of issue #1298 refactors and contains the following items:

Add a new class JsonVectorHelper, similar to current JsonHelper for objects
Ensure all NGSIv2 rendering logic (i.e. toJson() methods) are using the helpers. Removal of dead code and some fixes. After this change, no string concatenation is implemented in toJson() methods directly, all that is encapusulated in Json*Helper classes. Detail of the fixed methods (as far as I remember):
- NotifyContextRequeset::toJson()
- ContextElementResponseVector::toJson()
- EntityID::toJson()
- StatusCode::toJson()
- SubscribeError::toJson()
- SubscribeContextResponse::toJson()
- OrionError::toJson()
Avoid code duplication unifying toJsonString() and jsonIvalidCharsTransformation() code
Avoid + for std::string in Json*Helper clases (and some other placers), according to the recommendations of https://www.thecodingdelight.com/string-cplusplus/

I'd try to coduct a test with Apache Benchmark in order to see if item 4 provides some improvement, but PR review can be done in the meanwhile.

fgalan · 2018-10-03T11:29:29Z

src/lib/ngsi/StatusCode.cpp

 {
-  std::string  out  = "";


Lot of complexity removed here...

NTC

fgalan · 2018-10-03T11:29:40Z

src/lib/ngsi/SubscribeError.cpp

-

Lot of complexity removed here...

NTC

fgalan · 2018-10-03T11:30:03Z

src/lib/ngsi/SubscriptionId.cpp

-*
-* SubscriptionId::toJson -
-*/
-std::string SubscriptionId::toJson(RequestType container, bool comma)


Dead code. Mabye sometime is was used, but not at the present moment.

NTC

fgalan · 2018-10-03T11:32:07Z

...Test/cases/2763_postUpdateContext_error_return/postEntities_with_errors_in_service_path.test

@@ -53,14 +53,14 @@ echo
 01. Try to create an entity with more than one service path - see error
 =======================================================================
 HTTP/1.1 400 Bad Request
-Content-Length: 79
+Content-Length: 91
 Content-Type: application/json
 Fiware-Correlator: REGEX([0-9a-f\-]{36})
 Date: REGEX(.*)

 {


It seem we were wrongly using NGSIv1-like error response for POST /v2/entity. This PR has raised this and fixed (used to happen during cleanup refactors :)

A bit surprising not to found this in more places. I think it deservers a CNR line like this:

- Fix: NGSIv1-like error responses were used in some NGSIv2 operations

Fixed in edd01ff

fgalan · 2018-10-03T11:33:17Z

test/functionalTest/cases/3004_v2_reg_create/v2_reg_create.test

@@ -97,7 +97,7 @@ Date: REGEX(.*)
 02. GET the registration using APIv2
 ====================================
 HTTP/1.1 200 OK
-Content-Length: 312


Content length changes in this PR are due to httpInfo rendering has moved from http":{"url": "http://foo.bar"} to http":{"url":"http://foo.bar"} (1 less whitespace). Thus, the change in .test is fine.

NTC

fgalan · 2018-10-03T11:36:29Z

src/lib/common/JsonHelper.h

@@ -31,6 +31,8 @@
 #include <vector>
 #include <map>

+
+// FIXME P10: should be renamed to JsonObjectHelper


Two additional actions could be done:

Rename JsonHelper class to JsonObjectHelper class (I think this could be done in this same PR)

Split JsonHelper.h.cpp into JsonObjectHelper.h/cpp and JsonVectorHelper.h/cpp. I think this case is better to have them in only , as both classes are highly related and use some common logic such as toJsonString() function (which would force to have an additional couple jsonHelperCommon.h/cpp in the second case) and we have used this pattern in other places (e.g. Subscription related classes are all them in the same cpp/h module). However, I don't have an strong opinion on it so if you prefer the other way for me is ok and I can also implement it in this PR.

It seems that JsonHelper is an Object, by implementation.
What if the constructor would take a boolean for object/array ?
That ,might be a third option and instead of 2-3 classes we'd have only one

I can try with something as you propose in order to see how looks like. Probably the level of reutilizanton will not be too high (as the way to add a key-value, for object, is different for the way of adding just the value, for vector) but let's see. Let's also see how it looks like from the point of view of the "user code".

If at the end we go with just one class them, two comments:

Maybe the name JsonConteiner first better than JsonHelper, as the later is less precise ("helper" may mean many things :), "container" is more precise).

I think is more clear to use a meanifull type (a enum) instead of a bool to select the container type:

JsonContainer jco(jc::object); ... JsonContainer jcv(jc::vector);

(alternative usggestion for jh:: namespace are welcome)

At the end we will keep with two classes as it is now (discussed at skype).

The JsonHelper -> JsonObjectHelper renaming is pending.

Rename done in 33b9f6d

kzangeli · 2018-10-03T15:52:56Z

src/lib/apiTypesV2/Registration.cpp


-  jh.addRaw("http", urlAsJson);
+  JsonHelper   jh;
+  jh.addRaw("http", jhUrl.str());


Is JsonHelper only capable of maintaining one single object?
Otherwise, why would you need jhUrl ...

NTC (just trying to learn something)

Yes. Basically JsonHelper is an absraction for JSON objects (the same way new JsonHelper is an abstraction for JSON vectors) so the user doesn't have to deal with commas, ending items, brachets and so on and use add*() method to add things to the container.

A more powerfull approach would be to have only one constructor, passing in down all along the toJson() functions. We have discussed that in the past as a possible enhacement, but not for this PR :)

kzangeli · 2018-10-03T15:58:19Z

src/lib/common/JsonHelper.h

@@ -31,6 +31,8 @@
 #include <vector>
 #include <map>

+
+// FIXME P10: should be renamed to JsonObjectHelper


It seems that JsonHelper is an Object, by implementation.
What if the constructor would take a boolean for object/array ?
That ,might be a third option and instead of 2-3 classes we'd have only one

kzangeli · 2018-10-03T15:59:57Z

src/lib/common/JsonHelper.h

@@ -76,10 +98,12 @@ std::string vectorToJson(std::vector<T> &list)

  std::string ss;

-  ss += '[' + list[0].toJson();
+  ss += '[';


With my proposal, this line would be something like:

s += (isObject == true)? '{' : '[';

I'll take into account in the context of #3329 (comment)

At the end we will keep with two classes as it is now (discussed at skype).

So, NTC.

kzangeli · 2018-10-03T16:02:35Z

src/lib/ngsi/SubscribeError.cpp

-  }
-
-  return out;
+  return jh.str();


This we can do as we have deprecated API V1, I imagine ...
But, even deprecated, the broker still supports the requests, right?

I think so. What I think is happening is that NGSIv2 never renders this object. NGSIv1 still rendering it using the toJsonV1() method.

NTC (I guess)

If still used in NGSIv1, with this change, the NGSIv1 protocol is no longer followed.
That was my question, actually.
Not that I am very fond of NGSIv1 but, it is defined as it is ... with sub-id as mandatory field.

Or, am I wrong?

NGSIv1 protocol is no longer followed... in a place where doesn't need to follow. Note that here we are inside toJson() method that is only used by NGSIv2 logic.

NGSIv1 render logic is done in toJsonV1() that, as far as I understand, it still using this stuff.

(Maybe I haven't understood the question and my answer doesn't make sense :)

Ah, ok. My bad.
I misunderstood. Thought it was used for NGSIv1.
Then again ... SubscribeError is *very very * NGSIv1 so ... easy to make the mistake.
NTC

I think I understand now ... Finally!
It's a bad diff. The content that was for NGSIv1 seems removed but it has probably simply been moved to a toJsonV1 method.
NTC
NTC

kzangeli · 2018-10-03T16:08:19Z

src/lib/parse/CompoundValueNode.cpp

    break;

  case orion::ValueTypeObject:
+    // In thic case we cannot use JsonHelper to build the object, as we don't have a
+    // key-value sequence to invoke addXX() method


An empty JsonHelper would render as "{}", no?

Yes, but I cannot use the helper here. The way the helper works it needs both a key and value to invoke addXX():

addXX(key, value)

However, in this method I don't have a key because the child toJson() method I'm using returns the key along the value.

Maybe it can be solved with a deeper refactor of CompoundValue::toJson() but that's complex so I left out of this PR.

My point was that with an empty object, you wouldn't call any addXX, the loop would be empty and you would end up with '{}' ... Or not?

Not sure what you mean... you mean to use JsonObjectHelper in the case of no child? I mean, something like:

if (childV.size() == 0) { JsonObjectHelper jh; out = jh.str(); }

That could be use, but I think is better the current approach. However, I don't have a strong opinion on it, if you prefer to be changed to the above.

No, I mean to do nothing special in case of empty object, like this:

JsonHelper xxx; while (members) xxx.addMember() xxx.render();

If there are no members, nothing will be added and the render would give '{}' as output.
No need for special treatment of empty objects.

I might be wrong, perhaps there is some impediment in JsonHelper ... ?

Note that in this part of the code JsonHelper is not being used

Why JsonHelper is not being used? Due to:

we cannot use JsonHelper to build the object, as we don't have a key-value sequence to invoke addXX() method

Getting confused ... Sorry about that.
I wan't really interested in JsonHelper here, only the special treatment of empty objects. My bad.
Using code instead of words. More exact.

Something like this:

out = "{"; unsigned int ix; for (ix = 1; ix < childV.size(); ix++) { out += childV[0].toJson(false); if (ix != childV.size() - 1) out += ","; } out += "}";

No special treatment for 0 children.

Ok. I'll fix.

Fixed in 03e44cf

kzangeli · 2018-10-03T16:10:02Z

src/lib/parse/CompoundValueNode.cpp

+    std::string preOut = "\"";
+    preOut += toJsonString(name);
+    preOut += "\":";
+    return preOut + out;


Or:

preOut += out: return preOut;

If you really want to avoid ALL std::string operator+

Good catch!

Fixed in edd01ff

fgalan · 2018-10-04T13:39:53Z

I'd try to coduct a test with Apache Benchmark in order to see if item 4 provides some improvement, but PR review can be done in the meanwhile.

Comparing results from master and from this PR branch there is a slight throughput increase of 2.14% or 7.41% depending the case. Not bad.

kzangeli

LGTM

AlvaroVega

LGTM

fgalan added 4 commits October 2, 2018 10:09

ADD JsonVectorHelper utility class

f0c344f

FIX all toJson() methods using JsonHelper and JsonVectorHelper classes

c5ee8f8

FIX avoid string concatenation in JsonHelper classes

27b01a2

REFATOR unify jsonInvalidCharsTransformation() and toJsonString()

cb12786

fgalan mentioned this pull request Oct 3, 2018

JsonHelper usage in all NGSIv2 renders (second step: optimization and performance) #1298

Open

fgalan commented Oct 3, 2018

View reviewed changes

src/lib/ngsi/StatusCode.cpp

{

std::string out = "";

Copy link

Member Author

fgalan Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lot of complexity removed here...

NTC

fgalan commented Oct 3, 2018

View reviewed changes

src/lib/ngsi/SubscribeError.cpp

Copy link

Member Author

fgalan Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lot of complexity removed here...

NTC

fgalan commented Oct 3, 2018

View reviewed changes

kzangeli reviewed Oct 3, 2018

View reviewed changes

fgalan added 2 commits October 4, 2018 10:35

RENAME JsonHelper class -> JsonObjectHelper

33b9f6d

FIX issues raised in code review

edd01ff

FIX issue raised in code review

03e44cf

kzangeli approved these changes Oct 4, 2018

View reviewed changes

AlvaroVega approved these changes Oct 5, 2018

View reviewed changes

AlvaroVega merged commit ca2eac9 into master Oct 5, 2018

AlvaroVega deleted the hardening/1298_json_vector_helper branch October 5, 2018 07:33

Add JsonVectorHelper class and some related refactors #3329

Add JsonVectorHelper class and some related refactors #3329

Conversation

fgalan commented Oct 3, 2018

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 Oct 3, 2018 • edited Loading

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 Oct 3, 2018 • edited Loading

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

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 Oct 4, 2018 • edited Loading

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 Oct 4, 2018 • edited Loading

kzangeli left a comment

Choose a reason for hiding this comment

AlvaroVega left a comment

Choose a reason for hiding this comment

fgalan Oct 3, 2018 •

edited

Loading

fgalan Oct 3, 2018 •

edited

Loading

kzangeli Oct 4, 2018 •

edited

Loading

fgalan commented Oct 4, 2018 •

edited

Loading