Re: [ovirt-devel] Custom Properties code duplication

From: "Gilad Chaplik" <gchaplik(a)redhat.com&gt; To: "Lior Vernia" <lvernia(a)redhat.com&gt; Cc: devel(a)ovirt.org Sent: Thursday, May 8, 2014 9:50:19 AM Subject: Re: [ovirt-devel] Custom Properties code duplication ----- Original Message ----- > From: "Lior Vernia" <lvernia(a)redhat.com&gt; > To: "Gilad Chaplik" <gchaplik(a)redhat.com&gt; > Cc: "Vojtech Szocs" <vszocs(a)redhat.com&gt;, devel(a)ovirt.org > Sent: Wednesday, May 7, 2014 7:29:23 PM > Subject: Re: [ovirt-devel] Custom Properties code duplication > > > > On 07/05/14 18:57, Gilad Chaplik wrote: > > ----- Original Message ----- > >> From: "Lior Vernia" <lvernia(a)redhat.com&gt; > >> To: "Gilad Chaplik" <gchaplik(a)redhat.com&gt; > >> Cc: "Vojtech Szocs" <vszocs(a)redhat.com&gt;, devel(a)ovirt.org > >> Sent: Wednesday, May 7, 2014 5:32:38 PM > >> Subject: Re: [ovirt-devel] Custom Properties code duplication > >> > >> > >> > >> On 07/05/14 16:02, Gilad Chaplik wrote: > >>> ----- Original Message ----- > >>>> From: "Lior Vernia" <lvernia(a)redhat.com&gt; > >>>> To: "Vojtech Szocs" <vszocs(a)redhat.com&gt; > >>>> Cc: devel(a)ovirt.org > >>>> Sent: Monday, May 5, 2014 7:08:23 PM > >>>> Subject: Re: [ovirt-devel] Custom Properties code duplication > >>>> > >>>> Hey guys (and gals), > >>>> > >>>> A few patches are up for review starting at: > >>>> http://gerrit.ovirt.org/#/c/27383 > >>>> > >>>> In total, about 250 lines of code removed, hopefully not at the cost > >>>> of > >>>> regression. I put down some reviewers as I saw fit, but everyone can > >>>> feel free to add themselves. Summary of what was done compared to the > >>>> original plan: > >>>> > >>>> 1. Removed said dependencies, except for DeviceCustomPropertiesUtils > >>>> that was using the capture groups features of Pattern, and thus > >>>> remained > >>>> in the utils project. > >>>> > >>>> 2. Done. > >>>> > >>>> 3. Done. > >>>> > >>>> 4. Almost didn't touch this, it seems to involve a lot of moving > >>>> parts. > >>>> > >>>> Yours, Lior. > >>>> > >>>> On 30/04/14 18:01, Vojtech Szocs wrote: > >>>>> Hi Lior, please find my comments inline. > >>>>> > >>>>> > >>>>> ----- Original Message ----- > >>>>>> From: "Yair Zaslavsky" <yzaslavs(a)redhat.com&gt; > >>>>>> To: "Lior Vernia" <lvernia(a)redhat.com&gt; > >>>>>> Cc: devel(a)ovirt.org > >>>>>> Sent: Wednesday, April 30, 2014 3:28:06 PM > >>>>>> Subject: Re: [ovirt-devel] Custom Properties code duplication > >>>>>> > >>>>>> > >>>>>> > >>>>>> ----- Original Message ----- > >>>>>>> From: "Lior Vernia" <lvernia(a)redhat.com&gt; > >>>>>>> To: devel(a)ovirt.org > >>>>>>> Sent: Wednesday, April 30, 2014 3:52:16 PM > >>>>>>> Subject: [ovirt-devel] Custom Properties code duplication > >>>>>>> > >>>>>>> Hello, > >>>>>>> > >>>>>>> While adding network custom properties towards oVirt 3.5, I got to > >>>>>>> take > >>>>>>> a good look at the custom properties code in the backend and > >>>>>>> frontend. > >>>>>>> It seems to me like there's a lot of code duplication, and I would > >>>>>>> like > >>>>>>> to suggest the following refactoring: > >>>>>>> > >>>>>>> 1. Remove dependencies on Pattern/Matcher and ApacheCommons methods > >>>>>>> from > >>>>>>> *CustomPropertiesUtils.java, to make them compilable with GWT, and > >>>>>>> move > >>>>>>> these utilities to the common package. The usage of the said > >>>>>>> methods > >>>>>>> is > >>>>>>> minimal and could easily be replaced with String methods, etc. > >>>>> > >>>>> +1 > >>>>> > >>>>>> > >>>>>> > >>>>>> In general I am in favor, but how are you going to perform the regex > >>>>>> validation of values? > >>>>>> for example , with vm custom properties, you have - sap_agent that > >>>>>> can > >>>>>> be > >>>>>> either true or false. > >>>>>> So you need to validate both at the client and the engine, right? > >>>>> > >>>>> Lior mentioned above the possibility of using String methods, I > >>>>> assume > >>>>> by this he means java.lang.String.matches(String) and similar > >>>>> methods. > >>>>> > >>>>> During GWT compilation, JRE standard String implementation is > >>>>> replaced > >>>>> by emulated (GWT-friendly) String implementation, which implements > >>>>> methods like matches(String) using JavaScript RegExp object. You can > >>>>> find this emulated implementation here: > >>>>> > >>>>> gwt-user-{version}-sources.jar/com/google/gwt/emul/java/lang/String.java > >>>>> > >>>>> Another alternative is to use GWT's built-in regex support through > >>>>> com.google.gwt.regexp.shared.RegExp class. GWT's RegExp class has two > >>>>> implementations, default one using JRE Pattern/Matcher, emulated one > >>>>> using JavaScript RegExp object. The advantage is (mostly) consistent > >>>>> regex support on both client and server, the disadvantage is server's > >>>>> dependency on GWT JAR. (I don't think we want this.) > >>>>> > >>>>> For simple regex matches, I'd suggest to simply go with String > >>>>> approach. > >>>>> > >>>>> For complex regex matches, we can use JRE Pattern/Matcher on server, > >>>>> and emulate given implementation using GWT RegExp on client. > >>>>> > >>>>> Note that there are (slight) differences between JRE's > >>>>> Pattern/Matcher > >>>>> and JavaScript's RegExp object syntax/behavior. Check GWT RegExp > >>>>> class > >>>>> Javadoc to see details (for simple cases, it's not a big deal, > >>>>> though). > >>>>> > >>>>>> > >>>>>>> > >>>>>>> 2. Modify KeyValueModel to delegate to the common utilities instead > >>>>>>> of > >>>>>>> duplicating code, e.g. for validation. > >>>>> > >>>>> +1 > >>>>> > >>>>> I see that KeyValueModel uses RegexValidation class that delegates to > >>>>> compat's Regex class. Just like above, default Regex class utilizes > >>>>> JRE Pattern/Matcher but client uses emuluated implementation: > >>>>> > >>>>> gwt-extension/src/main/java/org/ovirt/engine/ui/uioverrides/org/ovirt/engine/core/compat/Regex.java > >>>>> > >>>>> and this emulated implementation uses GWT RegExp class. > >>>>> > >>>>>>> > >>>>>>> 3. Move some validation, which is relevant to all custom properties > >>>>>>> (e.g. duplicate keys), from specific utils (e.g. VmPropertiesUtils) > >>>>>>> up > >>>>>>> to the shared CustomPropertiesUtils. > >>>>> > >>>>> +1 > >>>>> > >>>>>>> > >>>>>>> 4. Optionally change the implementation of custom properties > >>>>>>> members > >>>>>>> in > >>>>>>> entities (e.g. VM) from String to Map<String, String>, which would > >>>>>>> obviate the need for different conversion methods between > >>>>>>> String/Map > >>>>>>> - > >>>>>>> (de)serialization would only be required in DB interaction. > >>>>> > >>>>> +1 > >>>>> > >>>>>> > >>>>>> 3,4 Agreed - good points. > >>>>>> > >>>>>>> > >>>>>>> The main argument against this would be that the frontend is to be > >>>>>>> moved > >>>>>>> over the REST, and might not be written in Java much longer anyway. > >>>>> > >>>>> :) I think there's a misunderstanding regarding our move to REST API. > >>> > >>> I think there isn't any misunderstanding here. I think that common code > >>> is > >>> not the best practice here, as Lior mentioned briefly. > >>> IMO one of the main reasons of using the REST is repo/project > >>> separation, > >>> some points to consider: > >>> > >>> 1) who will maintain the common code, both UI and core maintainers, > >>> we're > >>> missing the point, no? > >> > >> That's not how I see it. Common code is exactly that - common. If the > >> validation is the same in the backend and in the frontend (which it > >> often is), then it's better to share the one library between them than > >> write the code twice. > >> > >> The alternative would be to query the backend for validation, but in > >> some contexts it just wouldn't be responsive enough (e.g. dragging in > >> setup networks). > > > > As i see it, there are JS (client) validations server validations, they > > are > > inherently different. > > For responsive validations you have JS validations (duplicated as you > > call > > it)/ or you can query the server for generated JS file (probably not > > going > > to happen). > > You didn't answer my question about maintainers. > > > > Who the maintainers are is a minor detail in my opinion, I didn't even > realise this was your main point. They could stay backend maintainers > and approve of fixes that make stuff usable by the frontend. :-) okay, sorry for not being clear enough. So this is a major detail IMO, the motivation in separation, in to decouple the ui from backend (implies from the word separation). > > In my opinion the validations are inherently similar. If we take the on logically we can still debate :) but not programmatically, different environment/set of capabilities. > setup networks dialog as an example, the rules for what comprises a > valid configuration are the same, so there's no good reason for all the > logic to be duplicated (even though it is). Same goes for character > validation when entering the name of a new entity - usually the same > regex is checked in the backend and the frontend (although it's copied > and pasted rather than shared). you gave a wonderful example where we need to have duplicate code: regex (http://en.wikipedia.org/wiki/Comparison_of_regular_expression_engines) > > >> > >>> 2) no dependency separation in UI code. > >> > >> You could still compile the UI without the engine and the engine without > >> the UI and everything would work fine. They just both depend on the same > >> library (but it doesn't have to be the same version in both). > > > > According to what you're saying, your patches should get rejected, > > because > > you move the code to Common project and that's what we're trying to > > avoid. > > > > Please point me to where I said I thought we should avoid moving code to > common. referring to the patches you submitted (http://gerrit.ovirt.org/#/q/status:open+project:ovirt-engine+branch:maste...)

