I have looked at it now. I haven't seen any big issues. I ran into small things (some of you may know about):
- Bug: Template-related errors point to the dataModel in the response JSON - Bug: The validForm function in script.js... you will see if you look at it. I think blocking submissions with empty template is good, and empty data-model is not a problem. - All the old non-AJAX code can be removed now - Cleaning up stuff like unnecessary buildFreeMarkerResponse method, debug logs in scrip.js, etc. Also naming cleanup, like FreeMarkerError to ErrorCode. And use name() instead of toString() for enums. - Submitting makes the edit boxes "flash", as you quickly disable and enable them. I think it's kind of unpleasant. Maybe you just shouldn't disable them, only the submitt button, and then show the usual spinning wheel over the output area. Or something like that. - The selenium JUnit test fails. I guess it won't work with AJAX anyway, so it will have to be removed, but I haven't looked into it. - Can `Map<String, String> problems` be a `Map<ExecuteResourceErrorFields, String>`? I'm not sure if Jeresy will handle that, but if it does, it's cleaner. Monday, September 7, 2015, 4:25:12 PM, Pradeep Murugesan wrote: > Hi Daniel, Got a chance to review this ? > Pradeep. > >> Date: Thu, 3 Sep 2015 22:17:38 +0200 >> From: [email protected] >> To: [email protected] >> Subject: Re: Rest Service for FreeMarkerOnline >> >> I will... tomorrow or at the weekend. >> >> >> Thursday, September 3, 2015, 8:27:15 PM, Pradeep Murugesan wrote: >> >> > Hi Daniel, >> > Kindly review when ever you find time. >> > https://github.com/pradeepmurugesan/freemarker-online/commit/4445f45997c1eaf76c6b4e13e8c2054ee163e034https://github.com/pradeepmurugesan/freemarker-online/commit/d8e25be864a25596d29f875fa845695d02168e26 >> > Pradeep. >> >> From: [email protected] >> >> Subject: Re: Rest Service for FreeMarkerOnline >> >> Date: Tue, 1 Sep 2015 23:40:38 +0530 >> >> To: [email protected] >> >> >> >> Yup.. Finishing the current task is the priority. >> >> >> >> Pradeep. >> >> >> >> > On 01-Sep-2015, at 10:09 pm, "Daniel Dekany" <[email protected]> >> >> > wrote: >> >> > >> >> > Tuesday, September 1, 2015, 10:01:31 AM, Pradeep Murugesan wrote: >> >> > >> >> >> Sure Daniel, >> >> >> I will change the initialization @ method level. >> >> >> I have a couple of questions. Please reply when you find time. Sorry >> >> >> If its too naive. >> >> >> 1. I tried to check whether jersey's resource methods are singleton >> >> >> but found in the doc that they spawn a thread for each request. And >> >> >> we may turn on the singleton thing if required through some explicit >> >> >> settings. >> >> > >> >> > It's Spring bean (@Component), and those are singletons by default. >> >> > Even if wasn't a singleton, I see no reason for a response object to >> >> > be recycled between requests. It can be the source for hideous bugs, >> >> > as you start with a dirty object, etc. >> >> > >> >> >> I browsed our code to see if we have some setting but >> >> >> couldn't find anything. >> >> > >> >> > The easiest is just trying it with some log messages. >> >> > >> >> >> In 3.4 of >> >> >> https://jersey.java.net/nonav/documentation/latest/user-guide.html#d0e2586 >> >> >> we can see they mentioning the above. >> >> >> 2. In the mean time I am trying to get hold of how freemarker >> >> >> template engine works by trying to fix some low hanging bugs so that >> >> >> I can understand the architecture better. I am browsing for issues >> >> >> here http://sourceforge.net/p/freemarker/bugs/. Kindly let me know a >> >> >> good place or a minor bug to start with. >> >> > >> >> > Most bugs will be stuff that isn't fixed because it's tricky to fix >> >> > them, often because of backward compatibility requirements... But for >> >> > example here's this new one: >> >> > http://sourceforge.net/p/freemarker/bugs/434/ Could be checked if it's >> >> > true, and why it happens. But of course deal what we feel like with. >> >> > >> >> > (It's also a good idea to polishing what you have started, like >> >> > writing the JUnit tests, etc.) >> >> > >> >> >> Pradeep. >> >> >> >> >> >>> Date: Mon, 31 Aug 2015 20:49:34 +0200 >> >> >>> From: [email protected] >> >> >>> To: [email protected] >> >> >>> Subject: Re: Rest Service for FreeMarkerOnline >> >> >>> >> >> >>> 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 >> >> >>> >> >> >> >> >> > >> >> > -- >> >> > Thanks, >> >> > Daniel Dekany >> >> > >> > >> >> -- >> Thanks, >> Daniel Dekany >> > -- Thanks, Daniel Dekany
