Bug/2763 bug fix not refactor #2821

kzangeli · 2017-01-17T17:10:03Z

Three fixes:

if entity::type is given but as an empty string, an error is returned ([BUG] in POST v2/subscriptions, if "entities/type" has empty values #1985)
admin requests with errors now return '400 Bad Request', not '200 OK' (no issue, I think)
attempt to create an entity with an error in service path (and probably more types of errors) now returns '400 Bad Request', not '200 OK' (Error returns from postUpdateContext #2763)

crbrox · 2017-01-17T17:19:28Z

LGTM

fgalan · 2017-01-17T18:06:06Z

CHANGES_NEXT_RELEASE

@@ -15,6 +15,9 @@
 - Fix: bug when tenants are over 44 bytes long (#2811)
 - Add: Support for null values in string filters (#2359)
 - Fix: Partial sanity check for custon url, only if the url contains no replacements using ${xxx} (#2280, #2279)
+- Fix: if entity::type is given but as an empty string, an error is returned (#1985)


I'd suggest to explain in which operations this fix has been done. Maybe this?

"Fix: if subject.entities*.type is given but as an empty string in POST /v2/subscriptions or PATCH /v2/subscriptions/{sub} , an error is returned (#1985)"

The checks are done in low level functions, parseEntity() and parseEntityObject(). Should be valid for all V2 requests that have entites, including GET /v2/entities and probably more ops.

Do you still want a list of all the ops in CNR ?

So you mean that before this PR entity creation with "type": "" was failing? A bit surprissing... I think we already have .test in place that check min and max length for all id-kind fields (entity id/type, attr name/type, md name/type)...

That was my understanding, I may be wrong.
In the json parse stage (parseEntity() and parseEntityObject()), there were no checks for empty 'type'. Perhaps there are checks that cover part of the request set somewhere else ... ?

So, from what I've seen now, it seems all cases (except for /v2/subscriptions) of empty entity::type were caught already. just as @fgalan predicted :-), changing the CNR line

Fixed in a93291f

fgalan · 2017-01-17T18:07:20Z

CHANGES_NEXT_RELEASE

@@ -15,6 +15,9 @@
 - Fix: bug when tenants are over 44 bytes long (#2811)
 - Add: Support for null values in string filters (#2359)
 - Fix: Partial sanity check for custon url, only if the url contains no replacements using ${xxx} (#2280, #2279)
+- Fix: if entity::type is given but as an empty string, an error is returned (#1985)
+- Fix: admin requests with errors now return '400 Bad Request', not '200 OK'


"adming request" could be a big obscure, and example would help. I mean:

"Fix: admin requests (e.g. GET /version) with errors now return '400 Bad Request', not '200 OK'"

Btw, issue number is missing at the end of the line. If the fix is not related with any issue, that's perfect (asking only to check if that is the case...)

Sure, an example may make it easier to understand.
No, no issue here, as far as I know, see PR description:

admin requests with errors now return '400 Bad Request', not '200 OK' (no issue, I think)

Fixed in 8fa9c69

fgalan · 2017-01-17T18:10:04Z

src/lib/rest/OrionError.cpp

@@ -109,7 +109,7 @@ std::string OrionError::smartRender(ApiVersion apiVersion)
 */
 std::string OrionError::setStatusCodeAndSmartRender(ApiVersion apiVersion, HttpStatusCode* scP)
 {
-  if (apiVersion == V2)
+  if ((apiVersion == V2) || (apiVersion == ADMIN_API))


Great to see that the addition has not broken anything... setStatusCodeAndSmartRender() modifyication were hard in the past (at least to me...)

NTC

Yeah, I am not so sure. Seeing a few really strange errors in functests ...

Errors under control again, fixed in f422157 (this line is intact, the problem was elsewhere)

fgalan · 2017-01-17T18:11:55Z

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

+
+echo "02. Make sure the entity doesn't exist in the database"
+echo "======================================================"
+echo "db.entities.findOne()" | mongo ftest


Why don't use the GET /v2/entities operation to check it? We should use keep direct mongo queries only as last resort measure when the API doesn't provide alternatives.

(Btw, I think we have a mongoCmd() function for test harness to avoid direct calls to mongo shell)

Sure, GET /v2/entities works also. As we usually do. Not sure why I implemented like this (paranoia perhaps)

Fixed in 159bd5f

fgalan · 2017-01-17T18:20:31Z

...st/cases/2763_postUpdateContext_error_return/invalid_service_path_gives_400_bad_request.test

@@ -0,0 +1,56 @@
+# Copyright 2016 Telefonica Investigacion y Desarrollo, S.A.U


Looking again, I don't see the .test covering #1985.... Maybe you missed git add before commit? ;)

I can't find one. I have created the directory but there is no functest inside ...
Also can't find any modified functest for this ...

Must check this, understand what's happened here.

There wasn't any, and the problem wasn't even fixed.
I just assumed (and forgot about the functest), that the low level parse functions would take care of this (as they should). However subscriptions use their own implementation of entity parsing ...

Fixed in f422157

…s/restrict_chars_entity.test

…ity id and type

…restrict_chars_entity.test

…763_bug_fix_not_refactor

…ty id/type'

fgalan · 2017-01-18T16:08:16Z

src/lib/jsonParseV2/parseEntity.cpp

-          alarmMgr.badInput(clientIp, "invalid JSON type for entity id");
-          ciP->httpStatusCode = SccBadRequest;;
-          OrionError oe(SccBadRequest, "invalid JSON type for entity id", "BadRequest");
+          const char* errorText = "Invalid JSON type for entity id";


Move to errorMessages.h

Fixed in bc960d4

fgalan · 2017-01-18T16:08:36Z

src/lib/jsonParseV2/parseEntity.cpp

-        alarmMgr.badInput(clientIp, "'id' is not a valid attribute");
-        ciP->httpStatusCode = SccBadRequest;;
-        OrionError oe(SccBadRequest, "invalid input, 'id' as attribute", "BadRequest");
+        const char* errorText = "invalid input, 'id' as attribute";


Move to errorMessages.h

fgalan · 2017-01-18T16:09:01Z

src/lib/jsonParseV2/parseEntity.cpp

        return oe.toJson();
      }
    }
    else if (name == "type")
    {
      if (type != "String")
      {
-        alarmMgr.badInput(clientIp, "invalid JSON type for entity type");
+        const char* errorText = "Invalid JSON type for entity type";


Move to errorMessages.h

Fixed in bc960d4

fgalan · 2017-01-18T16:09:21Z

src/lib/jsonParseV2/parseEntity.cpp

+
+      if (eP->type.empty())
+      {
+        const char* errorText = "entity type length: 0, min length supported: 1";


Move to errorMessages.h (in fact, I think the text is already there in a previous define)

fgalan · 2017-01-18T16:11:52Z

src/lib/jsonParseV2/parseEntityObject.cpp

      {
-        return "Invalid characters in entity type";
+        return "entity type length: 0, min length supported: 1";


Move to errorMessages.h

fgalan · 2017-01-18T16:12:00Z

src/lib/jsonParseV2/parseSubscription.cpp

@@ -384,6 +384,10 @@ static std::string parseEntitiesVector(ConnectionInfo* ciP, std::vector<EntID>*
        {
          return badInput(ciP, "max type length exceeded");
        }
+        if (typeOpt.value.empty())
+        {
+          return badInput(ciP, "entity type length: 0, min length supported: 1");


Move to errorMessages.h

fgalan · 2017-01-18T16:14:45Z

test/functionalTest/cases/1985_empty_entity_type/empty_entity_type_in_subscription.test

+--SHELL--
+
+#
+# 01. Attempt to create a subscription with an empty entity type, see error


To make this complet and align with

"Fix: if entity::type is given but as an empty string, in payload of POST /v2/subscriptions or PATCH /v2/subscriptions/{sub}, an error is returned (#1985)"

I'd suggest to add

Successfull creation of a subscription

Attemp to update the existing subscription with an empty entity type, see error

Fixed in bc960d4

fgalan · 2017-01-18T17:56:17Z

LGTM

Ken Zangelin added 3 commits January 17, 2017 12:54

Fixed error in functest 2819

b582c86

three smaller issues fixed

493d873

picked a standard text for entity type empty

4adda71

kzangeli added this to the 1.7.0 milestone Jan 17, 2017

kzangeli assigned crbrox and fgalan Jan 17, 2017

fgalan requested changes Jan 17, 2017

View reviewed changes

admin request example in CNR

8fa9c69

This was referenced Jan 17, 2017

Error returns from postUpdateContext #2763

Closed

[BUG] in POST v2/subscriptions, if "entities/type" has empty values #1985

Closed

fgalan reviewed Jan 17, 2017

View reviewed changes

Ken Zangelin and others added 8 commits January 17, 2017 19:23

changed a mongo command for a GET /v2/entities

159bd5f

forbiddenChars etc in parseEntity, Modified 1601_restrict_chars_in_id…

f422157

…s/restrict_chars_entity.test

CHANGES_NEXT_RELEASE

a93291f

Using forbiddenIdChars instead of forbiddenChars in json parse of ent…

f1ac202

…ity id and type

moved the 'invalid character' texts to common/errorMessages.h

b9eb33a

FIX rollback in unneeded changes in cases/1601_restrict_chars_in_ids/…

a2e549b

…restrict_chars_entity.test

Merge remote branch 'origin/bug/2763_bug_fix_not_refactor' into bug/2…

a6faae9

…763_bug_fix_not_refactor

New error messages in errorMessages.h for 'Invalid JSON type for enti…

f0be43b

…ty id/type'

fgalan reviewed Jan 18, 2017

View reviewed changes

more stuff in a functest + more texts to errorMessages.h

bc960d4

fgalan approved these changes Jan 18, 2017

View reviewed changes

fgalan merged commit 6427083 into master Jan 18, 2017

fgalan deleted the bug/2763_bug_fix_not_refactor branch January 18, 2017 18:06

kzangeli mentioned this pull request Jan 18, 2017

Bug/2763 bug fix not refactor #2824

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/2763 bug fix not refactor #2821

Bug/2763 bug fix not refactor #2821

kzangeli commented Jan 17, 2017 •

edited

Loading

crbrox commented Jan 17, 2017

fgalan Jan 17, 2017

kzangeli Jan 17, 2017

fgalan Jan 17, 2017

kzangeli Jan 17, 2017

kzangeli Jan 18, 2017

kzangeli Jan 18, 2017

fgalan Jan 17, 2017

kzangeli Jan 17, 2017

kzangeli Jan 17, 2017

fgalan Jan 17, 2017

kzangeli Jan 17, 2017

kzangeli Jan 18, 2017 •

edited

Loading

fgalan Jan 17, 2017

kzangeli Jan 17, 2017

kzangeli Jan 17, 2017

fgalan Jan 17, 2017

kzangeli Jan 17, 2017

kzangeli Jan 18, 2017 •

edited

Loading

fgalan Jan 18, 2017

kzangeli Jan 18, 2017

fgalan Jan 18, 2017

fgalan Jan 18, 2017

kzangeli Jan 18, 2017

fgalan Jan 18, 2017

fgalan Jan 18, 2017

fgalan Jan 18, 2017

fgalan Jan 18, 2017

kzangeli Jan 18, 2017

fgalan commented Jan 18, 2017

		@@ -0,0 +1,56 @@
		# Copyright 2016 Telefonica Investigacion y Desarrollo, S.A.U

Bug/2763 bug fix not refactor #2821

Bug/2763 bug fix not refactor #2821

Conversation

kzangeli commented Jan 17, 2017 • edited Loading

crbrox commented Jan 17, 2017

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 Jan 18, 2017 • 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

kzangeli Jan 18, 2017 • 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

fgalan commented Jan 18, 2017

kzangeli commented Jan 17, 2017 •

edited

Loading

kzangeli Jan 18, 2017 •

edited

Loading

kzangeli Jan 18, 2017 •

edited

Loading