From c4f73ef0d81a27d2e286e690a422445c46240a4b Mon Sep 17 00:00:00 2001 From: Ken Zangelin Date: Sat, 28 Jan 2023 11:30:05 +0100 Subject: [PATCH 1/2] More logging for attribute value == null in pCheckAttribute (user request, issue #1312) --- CHANGES_NEXT_RELEASE | 1 + src/lib/orionld/forwarding/distOpSend.cpp | 2 +- .../legacyDriver/legacyPatchAttribute.cpp | 2 +- .../legacyDriver/legacyPatchEntity.cpp | 2 +- .../orionld/legacyDriver/legacyPostEntity.cpp | 2 +- .../orionld/payloadCheck/pCheckAttribute.cpp | 34 +++-- .../orionld/payloadCheck/pCheckAttribute.h | 1 + src/lib/orionld/payloadCheck/pCheckEntity.cpp | 8 +- .../serviceRoutines/orionldPatchAttribute.cpp | 2 +- .../serviceRoutines/orionldPatchEntity.cpp | 2 +- .../serviceRoutines/orionldPostEntity.cpp | 2 +- .../orionldPostTemporalEntities.cpp | 4 +- .../ngsild_patch_entity_errors.test | 144 ++++++++++++++++++ 13 files changed, 183 insertions(+), 23 deletions(-) create mode 100644 test/functionalTest/cases/0000_ngsild/ngsild_patch_entity_errors.test diff --git a/CHANGES_NEXT_RELEASE b/CHANGES_NEXT_RELEASE index a96ca4030d..8d3a02372c 100644 --- a/CHANGES_NEXT_RELEASE +++ b/CHANGES_NEXT_RELEASE @@ -43,3 +43,4 @@ * Issue #1307 Implemented notifications for entity deletion, for DELETE /entities/{entityId} * Issue #280 Implemented forwarding for entity deletion, for DELETE /entities/{entityId} * Issue #280 Implemented forwarding for Batch entity deletion, for POST /entityOperations/delete +* Issue #1312 Better error handling for a specific use case about RHS being 'null' diff --git a/src/lib/orionld/forwarding/distOpSend.cpp b/src/lib/orionld/forwarding/distOpSend.cpp index 45903156f6..e483861b62 100644 --- a/src/lib/orionld/forwarding/distOpSend.cpp +++ b/src/lib/orionld/forwarding/distOpSend.cpp @@ -546,7 +546,7 @@ bool distOpSend(DistOp* distOpP, const char* dateHeader, const char* xForwardedF if (tenant != NULL) { - char tenantHeader[64]; + char tenantHeader[80]; snprintf(tenantHeader, sizeof(tenantHeader), "NGSILD-Tenant: %s", tenant); headers = curl_slist_append(headers, tenantHeader); } diff --git a/src/lib/orionld/legacyDriver/legacyPatchAttribute.cpp b/src/lib/orionld/legacyDriver/legacyPatchAttribute.cpp index 31efaecfd2..51dff45c49 100644 --- a/src/lib/orionld/legacyDriver/legacyPatchAttribute.cpp +++ b/src/lib/orionld/legacyDriver/legacyPatchAttribute.cpp @@ -790,7 +790,7 @@ bool legacyPatchAttribute(void) if (attrTypeInDb != NULL) attributeType = orionldAttributeType(attrTypeInDb); - if (pCheckAttribute(inAttribute, true, attributeType, true, contextItemP) == false) + if (pCheckAttribute(entityId, inAttribute, true, attributeType, true, contextItemP) == false) return false; diff --git a/src/lib/orionld/legacyDriver/legacyPatchEntity.cpp b/src/lib/orionld/legacyDriver/legacyPatchEntity.cpp index fb5715dd61..46206d8064 100644 --- a/src/lib/orionld/legacyDriver/legacyPatchEntity.cpp +++ b/src/lib/orionld/legacyDriver/legacyPatchEntity.cpp @@ -199,7 +199,7 @@ bool legacyPatchEntity(void) if (dbTypeP != NULL) attributeType = orionldAttributeType(dbTypeP->value.s); - if (pCheckAttribute(newAttrP, true, attributeType, true, contextItemP) == false) + if (pCheckAttribute(entityId, newAttrP, true, attributeType, true, contextItemP) == false) { // // A failure will set a 400 (probably) in orionldState.pd.status diff --git a/src/lib/orionld/legacyDriver/legacyPostEntity.cpp b/src/lib/orionld/legacyDriver/legacyPostEntity.cpp index 0ca2cfbcda..fb8bec3d4f 100644 --- a/src/lib/orionld/legacyDriver/legacyPostEntity.cpp +++ b/src/lib/orionld/legacyDriver/legacyPostEntity.cpp @@ -261,7 +261,7 @@ bool legacyPostEntity(void) OrionldContextItem* contextItemP = NULL; char* shortName = attrP->name; - if (pCheckAttribute(attrP, true, NoAttributeType, false, contextItemP) == false) + if (pCheckAttribute(entityId, attrP, true, NoAttributeType, false, contextItemP) == false) { attributeNotUpdated(notUpdatedP, shortName, orionldState.pd.title, orionldState.pd.detail); diff --git a/src/lib/orionld/payloadCheck/pCheckAttribute.cpp b/src/lib/orionld/payloadCheck/pCheckAttribute.cpp index 4434f8a177..ab816aa277 100644 --- a/src/lib/orionld/payloadCheck/pCheckAttribute.cpp +++ b/src/lib/orionld/payloadCheck/pCheckAttribute.cpp @@ -190,6 +190,7 @@ static bool pCheckTypeFromContext(KjNode* attrP, OrionldContextItem* attrContext // inline bool pCheckAttributeString ( + const char* entityId, KjNode* attrP, bool isAttribute, OrionldAttributeType attrTypeFromDb, @@ -237,6 +238,7 @@ inline bool pCheckAttributeString // inline bool pCheckAttributeInteger ( + const char* entityId, KjNode* attrP, bool isAttribute, OrionldAttributeType attrTypeFromDb, @@ -259,6 +261,7 @@ inline bool pCheckAttributeInteger // inline bool pCheckAttributeFloat ( + const char* entityId, KjNode* attrP, bool isAttribute, OrionldAttributeType attrTypeFromDb, @@ -281,6 +284,7 @@ inline bool pCheckAttributeFloat // inline bool pCheckAttributeBoolean ( + const char* entityId, KjNode* attrP, bool isAttribute, OrionldAttributeType attrTypeFromDb, @@ -303,6 +307,7 @@ inline bool pCheckAttributeBoolean // inline bool pCheckAttributeArray ( + const char* entityId, KjNode* attrP, bool isAttribute, OrionldAttributeType attrTypeFromDb, @@ -328,7 +333,7 @@ inline bool pCheckAttributeArray // // pCheckAttributeNull - // -inline bool pCheckAttributeNull(KjNode* attrP) +inline bool pCheckAttributeNull(const char* entityId, KjNode* attrP) { #if 0 LM_W(("RHS for attribute '%s' is NULL - that is forbidden in the NGSI-LD API", attrP->name)); @@ -790,6 +795,7 @@ static bool isJsonLiteral(KjNode* attrP, KjNode* typeP) // static bool pCheckAttributeObject ( + const char* entityId, // For log messages only, still important KjNode* attrP, bool isAttribute, OrionldAttributeType attrTypeFromDb, @@ -963,7 +969,12 @@ static bool pCheckAttributeObject { if ((orionldState.serviceP->options & ORIONLD_SERVICE_OPTION_ACCEPT_JSONLD_NULL) == 0) { - orionldError(OrionldBadRequestData, "null is not allowed as RHS in a JSON-LD document", fieldP->name, 400); + char errorString[512]; + snprintf(errorString, sizeof(errorString), + "Got a JSON 'null' in RHS (not allowed in JSON-LD docs) for the entity '%s', attribute '%s', attribute field '%s'", + entityId, attrP->name, fieldP->name); + LM_E(("%s", errorString)); + orionldError(OrionldBadRequestData, "Bad Input", errorString, 400); return false; } @@ -1045,7 +1056,7 @@ static bool pCheckAttributeObject } else { - if (pCheckAttribute(fieldP, false, NoAttributeType, false, NULL) == false) + if (pCheckAttribute(entityId, fieldP, false, NoAttributeType, false, NULL) == false) return false; } @@ -1215,6 +1226,7 @@ static bool validAttrName(const char* attrName, bool isAttribute) // bool pCheckAttribute ( + const char* entityId, KjNode* attrP, bool isAttribute, OrionldAttributeType attrTypeFromDb, @@ -1268,7 +1280,7 @@ bool pCheckAttribute // => KjNode* datasetsP should be a parameter to this function // aInstanceP->name = attrP->name; - if (pCheckAttribute(aInstanceP, true, attrTypeFromDb, attrNameAlreadyExpanded, attrContextInfoP) == false) + if (pCheckAttribute(entityId, aInstanceP, true, attrTypeFromDb, attrNameAlreadyExpanded, attrContextInfoP) == false) return false; } @@ -1293,13 +1305,13 @@ bool pCheckAttribute // We don't know which sub-attributes are special until we know the type of the attribute (Property, Relationship, etc) // Postponed to ... all simple Property functions - pCheckAttribute[String|Integer|Float|Boolean] + pCheckAttributeObject (Geo) // - if (attrP->type == KjString) return pCheckAttributeString(attrP, isAttribute, attrTypeFromDb, attrContextInfoP); - else if (attrP->type == KjInt) return pCheckAttributeInteger(attrP, isAttribute, attrTypeFromDb, attrContextInfoP); - else if (attrP->type == KjFloat) return pCheckAttributeFloat(attrP, isAttribute, attrTypeFromDb, attrContextInfoP); - else if (attrP->type == KjBoolean) return pCheckAttributeBoolean(attrP, isAttribute, attrTypeFromDb, attrContextInfoP); - else if (attrP->type == KjArray) return pCheckAttributeArray(attrP, isAttribute, attrTypeFromDb, attrContextInfoP); - else if (attrP->type == KjObject) return pCheckAttributeObject(attrP, isAttribute, attrTypeFromDb, attrContextInfoP); - else if (attrP->type == KjNull) return pCheckAttributeNull(attrP); + if (attrP->type == KjString) return pCheckAttributeString(entityId, attrP, isAttribute, attrTypeFromDb, attrContextInfoP); + else if (attrP->type == KjInt) return pCheckAttributeInteger(entityId, attrP, isAttribute, attrTypeFromDb, attrContextInfoP); + else if (attrP->type == KjFloat) return pCheckAttributeFloat(entityId, attrP, isAttribute, attrTypeFromDb, attrContextInfoP); + else if (attrP->type == KjBoolean) return pCheckAttributeBoolean(entityId, attrP, isAttribute, attrTypeFromDb, attrContextInfoP); + else if (attrP->type == KjArray) return pCheckAttributeArray(entityId, attrP, isAttribute, attrTypeFromDb, attrContextInfoP); + else if (attrP->type == KjObject) return pCheckAttributeObject(entityId, attrP, isAttribute, attrTypeFromDb, attrContextInfoP); + else if (attrP->type == KjNull) return pCheckAttributeNull(entityId, attrP); // Invalid JSON type of the attribute - we should never reach this point orionldError(OrionldInternalError, "invalid value type for attribute", attrP->name, 500); diff --git a/src/lib/orionld/payloadCheck/pCheckAttribute.h b/src/lib/orionld/payloadCheck/pCheckAttribute.h index ba4e813c6a..b1d08bc905 100644 --- a/src/lib/orionld/payloadCheck/pCheckAttribute.h +++ b/src/lib/orionld/payloadCheck/pCheckAttribute.h @@ -48,6 +48,7 @@ extern "C" // extern bool pCheckAttribute ( + const char* entityId, KjNode* attrP, bool isAttribute, OrionldAttributeType attrTypeFromDb, diff --git a/src/lib/orionld/payloadCheck/pCheckEntity.cpp b/src/lib/orionld/payloadCheck/pCheckEntity.cpp index 1f35a573aa..b9ab6ee5ee 100644 --- a/src/lib/orionld/payloadCheck/pCheckEntity.cpp +++ b/src/lib/orionld/payloadCheck/pCheckEntity.cpp @@ -171,8 +171,9 @@ bool pCheckEntity if ((nodeP = kjLookup(entityP, "modifiedAt")) != NULL) kjChildRemove(entityP, nodeP); // Loop over attributes - KjNode* idP = NULL; - KjNode* typeP = NULL; + KjNode* idP = NULL; + KjNode* typeP = NULL; + char* entityId = (char*) "urn:unknown:id"; for (KjNode* attrP = entityP->value.firstChildP; attrP != NULL; attrP = attrP->next) { @@ -191,6 +192,7 @@ bool pCheckEntity return false; idP = attrP; + entityId = idP->value.s; continue; } @@ -233,7 +235,7 @@ bool pCheckEntity aTypeFromDb = attrTypeFromDb(dbAttrsP, attrName); } - if (pCheckAttribute(attrP, true, aTypeFromDb, true, contextItemP) == false) + if (pCheckAttribute(entityId, attrP, true, aTypeFromDb, true, contextItemP) == false) return false; } diff --git a/src/lib/orionld/serviceRoutines/orionldPatchAttribute.cpp b/src/lib/orionld/serviceRoutines/orionldPatchAttribute.cpp index d84d776751..45290a3c2e 100644 --- a/src/lib/orionld/serviceRoutines/orionldPatchAttribute.cpp +++ b/src/lib/orionld/serviceRoutines/orionldPatchAttribute.cpp @@ -350,7 +350,7 @@ bool orionldPatchAttribute(void) // Make sure the incoming attribute is valid // OrionldAttributeType attrType = orionldAttributeType(attrTypeInDb); - if (pCheckAttribute(orionldState.requestTree, true, attrType, true, NULL) == false) + if (pCheckAttribute(entityId, orionldState.requestTree, true, attrType, true, NULL) == false) return false; kjTreeLog(orionldState.requestTree, "TR: Right after pCheckAttribute"); diff --git a/src/lib/orionld/serviceRoutines/orionldPatchEntity.cpp b/src/lib/orionld/serviceRoutines/orionldPatchEntity.cpp index aa3f8b6bb3..b2335c0ecf 100644 --- a/src/lib/orionld/serviceRoutines/orionldPatchEntity.cpp +++ b/src/lib/orionld/serviceRoutines/orionldPatchEntity.cpp @@ -254,7 +254,7 @@ bool orionldPatchEntity(void) OrionldAttributeType attrTypeFromDb = orionldAttributeType(dbAttrTypeP->value.s); // Crash if dbAttrTypeP == NULL ... OK somehow ... OrionldContextItem* contextItemP = NULL; - if (pCheckAttribute(inAttrP, true, attrTypeFromDb, true, contextItemP) == false) + if (pCheckAttribute(entityId, inAttrP, true, attrTypeFromDb, true, contextItemP) == false) { kjChildRemove(orionldState.requestTree, inAttrP); attributeNotUpdated(notUpdatedV, shortName, orionldState.pd.title, orionldState.pd.detail); diff --git a/src/lib/orionld/serviceRoutines/orionldPostEntity.cpp b/src/lib/orionld/serviceRoutines/orionldPostEntity.cpp index 46be63cd5c..90a4098ecc 100644 --- a/src/lib/orionld/serviceRoutines/orionldPostEntity.cpp +++ b/src/lib/orionld/serviceRoutines/orionldPostEntity.cpp @@ -211,7 +211,7 @@ bool orionldPostEntity(void) // // Is the attribute valid? // - if (pCheckAttribute(attrP, true, NoAttributeType, false, contextItemP) == false) + if (pCheckAttribute(entityId, attrP, true, NoAttributeType, false, contextItemP) == false) { kjChildRemove(orionldState.requestTree, attrP); attributeNotUpdated(notUpdatedP, shortName, orionldState.pd.title, orionldState.pd.detail); diff --git a/src/lib/orionld/serviceRoutines/orionldPostTemporalEntities.cpp b/src/lib/orionld/serviceRoutines/orionldPostTemporalEntities.cpp index e59087c9d8..1b3e557eef 100644 --- a/src/lib/orionld/serviceRoutines/orionldPostTemporalEntities.cpp +++ b/src/lib/orionld/serviceRoutines/orionldPostTemporalEntities.cpp @@ -118,7 +118,7 @@ bool orionldPostTemporalEntities(void) { aInstanceP->name = attrP->name; // Need to "inherit" the name for the Array - if (pCheckAttribute(aInstanceP, true, NoAttributeType, false, NULL) == false) + if (pCheckAttribute(entityId, aInstanceP, true, NoAttributeType, false, NULL) == false) return false; attrP->name = aInstanceP->name; // It's been expanded ionside pCheckAttribute @@ -126,7 +126,7 @@ bool orionldPostTemporalEntities(void) } else // KjObject { - if (pCheckAttribute(attrP, true, NoAttributeType, false, NULL) == false) + if (pCheckAttribute(entityId, attrP, true, NoAttributeType, false, NULL) == false) return false; } } diff --git a/test/functionalTest/cases/0000_ngsild/ngsild_patch_entity_errors.test b/test/functionalTest/cases/0000_ngsild/ngsild_patch_entity_errors.test new file mode 100644 index 0000000000..8e6ddbef2a --- /dev/null +++ b/test/functionalTest/cases/0000_ngsild/ngsild_patch_entity_errors.test @@ -0,0 +1,144 @@ +# Copyright 2023 FIWARE Foundation e.V. +# +# This file is part of Orion-LD Context Broker. +# +# Orion-LD Context Broker is free software: you can redistribute it and/or +# modify it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# Orion-LD Context Broker is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero +# General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with Orion-LD Context Broker. If not, see http://www.gnu.org/licenses/. +# +# For those usages not covered by this license please contact with +# orionld at fiware dot org + +# VALGRIND_READY - to mark the test ready for valgrindTestSuite.sh + +--NAME-- +PATCH Entity error handling for null RHS as attribute value + +--SHELL-INIT-- +export BROKER=orionld +dbInit CB +brokerStart CB + +--SHELL-- +# +# 01. Create an entity E1 with an attribute P1 +# 02. GET E1, see P1==1 +# 03. Patch E1, setting P1.value to null - see error +# 04. GET E1, make sure the PATCH changed NOTHING +# + +echo "01. Create an entity E1 with an attribute P1" +echo "============================================" +payload='{ + "id": "urn:E1", + "type": "T", + "P1": 1 +}' +orionCurl --url /ngsi-ld/v1/entities -X POST --payload "$payload" +echo +echo + + +echo "02. GET E1, see P1==1" +echo "=====================" +orionCurl --url /ngsi-ld/v1/entities/urn:E1 +echo +echo + + +echo "03. Patch E1, setting P1.value to null - see error" +echo "==================================================" +payload='{ + "P1": { + "value": null + } +}' +orionCurl --url /ngsi-ld/v1/entities/urn:E1/attrs -X PATCH --payload "$payload" +echo +echo + + +echo "04. GET E1, make sure the PATCH changed NOTHING" +echo "===============================================" +orionCurl --url /ngsi-ld/v1/entities/urn:E1 +echo +echo + + +--REGEXPECT-- +01. Create an entity E1 with an attribute P1 +============================================ +HTTP/1.1 201 Created +Content-Length: 0 +Date: REGEX(.*) +Location: /ngsi-ld/v1/entities/urn:E1 + + + +02. GET E1, see P1==1 +===================== +HTTP/1.1 200 OK +Content-Length: 61 +Content-Type: application/json +Date: REGEX(.*) +Link: ; rel="http://www.w3.org/ns/json-ld#context"; type="application/ld+json" + +{ + "P1": { + "type": "Property", + "value": 1 + }, + "id": "urn:E1", + "type": "T" +} + + +03. Patch E1, setting P1.value to null - see error +================================================== +HTTP/1.1 207 Multi-Status +Content-Length: 239 +Content-Type: application/json +Date: REGEX(.*) +Link: ; rel="http://www.w3.org/ns/json-ld#context"; type="application/ld+json" + +{ + "notUpdated": [ + { + "attributeName": "P1", + "reason": "Bad Input: Got a JSON 'null' in RHS (not allowed in JSON-LD docs) for the entity 'urn:E1', attribute 'https://uri.etsi.org/ngsi-ld/default-context/P1', attribute field 'value'" + } + ], + "updated": [] +} + + +04. GET E1, make sure the PATCH changed NOTHING +=============================================== +HTTP/1.1 200 OK +Content-Length: 61 +Content-Type: application/json +Date: REGEX(.*) +Link: ; rel="http://www.w3.org/ns/json-ld#context"; type="application/ld+json" + +{ + "P1": { + "type": "Property", + "value": 1 + }, + "id": "urn:E1", + "type": "T" +} + + +--TEARDOWN-- +brokerStop CB +dbDrop CB From e10f42a3e5dd38c2e01af38264d1b563cd9259be Mon Sep 17 00:00:00 2001 From: Ken Zangelin Date: Sat, 28 Jan 2023 12:12:52 +0100 Subject: [PATCH 2/2] Modified two functersts due to better error handling --- ...gsild_error_reject_entity_if_property_value_is_null.test | 6 +++--- ..._error_reject_entity_if_relationship_object_is_null.test | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/functionalTest/cases/0000_ngsild/ngsild_error_reject_entity_if_property_value_is_null.test b/test/functionalTest/cases/0000_ngsild/ngsild_error_reject_entity_if_property_value_is_null.test index e49651eb76..5e62d96604 100644 --- a/test/functionalTest/cases/0000_ngsild/ngsild_error_reject_entity_if_property_value_is_null.test +++ b/test/functionalTest/cases/0000_ngsild/ngsild_error_reject_entity_if_property_value_is_null.test @@ -53,13 +53,13 @@ echo 01. POST /ngsi-ld/v1/entities with an attribute with value NULL =============================================================== HTTP/1.1 400 Bad Request -Content-Length: 137 +Content-Length: 265 Content-Type: application/json Date: REGEX(.*) { - "detail": "value", - "title": "null is not allowed as RHS in a JSON-LD document", + "detail": "Got a JSON 'null' in RHS (not allowed in JSON-LD docs) for the entity 'urn:unknown:id', attribute 'https://uri.etsi.org/ngsi-ld/default-context/P1', attribute field 'value'", + "title": "Bad Input", "type": "https://uri.etsi.org/ngsi-ld/errors/BadRequestData" } diff --git a/test/functionalTest/cases/0000_ngsild/ngsild_error_reject_entity_if_relationship_object_is_null.test b/test/functionalTest/cases/0000_ngsild/ngsild_error_reject_entity_if_relationship_object_is_null.test index ee30313e45..66946b7d1d 100644 --- a/test/functionalTest/cases/0000_ngsild/ngsild_error_reject_entity_if_relationship_object_is_null.test +++ b/test/functionalTest/cases/0000_ngsild/ngsild_error_reject_entity_if_relationship_object_is_null.test @@ -53,13 +53,13 @@ echo 01. POST /ngsi-ld/v1/entities with a relationship attribute with object == null =============================================================================== HTTP/1.1 400 Bad Request -Content-Length: 138 +Content-Length: 266 Content-Type: application/json Date: REGEX(.*) { - "detail": "object", - "title": "null is not allowed as RHS in a JSON-LD document", + "detail": "Got a JSON 'null' in RHS (not allowed in JSON-LD docs) for the entity 'urn:unknown:id', attribute 'https://uri.etsi.org/ngsi-ld/default-context/R1', attribute field 'object'", + "title": "Bad Input", "type": "https://uri.etsi.org/ngsi-ld/errors/BadRequestData" }