OK, thanks! I will be rather busy for a few days, so it might will take a while until I check this out in detail. But I assume you know of things that you wanted to finish/polish anyway.
Something I have spotted with this quick look now is that executeResponse shouldn't be a field in FreeMarkerOnlineExecuteResource, as I suppose the resource object is a singleton used by multiple threads. Monday, August 31, 2015, 3:21:09 PM, Pradeep Murugesan wrote: > Hi Daniel, > > https://github.com/pradeepmurugesan/freemarker-online/commit/71e70d6caf9bead735ca0f2b6eb4f81c708a1922 > I have fixed the code review comments and integrated the same with UI. > Please let me know your reviews. > > Pradeep. > >> Date: Mon, 31 Aug 2015 12:22:17 +0200 >> From: [email protected] >> To: [email protected] >> Subject: Re: Rest Service for FreeMarkerOnline >> >> Certainly it's good enough to go ahead with the UI. >> >> Some random stuff I have noticed: >> >> - In "problems", the field names by convention should start with lower >> case, and they will have to be extracted to enums (or to static >> final String-s). >> >> - Class names will need some cleanup. Like FreeMarkerPayload and >> FreeMarkerResponse are in fact for the "execute" resource only, not >> for FM-Online in general, so I guess they should be ExecutreRequest >> and ExecuteResponse. FreeMarkerErrorReponse is for the FM *online* >> service, but that's already told by the package so... maybe just >> ErrorResponse. Anyway, these are just Alt+Shift+R things. >> >> -- >> Thanks, >> Daniel Dekany >> >> >> Sunday, August 30, 2015, 4:47:55 PM, Pradeep Murugesan wrote: >> >> > Hi Daniel, >> > https://github.com/pradeepmurugesan/freemarker-online/commit/80c984af1e810d69db2894146d67b52e2449a584 >> > >> > I have made the changes as per your comments below. Please review >> > and let me know if any corrections. >> > Still I didn't do the UI for this new Response structure. Thought >> > we will finalize the API Responses then we will get into the UI. >> > Pradeep. >> > >> >> Date: Sat, 29 Aug 2015 18:30:33 +0200 >> >> From: [email protected] >> >> To: [email protected] >> >> Subject: Re: Rest Service for FreeMarkerOnline >> >> >> >> Saturday, August 29, 2015, 4:12:16 PM, Pradeep Murugesan wrote: >> >> >> >> > okay.. >> >> > So almost all the errors we handle should go under Success right. >> >> > Having a structure like this would help I believe. >> >> > { error: true/false, outputTruncated: true/false, failureReason: >> >> > String output: String} >> >> >> >> That's for the HTTP 200 answers only, I assume. We miss the >> >> information about which field the failureReason applies to. So, I >> >> think, the cleanest and most flexible would be if we remove "error" >> >> (it's confusing and redundant anyway) and "failureReason", and instead >> >> add a "problems" JSON Object, in which the keys are the field names, >> >> and the value are the error descriptions. It's like some simple form >> >> processing answer. >> >> >> >> And then there are the HTTP 5xx errors. There I think we can get away >> >> with an "errorCode" and an "errorDescription" for now. >> >> >> >> -- >> >> Thanks, >> >> Daniel Dekany >> >> >> >> > Based on the error flag we can decide what to display. Sorry If I >> >> > am taking a longer time to get hold of things. >> >> > Pradeep. >> >> >> Date: Sat, 29 Aug 2015 14:34:58 +0200 >> >> >> From: [email protected] >> >> >> To: [email protected] >> >> >> Subject: Re: Rest Service for FreeMarkerOnline >> >> >> >> >> >> There are some cases whose distinction can be interesting for a UI >> >> >> (without much research, so it might not be accurate): >> >> >> - Successful Web service call results: >> >> >> - Template output, no template or data-model errors >> >> >> - Template output that's cut at a point as it was too long >> >> >> - Failed data model building >> >> >> - Failed template execution (position can be interesting here on the >> >> >> long run) >> >> >> - Service errors: I guess we just need the error message here. >> >> >> - A bit of both: Long running template timeout. Usually it's the >> >> >> user's mistake... usually. >> >> >> >> >> >> The *template* used by the current UI doesn't care about such details, >> >> >> but what it displays was already assembled by code that also belongs >> >> >> to the UI (i.e., to the JavaScript that processes the JSON response). >> >> >> That logic is certainly simplistic currently, but then, UI-s can >> >> >> change any time (and multiple different UI-s can co-exist), while Web >> >> >> service API-s less so. >> >> >> >> >> >> >> >> >> Saturday, August 29, 2015, 1:19:41 PM, Pradeep Murugesan wrote: >> >> >> >> >> >> > Hi Daniel, >> >> >> > The thing I am trying to understand here is the need for all the 3 >> >> >> > fields of FreeMarkerServiceResponse for the UI. >> >> >> > FreeMarkerOnline view while rendering the template just uses the 2 >> >> >> > parameters. >> >> >> > 1. Is there an error in the result 2. what is the error message >> >> >> > <#if hasResult> <div class="resultContainer"> <label >> >> >> > for="result">Result</label> <textarea id="result" >> >> >> > class="pure-input-1 source-code <#if errorResult> error</#if>" >> >> >> > readonly>${result}</textarea> </div></#if> >> >> >> > >> >> >> > So assuming we too need the same from Ajax requests >> >> >> > I am returning the result & the error is found based on the status >> >> >> > code of the response. >> >> >> > Kindly let me know your thoughts. >> >> >> > Pradeep. >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> >> Date: Fri, 28 Aug 2015 22:51:52 +0200 >> >> >> >> From: [email protected] >> >> >> >> To: [email protected] >> >> >> >> Subject: Re: Rest Service for FreeMarkerOnline >> >> >> >> >> >> >> >> The response should be JSON because we will need a few separate >> >> >> >> fields >> >> >> >> there. If you look at FreeMarkerServiceResponse, you will see 3 >> >> >> >> candidates, but then you will find more, as the thrown exceptions >> >> >> >> also >> >> >> >> carries information that's needed for the UI, as you can see in >> >> >> >> FreeMarkerService.calculateTemplateOutput. >> >> >> >> >> >> >> >> -- >> >> >> >> Thanks, >> >> >> >> Daniel Dekany >> >> >> >> >> >> >> >> >> >> >> >> Friday, August 28, 2015, 9:57:58 PM, Pradeep Murugesan wrote: >> >> >> >> >> >> >> >> > Sure Daniel, >> >> >> >> > I will change as per your comments. >> >> >> >> > A quick clarification regarding the json response can be like >> >> >> >> > following ? >> >> >> >> > { result: <output> } >> >> >> >> > Pradeep. >> >> >> >> > >> >> >> >> >> Date: Fri, 28 Aug 2015 21:26:58 +0200 >> >> >> >> >> From: [email protected] >> >> >> >> >> To: [email protected] >> >> >> >> >> Subject: Re: Rest Service for FreeMarkerOnline >> >> >> >> >> >> >> >> >> >> Friday, August 28, 2015, 7:53:45 PM, Pradeep Murugesan wrote: >> >> >> >> >> >> >> >> >> >> > Hi Daniel, >> >> >> >> >> > I have made the rest service up @ a new path /compile >> >> >> >> >> > The service takes the following json as input >> >> >> >> >> > { "template": "Hello ${user}", "dataModel": >> >> >> >> >> > "user=pradeep"} >> >> >> >> >> > and then compiles the template and dataModel and returns the >> >> >> >> >> > output. >> >> >> >> >> > https://github.com/pradeepmurugesan/freemarker-online/commit/10de59ac0db0bf0f79ab28214f50c851a5610e20 >> >> >> >> >> > >> >> >> >> >> > Please review the above commit and let me know if its ok. >> >> >> >> >> >> >> >> >> >> The response will have to be JSON as well (not TEXT_PLAIN), but I >> >> >> >> >> guess that was planned later. >> >> >> >> >> >> >> >> >> >> The usage of the "compile" term is confusing here, as you >> >> >> >> >> actually >> >> >> >> >> parse (aka. compile) and then "process" (aka. execute) here. The >> >> >> >> >> last >> >> >> >> >> naturally implies the first. So I guess it should be, like, >> >> >> >> >> "run" or >> >> >> >> >> "execute". >> >> >> >> >> >> >> >> >> >> Also, all the web service operations should go under /api/, and >> >> >> >> >> the UI >> >> >> >> >> outside it. >> >> >> >> >> >> >> >> >> >> > Will >> >> >> >> >> > Integrate with the UI. I have a questions though >> >> >> >> >> > 1. Should I modify in the same path as "/" or keeping it in a >> >> >> >> >> > separate path like "/compile" is fine ? >> >> >> >> >> >> >> >> >> >> The UI addresses should remain /, and the current web service >> >> >> >> >> should >> >> >> >> >> be under /api/run or something, I think. >> >> >> >> >> >> >> >> >> >> > Also I will write unit tests once we finalize the path. >> >> >> >> >> > Pradeep. >> >> >> >> >> >> >> >> >> >> -- >> >> >> >> >> Thanks, >> >> >> >> >> Daniel Dekany >> >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> > >> >> >> >> >> >> -- >> >> >> Thanks, >> >> >> Daniel Dekany >> > -- Thanks, Daniel Dekany
