OnSystemRequest has a lot of bugs #86
Comments
|
Now that some of the dust has settled and the JSON within OnSystemRequests binary data has become better defined we can perform some cleanup on the class. Yes initially, the class was written quickly as we were concerned with getting policy table updates working asap. Re-writing the entire class is overkill. I would suggest we include this as an agenda item on our next SmartDeviceLink open source review meeting. |
mikeburke106
added
the
REVIEW - scheduled
|
Ok, scheduled for review. Re-writing a class isn't the end of the world - obviously most of the setters and getters would remain the same, so it won't change entirely. I'm basically just trying to say this won't be a trivial fix and will require a good amount of change and will need to be reviewed thoroughly. This class will only work correctly if the hashtable constructor is used. There's no way to set the headers and body data other than through the hashtable constructor, which makes unit testing this class very difficult if not impossible. If we attempt to create a class like we do all the other RPCs, it will look something like this: // BIN_DATA, HEADERS and BODY defined as constants
OnSystemRequest onSystemRequest = new OnSystemRequest();
// does practically nothing because bulk data is only used in hashtable constructor
onSystemRequest.setBinData(BIN_DATA);
// CRASH - NullPointerException due to `httpreqparams` not being initialized
onSystemRequest.setHeaders(HEADERS);
onSystemRequest.setBody(BODY);
// always returns null since `httpreqparams` and `myJSONObj` are both null
String body = onSystemRequest.getBody();
// always returns null since `httpreqparams` is null
Headers headers = onSystemRequest.getHeader();This needs to change soon because Livio's co-ops are working on writing unit tests for the mobile libraries and we need to be able to test all the RPC objects against the spec. |
OnSystemRequest doesn't really follow the guidelines and practices of the rest of the project. It seems like it was hurriedly thrown together and therefore, it has a lot of bugs.
I can see here that the class is attempting to create a JSON object from the binary bulk data. If the BulkData is valid, it will save 2 global variables,
httpreqparamsandmyJSONObj. If the bulk data is not valid, both of these variables will be null.In the setHeaders method, if the
httpreqparamsis null, it will cause a NullPointerException crash.The body of the request is even more confusing. In the getBody method, it is trying to read from
httpreqparamsif that's not null. Even thoughhttpreqparamsis already the object returned from the"HTTPRequest"key, the code is reading the exact same value from the original bulk data and then reading the"body"key from that. If thehttpreqparamsis null, the code just returns the original bulk data as a string, which I don't really understand. Why are we just blindly assuming that whatever is in the bulk data is the body of an HTTP request in this one case? In addition, in the setBody method, the body information is stored in the parameters hashtable, which isn't even where it came from and obviously doesn't belong here.I think this whole class should be re-written, to be honest.
The text was updated successfully, but these errors were encountered: