----- Original Message ----- > From: "Gilad Chaplik" <gchap...@redhat.com> > To: "Lior Vernia" <lver...@redhat.com> > Cc: devel@ovirt.org > Sent: Thursday, May 8, 2014 9:50:19 AM > Subject: Re: [ovirt-devel] Custom Properties code duplication > > ----- Original Message ----- > > From: "Lior Vernia" <lver...@redhat.com> > > To: "Gilad Chaplik" <gchap...@redhat.com> > > Cc: "Vojtech Szocs" <vsz...@redhat.com>, devel@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" <lver...@redhat.com> > > >> To: "Gilad Chaplik" <gchap...@redhat.com> > > >> Cc: "Vojtech Szocs" <vsz...@redhat.com>, devel@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" <lver...@redhat.com> > > >>>> To: "Vojtech Szocs" <vsz...@redhat.com> > > >>>> Cc: devel@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" <yzasl...@redhat.com> > > >>>>>> To: "Lior Vernia" <lver...@redhat.com> > > >>>>>> Cc: devel@ovirt.org > > >>>>>> Sent: Wednesday, April 30, 2014 3:28:06 PM > > >>>>>> Subject: Re: [ovirt-devel] Custom Properties code duplication > > >>>>>> > > >>>>>> > > >>>>>> > > >>>>>> ----- Original Message ----- > > >>>>>>> From: "Lior Vernia" <lver...@redhat.com> > > >>>>>>> To: devel@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:master+topic:NetworkCustomProperties,n,z)
common is part of backend = 'You could still compile the UI without the engine' is inaccurate. > > > > > >> > > >>> 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@ovirt.org > > >>>>>>> http://lists.ovirt.org/mailman/listinfo/devel > > >>>>>>> > > >>>>>> _______________________________________________ > > >>>>>> Devel mailing list > > >>>>>> Devel@ovirt.org > > >>>>>> http://lists.ovirt.org/mailman/listinfo/devel > > >>>>>> > > >>>> _______________________________________________ > > >>>> Devel mailing list > > >>>> Devel@ovirt.org > > >>>> http://lists.ovirt.org/mailman/listinfo/devel > > >>>> > > >> > > > _______________________________________________ > Devel mailing list > Devel@ovirt.org > http://lists.ovirt.org/mailman/listinfo/devel > _______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel