FIREFLY-1331: Add Version validation for compatibility check between python client and firefly server#1936
Conversation
robyww
left a comment
robyww
left a comment
There was a problem hiding this comment.
I have not looked at the python yet but I think this is fine after the clean up I suggested.
|
|
||
| public static class GetVersion extends ServCommand { | ||
| public String doCommand(SrvParam params) throws Exception { | ||
| boolean raw = Boolean.parseBoolean(params.getOptional("raw")); |
There was a problem hiding this comment.
you should use
parmas.getOptionalBoolean(“raw”,false)| boolean raw = Boolean.parseBoolean(params.getOptional("raw")); | ||
| Object data = raw ? VersionUtil.getAppVersion() | ||
| : VersionUtil.getVersionInfo().stream().collect(Collectors.toMap(KeyVal::getKey, KeyVal::getValue)); | ||
| return Serializer.getJsonMapper().writeValueAsString(data); |
There was a problem hiding this comment.
it is a little verbose but wrap all out returns in an object.
this is SimpleJson taken from GetUserInfo (a few lines below) just to demonstrate
map.put( "success", true);
map.put("data", data);
There was a problem hiding this comment.
Using Jackson for JSON conversion is a good approach, and I’d like to gradually migrate SimpleJson to Jackson.
The added complexity here comes from converting a list of <name, value> pairs into a map. This could be simplified if VersionUtil.getVersionInfo() returned a Map directly. From what I’ve seen, all current usages end up converting or streaming the list anyway to access the values.
There was a problem hiding this comment.
it is a little verbose but wrap all out returns in an object.
I was reviewing from my ipad last night and the above line got garbled. Let me try that again.
Our standard return style is a little verbose, but we always wrap the json data returned in an outer json object. You are just returning the data and it should be wrapped in an object {status: true, data:data}
There was a problem hiding this comment.
Oh I see. I jumped the gun and merged already. Should I revert this merge and make this change in this branch? OR just push this change directly to dev?
| boolean raw = Boolean.parseBoolean(params.getOptional("raw")); | ||
| Object data = raw ? VersionUtil.getAppVersion() | ||
| : VersionUtil.getVersionInfo().stream().collect(Collectors.toMap(KeyVal::getKey, KeyVal::getValue)); | ||
| return Serializer.getJsonMapper().writeValueAsString(data); |
There was a problem hiding this comment.
Using Jackson for JSON conversion is a good approach, and I’d like to gradually migrate SimpleJson to Jackson.
The added complexity here comes from converting a list of <name, value> pairs into a map. This could be simplified if VersionUtil.getVersionInfo() returned a Map directly. From what I’ve seen, all current usages end up converting or streaming the list anyway to access the values.
FIREFLY-1331: Add VersionServlet to return application version information as JSON FIREFLY-1331: Add raw param to VersionServlet FIREFLY-1331: Change Version Servlet to Cmd FIREFLY-1331: Simplify the JSON generation for version FIREFLY-1331: Apply feedback from PR
jaladh-singhal
force-pushed
the
FIREFLY-1331-version-validation
branch
from
ee1181c to
19c2722
Compare
3 participants
Fixes FIREFLY-1331
Exposed the version info as
cmd=CmdVersionat the commands servelet endpoint (CmdSrv/sync).Firefly client is not using the
raw=trueparam but it maybe helpful in future if validation logic becomes complicated.See Firefly client PR: Caltech-IPAC/firefly_client#79
Testing
http://aac8ce8bef8604e7ca9d38b934ab83c5-4855331a01d9fb87.elb.us-east-1.amazonaws.com/firefly-1331/firefly (yes I managed to deploy the PR build on AWS using helm!)
Open the version dialog and check if it matches the JSON returned by
/CmdSrv/sync?cmd=CmdVersionquery paramsCheck that
/CmdSrv/sync?cmd=CmdVersion?raw=truequery params returns primitive version object which can also be useful in some caseshttps://firefly-1331-version-validation.irsakubedev.ipac.caltech.edu/irsaviewer/CmdSrv/sync?cmd=CmdVersion