Skip to content
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

Better bulk data handling in OnSystemRequest #123

Merged
merged 2 commits into from
Mar 9, 2015

Conversation

mikeburke106
Copy link
Contributor

Fixes #86.

Signed-off-by: Mike Burke mike@livioconnect.com

Signed-off-by: Mike Burke <mike@livioconnect.com>
@mikeburke106 mikeburke106 added the REVIEW - scheduled The reviewer has scheduled a review of this PR for the current sprint label Feb 23, 2015
@mikeburke106 mikeburke106 added this to the 3.4.0 milestone Feb 23, 2015
@mikeburke106
Copy link
Contributor Author

This is technically a hotfix since no APIs have changed, but I wanted to make sure the appropriate parties have time to review, so I'm marking this one for the regularly scheduled review.

I ran unit tests on this PR and all tests were passed, so I'm confident that it's good to go.

Only open question is whether the body field needs to conform to this weird logic in the existing code.

JSONObject jLayer1 = null;
String sReturn = null;        
try
{
    if (httpreqparams != null)
    {
        jLayer1 = myJSONObj.getJSONObject("HTTPRequest");           
        sReturn = jLayer1.getString("body");
        return sReturn;
    }
    else if (myJSONObj != null)
    {
        return myJSONObj.toString();
    }
    else
    {
        return null;
    }
}
catch (Exception e) 
{
    return null;            
}

The first if statement is the "normal" case where it reads the body from the HTTPRequest JSON message. However, if the HTTPRequest JSON message doesn't exist, it is assumed the entire bulk data is the body. Does the new code need to conform to this requirement? Does the request ever come across with just the body like this? Is there different behavior in different message protocol versions?

Any info I could get on this behavior would be very useful. Thanks.

@mikeburke106 mikeburke106 self-assigned this Feb 23, 2015
@mrapitis
Copy link
Contributor

I ran a round testing with the changes and so far things look good. As far as the weird logic you mentioned, we should be safe in removing this… at one point the json inside of the binary data that we received from the module for a policy table update request differed in format from the json (in the bin data) that we received for a vehicle update request. Since the module now provides the data in the same format for both types of onSystemRequests, we can safely remove the else if that you referenced.

@mikeburke106
Copy link
Contributor Author

Thanks @mrapitis. I will remove my TODO comment and this PR should be all set.

Signed-off-by: Mike Burke <mike@livioconnect.com>
@justinjdickow justinjdickow added REVIEW - accepted The reviewer has approved the PR and removed REVIEW - scheduled The reviewer has scheduled a review of this PR for the current sprint labels Mar 6, 2015
@mikeburke106 mikeburke106 mentioned this pull request Mar 9, 2015
@mikeburke106 mikeburke106 merged commit 3960cd5 into master Mar 9, 2015
@mikeburke106 mikeburke106 deleted the hotfix/improve_on_system_request branch March 9, 2015 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REVIEW - accepted The reviewer has approved the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OnSystemRequest has a lot of bugs
3 participants