Use options=count when getting entities #1251

crbrox · 2015-09-18T10:18:38Z

Use options=count when getting entities, closes #1041

kzangeli · 2015-09-18T10:23:18Z

src/lib/serviceRoutinesV2/getEntities.cpp

@@ -70,6 +70,11 @@ std::string getEntities
  string    idPattern  = ciP->uriParam["idPattern"];
  string    q          = ciP->uriParam["q"];

+  if (ciP->uriParam["options"] == "count")


Not sure this is enough ....

What if options == 'xxx,count', or 'count,yyy' ?

I think there is a vector somewhere, to do something similar to:

if (ciP->uriParamOptions['count'] == "true") { ... }

And, if there isn't, we'll have to implement it ...

Found it :-)

uriParamOptionsParse() in ConnectionInfo.cpp is called by uriArgumentGet() in rest/rest.cpp.
There, all options in the URI param 'options' are put in the vector ciP->uriParamOptions.

So, I would propose to use

if (ciP->uriParamOptions["count"] == true)

wherever needed, and probably remove these four lines.
[ It is a little ugly also to add parameters to ciP->uriParam ... it's like lying ... ]

I thought we were doing only one step after another one, I followed getEntityAllTypes
https://github.com/telefonicaid/fiware-orion/blob/develop/src/lib/serviceRoutinesV2/getEntityAllTypes.cpp#L59
which has an options=value possibility in api doc. As said in your PR #1171
"Good enough for now, until options itself is implemented.
options=values is not implemented."

In postQueryContext we have the following line:

if ((ciP->apiVersion == "v2") && (ciP->uriParam["count"] == "true"))

Now, as there are yet another way to ask for count, we'd need to add the following code:

if ((ciP->apiVersion == "v2") && (ciP->uriParamOptions["count"] == true)) { countP = &count; }

With this, it's all taken care of in postQueryContext(), so no code needed in getEntities()

OK, using uriParamOptions

In that case, I think reference to #958 inside #1041 can be misleading. I would change the text for this self PR also

Yes, probably is misleanding, sorry...

What I tried to mean in #1041 is that in PR #958 "get all entities" operation was implemented using count=true and that implementation needs to be fixed now to use count option (not exclusively: I mean, it could happen that other operations appar from "get all entities" are currently using count=true, in that case, they also need to be fixed to use count option).

Bottom line: at the end only count option will be the valid way of getting the count in any NGSIv2 operation in which counting makes sense and this PR should address that (of course, only for currently implemented operations, e.g. list subscription is not).

About "Good enough for now" ...
That statement comes from before ciP->uriParamOptions was implemented.

But ... now it is implemented. That changes everything, right?

And of course, we need to fix those older 'good enough for then' to use uriParamOptions. It is not good enough any more.

I just added this to issue #1041

Well, that comment was before I read your message about your "found" of the uriParamOptionsParse, which you don't remember if existed. I think both comments have crossed. Obviously, if the options param is parsed it should be used the parsed form. The strange thing is not to use the parse function before implementing the version in getEntitiesAllTypes being all commits so close in time, both on Aug 14. (a3fa6aa, 38fb8ae) (not such older the 'raw' use of options)

The original issue #1041 talked about changing from count=trueto options=count(in getEntities, could be understood) so the use in getEntitesAllTypes looked like a similar issue. It did not talk about allowing multiple values for options (which, I think, is not supported anywhere in the code and it's a orthogonal issue to changing count=true->options=count). So the quote of "good enough" wanted to express that this issue was not the best place to implement the parsing of options param, perhaps, in case it had to do from zero (as you thought for a3fa6aa)

I think changing the code for using the parsed form of options should be an issue by itself. As you wrote, it is not only for "count" but "anything" and the original issue was not related to things like "text" or "values" or any other options value. I would use the parsed form for 'options` in this issue, maybe for all cases of "count=true" in the code but I think it will break the "integrity" of the PR if we changed a lot of different unrelated, in other way, files. Strictly speaking the change in getEntitesAllTypes should not be covered by this issue, but by another issue for refactoring of "options" param use

Fixed ~~5c9ef1a41b35af8e13c0f49166970700ee632d4~~
Fixed 5bbfd1d

crbrox · 2015-09-21T09:40:07Z

Ready for final review/approval
@fgalan
@kzangeli

kzangeli · 2015-09-21T09:43:16Z

LGTM

fgalan · 2015-09-21T09:53:03Z

test/functionalTest/cases/0947_v2_list_all_entities/GET_v2_entities_pagination.test

@@ -35,7 +35,7 @@ brokerStart CB
 # 03. POST /v2/entities, created third  entity
 # 04. POST /v2/entities, created fourth entity
 # 05. POST /v2/entities, created fifth  entity
-# 06. GET  /v2/entities?limit=2&count=true
+# 06. GET  /v2/entities?limit=2&options=count,normalized


Probably 'normalized' will not be used at the end after the end, but it is ok to use it in this test as "random" option to check that the ability to mix count with other options works.

NTC

fgalan · 2015-09-21T10:26:32Z

LGTM

…options_count Use options=count when getting entities

kzangeli reviewed Sep 18, 2015
View reviewed changes

crbrox added 2 commits September 21, 2015 11:33

Use options=count when getting entities

e7f689d

Use postQueyContext for options=count

5bbfd1d

crbrox force-pushed the feature/1041_get_entities_options_count branch from 5c9ef1a to 5bbfd1d Compare September 21, 2015 09:35

fgalan reviewed Sep 21, 2015
View reviewed changes

Refactor get types to use options param map

9a4b5af

fgalan pushed a commit that referenced this pull request Sep 21, 2015

Merge pull request #1251 from telefonicaid/feature/1041_get_entities_…

43a6aaf

…options_count Use options=count when getting entities

fgalan merged commit 43a6aaf into develop Sep 21, 2015

fgalan deleted the feature/1041_get_entities_options_count branch September 21, 2015 10:26

fgalan mentioned this pull request Sep 21, 2015

options=count #1041

Closed

fgalan added this to the 0.25.0 milestone Sep 21, 2015

crbrox restored the feature/1041_get_entities_options_count branch September 24, 2015 09:50

fgalan deleted the feature/1041_get_entities_options_count branch September 26, 2015 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use options=count when getting entities #1251

Use options=count when getting entities #1251

crbrox commented Sep 18, 2015

kzangeli Sep 18, 2015

kzangeli Sep 18, 2015

crbrox Sep 18, 2015

kzangeli Sep 18, 2015

crbrox Sep 18, 2015

crbrox Sep 18, 2015

fgalan Sep 18, 2015

kzangeli Sep 18, 2015

crbrox Sep 18, 2015

crbrox Sep 21, 2015

crbrox commented Sep 21, 2015

kzangeli commented Sep 21, 2015

fgalan Sep 21, 2015

fgalan commented Sep 21, 2015

Use options=count when getting entities #1251

Use options=count when getting entities #1251

Conversation

crbrox commented Sep 18, 2015

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

crbrox commented Sep 21, 2015

kzangeli commented Sep 21, 2015

Choose a reason for hiding this comment

fgalan commented Sep 21, 2015