Hardening/3109 url parse refactor #20

kzangeli · 2018-02-28T21:11:17Z

Part 1 of refactoring of the url parse

…tine

fgalan · 2018-03-06T08:04:58Z

src/app/contextBroker/contextBroker.cpp

@@ -783,495 +783,393 @@ static const char* validLogLevels[] =



I undesrtand this is the first of a series of PRs that will end with telefonicaid#3109 step 1 covered. Could you elaborate on your "PR plan" (I mean, the topic for the next PRs after this one)? Maybe the two-levels branch approach (feature/ and task/ branches) should be applied if this is going be a long work?

No real reason to do all this work in a separate branch, pretty much only me working on the broker anyway ... :-)
It is not a problem to me to finish every PR leaving the broker in a 100% working state.
There is nothing about "half-working functionalities" here, only refactors.
Doing it in a feature branch would only add extra work.

The next step is to remove the 'payloadWord' from the RestService vector and after that more simplifications of the vector

In that case, then CHANGES_NEXT_RELEASE entry should be "inaugurated" in the first PR addressing this, eg.:

- Hardening: refactor request routing logic (#3109, step 1)

or a similar wording.

There is no impact on the usage of the broker ...
Normally, when we refactor stuff we don't mention it in the CNR.
The only thing users will notice is that the broker is faster after these changes ...

I agree there is no impact for the user from a functional perspetive, but even in that case it would be interesing to mark it in the changelog (specially when there could be a potentiall impact, non-functional, in the form of performance increase). That's way we use Hardening as prefix instead of Add or Fix. It doesn't hurt and may be useful.

Note we have already done this in the past, eg:

- Hardening: NGSIv1 responses don't use pretty-print JSON format any longer, as NGSIv2 responses work (potentially saving around 50% size) (#2760)

Fixed in 74c7a9c

fgalan · 2018-03-06T08:06:00Z

src/app/contextBroker/contextBroker.cpp

@@ -783,495 +783,393 @@ static const char* validLogLevels[] =



-#define API_V2                                                                                         \


As far as I understand, the change in contextBroker.cpp consists on spliting the global routes vector is a per-verb vector.

Is my intepretation correct? Any other change that I may be missing?

Maybe for the same price these loooong vectors could be moved out of contextBroker.cpp (to some new .h or something)

Yes, the PR is all about removing the VERB/METHOD from the vector and splitting the ONE vector into X vectors, one per verb (the last one is special, the one I call badVerbV, containing the service routines where no adequate verb/method has been found)

Sure, I could move the vector to another CPP file (and its header), but ... what do we gain with that?
Just more complexity, in my opinion

I think is more clear to have all routes in the same place, not mixed with the rest of logic of the contextBroker.cpp (initialization functions, etc.).

Mixed?
Not sure what you mean with that ...
The service routine vectors are where they have always been, right where they should be and right where they are used.
It's been like this since 2011, so ... very hard to understand why we should move them out to a different file (where they are not used) now ...
The vectors are needed in the call to restInit(), that will stay inside contextBroker.cpp.
I see no reason whatsoever to create another file only with the service vectors.

I wouldn't use the "it has been that way from a time long" argument because if we follow it, then even the refactor done by this PR could be put on doubt :)

Routes configuration is pretty much static and could be considered part of the rest library, so the movement could make sense, changing the way restInit() works, i.e. restInit() wouldn't pass the vector as arguments (as much it would select the mode to use, e.g. cors, no-cors, etc.).

However, it is not so important at the present moment, so we can keep it as it is now and think again in the future.

NTC

If we do the refactoring mentioned in this PR, about having restInit less "flexible", which we needed yeara ago but no longer, then the service vectors would go into the rest library, no doubt.
It's not a bad idea, let's do it. But, in a different PR ...

Maybe adding // FIXME PR: to be removed within #3109 refactor task would be useful. I think I will not forget about this but... never knows :)

Fixed in 74c7a9c

fgalan · 2018-03-06T08:08:45Z

src/lib/rest/RestService.cpp

+static RestService*              patchServiceV         = NULL;
+static RestService*              deleteServiceV        = NULL;
+static RestService*              optionsServiceV       = NULL;
+RestService*                     restBadVerbV          = NULL;


Why this one is not static? Why the name uses a different pattern (versus badVerbServiceV for instance)?

This vector is needed in another file (use grep to see which), and as it is not static, it needs a prefix.
As it is in the rest-library, I used the prefix "rest".
Perhaps restBadVerbServiceV would have been a better name. A bit long though

In the next-coming refactoring, all RestService vectors will be moved to RestService.cpp or rest.cpp, and then perhaps all of them can be made static ...

FIXME PR for this or NTC... As you prefer :)