> > >> > >>> 3) complexity. > >> > >> This is part of what having common libraries aims to reduce. > > > > what is 'common' lib btw?, backend/manager/modules/common is the wrong > > answer. > > > > To me common is the right answer. It hosts all the common business > entities that are used by the backend, the frontend and the REST. I've > used some networking constants used for backend validation in frontend > validation. And now the custom properties validation is hosted there and > shared. now it's not the case, common is a server lib (used by the client as well). > > >> > >>> 4) compatibility. > >> > >> As mentioned above, I don't see how compatibility is compromised. > > > > As I said, +1 for your patches, because they're already submitted, and > > they're good for the very immediate future (fyi, regressions is sth that > > I > > didn't consider, this will be yours and the reviewers responsibility), > > but > > note that the code will be duplicated soon, and altered to meet REST > > entities; all of this will happen in the near future (near is relative > > :-)). > > > > That's not what I understood about the move over REST. I thought another > layer of code would be added between the frontend and backend to > translate backend entities to frontend entities if needed, and that all > the current frontend code would stay as is. We might over time pick at > the Java code and incrementally replace it with JavaScript code where > beneficial, but again this doesn't mean duplication. The move over REST is removing any dependency to backend jars. according to last meeting I attended, we will get the data from the RESTful API using the JS-SDK, the UI will have it's 'own' BEs, built on top JS entities (java). There will be a second project which will help in integrating JS-SDK to ui's needs. The client shouldn't know what is the word VDS (and other BE entities in that matter), and that includes any translation code. All of this thread started from the work misunderstanding :-) Vojtech can you shed some light? > > > Thanks, > > Gilad. > > > >> > >>> > >>> In conclusion, I don't understand the motivation, but... as I commented > >>> in > >>> the patch... +1. > >>> > >>> Thanks, > >>> Gilad. > >>> > >>>>> > >>>>> The plan is to have JavaScript SDK for working with REST API (so that > >>>>> any JavaScript client can work with Engine, be it web application in > >>>>> browser, server application on node.js etc.), and for our GWT-based > >>>>> UI, > >>>>> generate GWT/Java overlay code that delegates (via JSNI) to > >>>>> JavaScript. > >>>>> > >>>>> This is similar approach to projects like SmartGWT which are simply > >>>>> overlays (wrappers) to native JavaScript library (like SmartClient). > >>>>> > >>>>> So the frontend code will still be GWT/Java, it will just consume > >>>>> JavaScript SDK via the above mentioned overlay code for seamless > >>>>> SDK user experience in GWT/Java context. > >>>>> > >>>>> We may think of implementing parts of UI in JavaScript, though. > >>>>> For example, utilizing UI plugins to implement different UI parts > >>>>> as plugins, possibly by using JavaScript directly (which could use > >>>>> JavaScript SDK). However, this is something different and requires > >>>>> deeper thought. > >>>>> > >>>>>>> > >>>>>>> However, to my understanding, there's some time until these changes > >>>>>>> take > >>>>>>> effect. And even if the frontend is to be written in JavaScript, at > >>>>>>> least initially the existing frontend code will have to still be > >>>>>>> used > >>>>>>> somehow (e.g. auto-translated to JavaScript). That is to say, this > >>>>>>> refactoring might still be beneficial for the not-so-short term. > >>>>> > >>>>> I think this refactoring will be beneficial also in long term :) > >>>>> > >>>>>> > >>>>>> I agree with you here. > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>>> > >>>>>>> Before going through with this, I wanted to ask for your thoughts > >>>>>>> and > >>>>>>> to > >>>>>>> hear any specific objections to the proposed changes. > >>>>>>> > >>>>>>> Yours, Lior. > >>>>>>> _______________________________________________ > >>>>>>> Devel mailing list > >>>>>>> Devel(a)ovirt.org > >>>>>>> http://lists.ovirt.org/mailman/listinfo/devel > >>>>>>> > >>>>>> _______________________________________________ > >>>>>> Devel mailing list > >>>>>> Devel(a)ovirt.org > >>>>>> http://lists.ovirt.org/mailman/listinfo/devel > >>>>>> > >>>> _______________________________________________ > >>>> Devel mailing list > >>>> Devel(a)ovirt.org > >>>> http://lists.ovirt.org/mailman/listinfo/devel > >>>> > >> > _______________________________________________ Devel mailing list Devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/devel