-
Notifications
You must be signed in to change notification settings - Fork 265
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
FIX issues related with NGSIv2 refactor (#1720 and #1428) #1727
Conversation
@@ -49,7 +49,8 @@ std::string parseAttributeValue(ConnectionInfo* ciP, ContextAttribute* caP) | |||
if (document.HasParseError()) | |||
{ | |||
alarmMgr.badInput(clientIp, "JSON parse error"); | |||
oe.fill(SccBadRequest, "Errors found in incoming JSON buffer"); | |||
oe.reasonPhrase = ERROR_STRING_PARSERROR; |
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 could be done with a new fill() method, but not sure if just one case (this one) justifies the creation of the new method...
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.
Or simply use the old fill() but with this new define:
oe.fill(SccBadRequest, ERROR_STRING_PARSERROR);
Isn't that what you want?
BTW, what about oe.code? Not calling fill, just setting the reasonPhrase might not be enough ...
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.
Doesn't work that way:
OrionError::OrionError(HttpStatusCode _code, const std::string& _details)
{
code = _code;
reasonPhrase = httpStatusCodeString(code);
details = _details;
}
The reasonPhrase is automatically derived from SccBadRequest (i.e. something like "Bad Request"), but in this case I want to use a different one. And I don't what details be "ERROR_STRING_PARSERROR", but "Errors found in incoming JSON buffer".
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.
Yeah, I know. We "fixed" the text.
However, you still have no oe.code. You might need one.
To "fix" your problem, we could add a third parameter to the constructor, and default it to NULL:
OrionError::OrionError(HttpStatusCode _code, const std::string& _details, const char* reason = NULL)
{
code = _code;
reasonPhrase = (reason == NULL) httpStatusCodeString(code) : reason;
details = _details;
}
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.
Fixed in fa4ed7e.
code = _code; | ||
reasonPhrase = httpStatusCodeString(code); | ||
code = SccNone; | ||
reasonPhrase = _reasonPhrase; | ||
details = _details; | ||
} |
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.
So, you remove the code, setting it no SccNone always?
Not sure this is a good idea ...
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 agree. Probably what is a but idea is a OrionError::OrionError(const std::string _reasonPhrase, const std::string& _details)
constructor, without code. I'll have a second pass to OrionError constructors, trying to fix this.
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.
Fixed in fa4ed7e
… into bug/prost_verb
Conflicts: src/lib/jsonParseV2/parseSubscription.cpp
LGTM |
Fixes:
No CNR entry needed (already covered by the entry about #1259)
@crbrox
@kzangeli