NTC (I am preparing a list of nextcoming PRs about this)

fgalan · 2018-03-06T08:09:32Z

src/lib/rest/RestService.cpp

+*
+* serviceVectorsSet - only for unit tests
+*/
+void serviceVectorsSet


If only used in unit tests, then use #ifdef UNIT_TEST.

Actually, the function is used, by restInit() to set the vectors.
It wasn't like that in the beginning, that's why I added the comment.
But now the function is used both for the normal broker compilation as for the unit test compilation so ...

NTC

fgalan · 2018-03-06T08:23:26Z

src/lib/rest/RestService.cpp

@@ -445,7 +487,7 @@ static bool compErrorDetect
 *
 * restService -
 */
-std::string restService(ConnectionInfo* ciP, RestService* serviceV)
+static std::string restService(ConnectionInfo* ciP, RestService* serviceV, const char* serviceVectorName)


Where is the new parameter serviceVectorName being used in this function?

Maybe github diff is fooling me, but I cannot find any ocurrence in the diff page appart from this one...

Not easy to find this kind of things in the github jungle ...

% find . -name "*.cpp" -exec grep "restService(" {} /dev/null ./src/lib/rest/RestService.cpp:static std::string restService(ConnectionInfo* ciP, RestService* serviceV, const char* serviceVectorName) ./src/lib/rest/RestService.cpp: return restService(ciP, restBadVerbV, "BAD VERB"); ./src/lib/rest/RestService.cpp: if ((ciP->verb == GET) && (getServiceV != NULL)) return restService(ciP, getServiceV, "GET"); ./src/lib/rest/RestService.cpp: else if ((ciP->verb == POST) && (postServiceV != NULL)) return restService(ciP, postServiceV, "POST"); ./src/lib/rest/RestService.cpp: else if ((ciP->verb == PUT) && (putServiceV != NULL)) return restService(ciP, putServiceV, "PUT"); ./src/lib/rest/RestService.cpp: else if ((ciP->verb == PATCH) && (patchServiceV != NULL)) return restService(ciP, patchServiceV, "PATCH"); ./src/lib/rest/RestService.cpp: else if ((ciP->verb == DELETE) && (deleteServiceV != NULL)) return restService(ciP, deleteServiceV, "DELETE"); ./src/lib/rest/RestService.cpp: else if ((ciP->verb == OPTIONS) && (optionsServiceV != NULL)) return restService(ciP, optionsServiceV, "OPTIONS"); ./src/lib/rest/RestService.cpp: else return restService(ciP, restBadVerbV, "BADVERB");

If I'm not mistaken, I used the "serviceVectorName" fior debugging purposes.
Need to make sure, but if so, it will be removed.

The parameter is no longer used.
Removed in 64983d1

Fixed in 64983d1

fgalan · 2018-03-06T08:46:39Z

src/lib/rest/rest.cpp

  ipVersionUsed    = _ipVersion;
-  serveFunction    = (_serveFunction != NULL)? _serveFunction : serve;
+  serveFunction    = (_serveFunction != NULL)? _serveFunction : orionServe;


I have looked for usages of restInit() and it seems that we neever use _serveFunction parameter. Thus, maybe we should avoid the complexity of this function selection function and hardwire orionServer() function as routing logic.

This is old stuff from when orion was both ContextBroker and IotAgent and ConfiguratrionManager at the same time.
So, yes, it definitely could be removed, but that is another refactoring. restInit() will change completely etc, etc.
Not for this PR

Ok. I'd suggest to mark it with a // FIXME PR: to be removed within #3109 refactor task or a similar wording.

This has nothing to do with the current refactoring.
I could mark it with a FIXME saying something like "this might be refactored, or not, in the future" ... Really don't see the need of it though.
What would help here if we really want to make this change, is a new issue about it

Name changed from orionServe to orion::requestServe in 02d46a8

Next refactoring on my list is exactly this - move RestService vectors into rest lib. No FIXME needed ... Right?

Current situation is that there is an parameter, the one named _serveFunction in a function that is never used. I think that is a "weird" situation that worth the one-line of effort ;) that a FIXME PR require.

But you have more neurons on this than me right now, so as you prefer...

Sure, I can fix that. Almost easier than putting it in the issue ... :)

Fixed in 124c24a (the variable serveFunction is removed as well - which makes us have to modify images ...)

Images modified in eaa194d

fgalan · 2018-03-06T08:47:54Z

src/lib/rest/rest.h

+*
+* serve -
+*/
+extern std::string serve(ConnectionInfo* ciP);


After the change from

