aws / Jobs-for-AWS-IoT-embedded-sdk

Client library for using AWS IoT Jobs service on embedded devices
MIT License
13 stars 38 forks source link

UpdateMsg Format #107

Closed TheCrazyKing closed 2 months ago

TheCrazyKing commented 2 months ago

Hi,

It seems that when following the example to create a update msg yields something like

"{"status":"SUCCEEDED","statusDetails":"{"key":"value"}"}}

Now, I don't know how you expect the MQTT API to be like, but in my case having a JSON content enclosed in quotes ("{"key":"val"}") is not working. I think it is safe to assume that job details will always be formatted as JSON and hence these quotes should be omitted.

If you don't agree I'll just use a local patch, so let me know :)

All the best

Ref:

 * const char * expectedVersion = "2";
 * const chat * statusDetails = "{\"key\":\"value\"}";
 * char messageBuffer[ UPDATE_JOB_MSG_LENGTH ]  = {0};
 * size_t messageLength = 0U;
 *
 * JobsUpdateRequest_t request;
 * request.status = Succeeded;
 * request.expectedVersion = expectedVersion;
 * request.expectedVersionLength = ( sizeof( expectedVersion ) - 1U );
 * request.statusDetails = statusDetails;
 * request.statusDetailsLength = ( sizeof( statusDetails ) - 1U );
 *
 * messageLength = Jobs_UpdateMsg( request
 *                                 messageBuffer,
 *                                 UPDATE_JOB_MSG_LENGTH );
 *
 * if (messageBufferLength > 0 )
 * {
 *     // The message string of length, messageLength, has been
 *     // generated in the buffer, messageBuffer, for the UpdateJobExecution API
 *     // Publish this message to the topic generated by Jobs_Update using an
 *     // MQTT client of your choice.
 * }
 * @endcode
 */
/* @[declare_jobs_updatemsg] */
size_t Jobs_UpdateMsg( JobsUpdateRequest_t request,
                       char * buffer,
                       size_t bufferSize );
ActoryOu commented 2 months ago

Hello @TheCrazyKing, Thanks for bringing this into our attension! I've verified that example and it's also failure in my case. I'll create a PR to fix that.

The problem of this example is not the quote, and the statusDetails is also expected to be JSON format (checked by areOptionalFieldsValid()). Note that the expected result would be: {"status":"SUCCEEDED","expectedVersion":"2","statusDetails":"{"key":"value"}"}.

Thank you.

TheCrazyKing commented 2 months ago

Ok! I saw your PR and it indeed corrects a bug. Now, I had just this question regarding the quotes if I may: when publishing (with the AWS IoT MQTT test client) to the jobs update topic, the following content throws an error ({ "code": "InvalidJson", "message": "Unexpected character ('p' (code 112)): was expecting comma to separate Object entries at [Source: (iot.laser.tjm.handler.utils.DeviceMessageParser$1); line: 1, column: 68]" }):

{
 "status":"IN_PROGRESS",
 "statusDetails": "{"progress":"testerQuotes"}"
}

but the following message format is working:

{
 "status":"IN_PROGRESS",
 "statusDetails": {"progress":"testerQuotes"}
}

Is it still compliant with the expected result {"status":"SUCCEEDED","expectedVersion":"2","statusDetails":"{"key":"value"}"} you mentioned ? Or am I missing something here ?

PS: the message format

{"status":"IN_PROGRESS","statusDetails":"{'progress':'testerQuotes'}"}

gives this error "code": "InvalidRequest","message": "Value at statusDetails cannot be a STRING for topic $aws/XXX

aggarg commented 2 months ago

Is it still compliant with the expected result {"status":"SUCCEEDED","expectedVersion":"2","statusDetails":"{"key":"value"}"} you mentioned ? Or am I missing something here ?

This is not a valid JSON. You can check that using any JSON validator - https://jsonformatter.org/json-pretty-print.

ActoryOu commented 2 months ago

Hi @TheCrazyKing, "{"progress":"testerQuotes"}" is not a valid JSON, as @aggarg said. When we declare a char string like const chat statusDetails[] = "{\"key\":\"value\"}";, the string is actually {"key":"value"}. No quote at the begining and no quote at the end.

Thank you.

ActoryOu commented 2 months ago

Closing because it's resolved.

TheCrazyKing commented 2 months ago

@ActoryOu Ok but the SDK (JobsUpdateMsg) will enclose the status details JSON with quotes:

#define JOBS_API_STATUS_DETAILS "\",\"statusDetails\":\"" and at the end of the function: ( void ) strnAppend( buffer, &start, bufferSize, "\"}", ( CONST_STRLEN( "\"}" ) ) );

I would apply a patch like this one:

diff --git a/source/include/jobs.h b/source/include/jobs.h
index 0939e54..ec3864d 100644
--- a/source/include/jobs.h
+++ b/source/include/jobs.h
@@ -150,7 +150,7 @@
 #define JOBS_API_EXPECTED_VERSION           "\",\"expectedVersion\":\""
 #define JOBS_API_EXPECTED_VERSION_LENGTH    ( sizeof( JOBS_API_EXPECTED_VERSION ) - 1U )

-#define JOBS_API_STATUS_DETAILS             "\",\"statusDetails\":\""
+#define JOBS_API_STATUS_DETAILS             "\",\"statusDetails\":"
 #define JOBS_API_STATUS_DETAILS_LENGTH      ( sizeof( JOBS_API_STATUS_DETAILS ) - 1U )

 #define JOBS_API_COMMON_LENGTH( thingNameLength ) \
diff --git a/source/jobs.c b/source/jobs.c
index bd15740..89c4210 100644
--- a/source/jobs.c
+++ b/source/jobs.c
@@ -907,13 +907,17 @@ size_t Jobs_UpdateMsg( JobsUpdateRequest_t request,
     {
         ( void ) strnAppend( buffer, &start, bufferSize, JOBS_API_STATUS_DETAILS, JOBS_API_STATUS_DETAILS_LENGTH );
         ( void ) strnAppend( buffer, &start, bufferSize, request.statusDetails, request.statusDetailsLength );
-    }

-    if( !writeFailed )
-    {
-        ( void ) strnAppend( buffer, &start, bufferSize, "\"}", ( CONST_STRLEN( "\"}" ) ) );
+        ( void ) strnAppend( buffer, &start, bufferSize, "}", ( CONST_STRLEN( "}" ) ) );
+    }else{
+
+        if( !writeFailed )
+        {
+            ( void ) strnAppend( buffer, &start, bufferSize, "\"}", ( CONST_STRLEN( "\"}" ) ) );
+        }
     }

+
     return start;
 }

If you append \" after the job status details, there will be a quote after the JSON status detail.

If you do not agree, I'll use the patch only locally.

All the best

aggarg commented 2 months ago

You are exactly right. This PR addresses the issue - https://github.com/aws/Jobs-for-AWS-IoT-embedded-sdk/pull/109.

Thank you for reporting it.