-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Task/3068 ngsiv2 update forwarding #3480
base: master
Are you sure you want to change the base?
Changes from all commits
2a9c04c
4e9f7de
5496aaf
53551dc
6502454
fc0f97d
5dfea53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
- Fix: idPattern '.*' is now allowed in NGSIv2 registrations (#3458) | ||
- Add: Forwarded queries work for registrations using idPattern == '.*', but only for simple cases (#3458) | ||
- Add: Forwarded updates work for registrations using idPattern == '.*', but only for simple cases (#3458) | ||
- Add: NGSIv2 forwards now work for queries and updates - for simple cases (#3068) | ||
- Fix: The 'Allow:' header for the service /v2/registrations had the incorrect value of "Allow: POST". The correct value is: "Allow: GET, POST" | ||
- Add: NGSIv2 forwards now works for queries - for simple cases (#3068) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ In NGSIv1 (deprecated), the request `POST /v1/updateContext` has a field called | |
* APPEND_STRICT | ||
* REPLACE | ||
|
||
> Side-node: The first three are "standard NGSIv1" while the second two were added for NGSIv2. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file modificaiton seems to be related with fixes in the query forward documentation, but the update forwared documetnation about (FW-02 if I'm remembering correctly) has not be modified. Probably an update of the png for FW-02 is needed also, but I haven't seen yet the changes in the code so I'm not fully sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you probably can see, the modifications in this file are just corrections. About documenting Update Forward, part of it is added, but most probably not all that is necessary |
||
> Side-note: The first three are "standard NGSIv1" while the second two were added for NGSIv2. | ||
|
||
* Requests with `UPDATE` or `REPLACE` may provoke forwarding of the request. | ||
Only if **not found locally but found in a registration**. | ||
|
@@ -89,9 +89,9 @@ The `QueryContextRequest` items are filled in based on the output of the [**mong | |
_FW-04: `queryForward()` function detail_ | ||
|
||
* Parse the context provider string to extract IP, port, URI path, etc. (step 1). | ||
* The request to forward has to be built (step 2). In the case of NGSIv1, we need to extract information of the binary object into text to be able to send the REST request (plain text) to the Context Provider using POST /v1/queryContext. In the case of NGSIv2, the use GET /v2/entities and no payload is required. | ||
* The request to forward has to be built (step 2). In the case of NGSIv1, we need to extract information of the binary object into text to be able to send the REST request (plain text) to the Context Provider using POST /v1/queryContext. In the case of NGSIv2, the use of GET /v2/entities (without payload) is required. | ||
* The request to forward is sent with the help of `httpRequestSend()` (step 3) which uses [libcurl](https://curl.haxx.se/libcurl/) to forward the request (step 4). libcurl sends in sequence the request to the Context Provider (step 5). | ||
* The textual response from the Context Provider is parsed and an `QueryContextResponse` object is created (step 6). Parsing details are provided in diagram [PP-01](jsonParse.md#flow-pp-01). | ||
* The textual response from the Context Provider is parsed and a `QueryContextResponse` object is created (step 6). Parsing details are provided in diagram [PP-01](jsonParse.md#flow-pp-01). | ||
|
||
## A Caveat about shadowing of entities | ||
The Context Provider mechanism is implemented using standard registration requests and this might lead to unwanted situations. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -335,8 +335,9 @@ for the following aspects: | |
directly. I.e., updates must be done deleting and re-creating the registration. Please | ||
see [this issue](https://github.com/telefonicaid/fiware-orion/issues/3007) about this. | ||
* `idPattern` is supported but only for the exact regular expression `.*` | ||
And, right now (2019-03-21), only forwarding of queries is working for registrations with idPatterns. | ||
forwarded updates do not work for registrations with idPattern (the plan is to fix this asap). | ||
And, right now (2019-04-24), forwarding for registrations with idPatterns works for queries using `GET /v2/entities` | ||
as well as updates using `PATCH /v2/entities/{Entity-ID}/attrs`. | ||
More requests will be added to this list. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think is better the old wording, without providing the detail of the particular operations. At least until the translation of cases/0787 test so we can have more insight in which operations are supported and which operations doesn't. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Until we have a clear path forward, let's wait with these kind of changes. No need to spend time before we are sure what we want exactly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... and if there is no need of spending time, why this file was modified? ;) I'd suggest to go back for its previous version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was following a perhaps unclear spec and this was my interpretation, a logical one by the way. |
||
* `typePattern` is not implemented. | ||
* The only valid `supportedForwardingMode` is `all`. Trying to use any other value will end | ||
in a 501 Not Implemented error response. Please | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2703,17 +2703,24 @@ static void searchContextProviders | |
std::string err; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any change in this file appart from LM_T LmtForward adddition? It seems so but I would like to have a confirmation from your side, pls. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems to be only LM_T in this file, yes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NTC |
||
|
||
/* Fill input data for registrationsQuery() */ | ||
LM_T(LmtForward, ("Entity Id: %s", en.id.c_str())); | ||
LM_T(LmtForward, ("Entity isPattern: %s", en.isPattern.c_str())); | ||
LM_T(LmtForward, ("Entity Type: %s", en.type.c_str())); | ||
|
||
enV.push_back(&en); | ||
for (unsigned int ix = 0; ix < caV.size(); ++ix) | ||
{ | ||
attrL.push_back(caV[ix]->name); | ||
LM_T(LmtForward, ("Attr %d: %s", ix, caV[ix]->name.c_str())); | ||
} | ||
|
||
/* First CPr lookup (in the case some CER is not found): looking in E-A registrations */ | ||
if (someContextElementNotFound(*cerP)) | ||
{ | ||
LM_T(LmtForward, ("Calling registrationsQuery I")); | ||
if (registrationsQuery(enV, attrL, &crrV, &err, tenant, servicePathV, 0, 0, false)) | ||
{ | ||
LM_T(LmtForward, ("registrationsQuery I returned %d items in crrV", crrV.size())); | ||
if (crrV.size() > 0) | ||
{ | ||
fillContextProviders(cerP, crrV); | ||
|
@@ -2734,8 +2741,10 @@ static void searchContextProviders | |
StringList attrNullList; | ||
if (someContextElementNotFound(*cerP)) | ||
{ | ||
LM_T(LmtForward, ("Calling registrationsQuery II")); | ||
if (registrationsQuery(enV, attrNullList, &crrV, &err, tenant, servicePathV, 0, 0, false)) | ||
{ | ||
LM_T(LmtForward, ("registrationsQuery II returned %d items in crrV", crrV.size())); | ||
if (crrV.size() > 0) | ||
{ | ||
fillContextProviders(cerP, crrV); | ||
|
@@ -2761,21 +2770,26 @@ static void searchContextProviders | |
*/ | ||
static bool forwardsPending(UpdateContextResponse* upcrsP) | ||
{ | ||
LM_T(LmtForward, ("Looping over %d context element responses", upcrsP->contextElementResponseVector.size())); | ||
for (unsigned int cerIx = 0; cerIx < upcrsP->contextElementResponseVector.size(); ++cerIx) | ||
{ | ||
ContextElementResponse* cerP = upcrsP->contextElementResponseVector[cerIx]; | ||
|
||
LM_T(LmtForward, ("Looping over %d attributes", cerP->entity.attributeVector.size())); | ||
for (unsigned int aIx = 0 ; aIx < cerP->entity.attributeVector.size(); ++aIx) | ||
{ | ||
ContextAttribute* aP = cerP->entity.attributeVector[aIx]; | ||
|
||
LM_T(LmtForward, ("Attribute: '%s'", aP->name.c_str())); | ||
if (aP->providingApplication.get() != "") | ||
{ | ||
LM_T(LmtForward, ("Found a providingApplication: %s", aP->providingApplication.get().c_str())); | ||
return true; | ||
} | ||
} | ||
} | ||
|
||
LM_T(LmtForward, ("No providingApplication found")); | ||
return false; | ||
} | ||
|
||
|
@@ -2952,6 +2966,7 @@ static void updateEntity | |
// FIXME P8: the same three statements are at the end of the while loop. Refactor the code to have this | ||
// in only one place | ||
// | ||
LM_T(LmtForward, ("Calling searchContextProviders")); | ||
searchContextProviders(tenant, servicePathV, en, eP->attributeVector, cerP); | ||
|
||
if (!(attributeAlreadyExistsError && (action == ActionTypeAppendStrict))) | ||
|
@@ -3488,8 +3503,11 @@ void processContextElement | |
// | ||
// If no context providers found, then the UPDATE was simply for a non-found entity and an error should be returned | ||
// | ||
LM_T(LmtForward, ("Calling forwardsPending")); | ||
if (forwardsPending(responseP) == false) | ||
{ | ||
LM_T(LmtForward, ("No forwards pending")); | ||
|
||
cerP->statusCode.fill(SccContextElementNotFound); | ||
|
||
if (apiVersion == V1) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line seems to be duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One line is about queries, the other about updates.
They are very similar but also completely different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misses the "queries" vs "udpates" difference. I's ok. Sorry the noise.
NTC