serveFunction = (_serveFunction != NULL)? _serveFunction : serve;

to

serveFunction = (_serveFunction != NULL)? _serveFunction : orionServe;

Is the serve() function actually used in some place? Should it be removed?

The serve()function changed name to orionServe(), that's all.
And yes, it is used :-)

Ok. Github diff is not always as smart as it should be with renamings... ;)

fgalan · 2018-03-06T08:48:45Z

src/lib/rest/rest.h

+*
+* serviceVectorsSet - only for unit tests
+*/
+extern void serviceVectorsSet


If only used in unit tests, then use #ifdef UNIT_TEST.

(Maybe I'm repeating myself too much... sorry :)

As mentioned, the comment is no longer valid.

NTC

fgalan · 2018-03-06T08:53:51Z

src/app/contextBroker/contextBroker.cpp

@@ -783,495 +783,393 @@ static const char* validLogLevels[] =





This PR impacts in development manual. At least, the following should be reviewed:

The libraries touched by this PR in https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/devel/sourceCode.md

The flows related with request processing, i.e. RQ-01 and RQ-02.

However, a grep -i ... *.md in the doc/manuals/devel with the name of the key functions being modified will help to locate other possible changes needed.

sourceCode.md modified in 7c82dd8.

RQ-01 and RQ-02 ... images?

ok, took a look at RQ-01 and 02.
Not sure why you mention these images. No changes to be made there.

NTC (after changes in sourceCode.md in commit 7c82dd8).

fgalan · 2018-03-06T09:00:03Z

src/lib/rest/rest.h

@@ -73,14 +73,23 @@ extern bool                    multitenant;
 extern bool                    corsEnabled;
 extern char                    corsOrigin[64];
 extern int                     corsMaxAge;
+extern RestService*            restBadVerbV;


Why this addition to global variables?

I answered before. The vector for bad verbs is needed in RestService.cpp

… rest.h

fgalan · 2018-03-09T08:08:59Z

src/lib/rest/RestService.h

+
+/* ****************************************************************************
+*
+* serviceVectorsSet - only for unit tests


It seems that at the end this function is not only used only for unit tests, as restInit() also uses it. Thus the "- only for unit tests" fragment should be removed.

(Same in the .ccp file)

Fixed in a265c79

fgalan · 2018-03-09T08:26:35Z

src/lib/rest/rest.h

+
+/* ****************************************************************************
+*
+* serviceVectorsSet - only for unit tests


Repeated declaration? (in seems is also at RestService.h)

Fixed in a265c79

fgalan · 2018-03-09T09:00:54Z

Except for some minor issues with serviceVectorsSet that should be reviewed, LGTM

kzangeli added 2 commits February 28, 2018 18:08

Important refactoring of parsing step of Verb/URL to pick service rou…

28c27c6

…tine

Pre PR fixes - changed name of serve() to orionServe()

3c30f33

fgalan requested changes Mar 6, 2018

View reviewed changes

kzangeli added 7 commits March 6, 2018 19:42

Renamed orionServe to orion::requestServe

02d46a8

Removed unused third parameter from restService. Removed 'serve' from…

64983d1

… rest.h

Modified sourceCode.md according to changes in RestService vectors

7c82dd8

PR fixes

74c7a9c

Removed serveFunction - now calling orion::requestServe() directly

124c24a

serveFunction replaced by orion::requestServe in sourceCode.md

41ad1ac

New images for the change serveFunction => orion::requestServe

eaa194d

fgalan reviewed Mar 9, 2018

View reviewed changes

3 serviceVectorsSet mistakes fixes

a265c79

kzangeli merged commit 11b856d into master Mar 10, 2018

kzangeli mentioned this pull request Mar 10, 2018

Hardening/3109 url parse refactor telefonicaid/fiware-orion#3128

Merged

		@@ -783,495 +783,393 @@ static const char* validLogLevels[] =

		@@ -783,495 +783,393 @@ static const char* validLogLevels[] =



		#define API_V2 \

Hardening/3109 url parse refactor #20

Hardening/3109 url parse refactor #20

Conversation

kzangeli commented Feb 28, 2018

fgalan Mar 6, 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

fgalan Mar 7, 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

kzangeli Mar 6, 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 Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzangeli Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

fgalan Mar 7, 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

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 Mar 9, 2018

fgalan Mar 6, 2018 •

edited

Loading

fgalan Mar 7, 2018 •

edited

Loading

kzangeli Mar 6, 2018 •

edited

Loading

fgalan Mar 6, 2018 •

edited

Loading

kzangeli Mar 7, 2018 •

edited

Loading

fgalan Mar 7, 2018 •

edited

Loading