[ovirt-devel] Custom Properties code duplication

Thu May 8 07:16:56 UTC 2014

----- Original Message -----
> From: "Gilad Chaplik" <gchaplik at redhat.com>
> To: "Lior Vernia" <lvernia at redhat.com>
> Cc: devel at 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 at redhat.com>
> > To: "Gilad Chaplik" <gchaplik at redhat.com>
> > Cc: "Vojtech Szocs" <vszocs at redhat.com>, devel at 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 at redhat.com>
> > >> To: "Gilad Chaplik" <gchaplik at redhat.com>
> > >> Cc: "Vojtech Szocs" <vszocs at redhat.com>, devel at 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 at redhat.com>
> > >>>> To: "Vojtech Szocs" <vszocs at redhat.com>
> > >>>> Cc: devel at 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 at redhat.com>
> > >>>>>> To: "Lior Vernia" <lvernia at redhat.com>
> > >>>>>> Cc: devel at 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 at redhat.com>
> > >>>>>>> To: devel at 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 at ovirt.org
> > >>>>>>> http://lists.ovirt.org/mailman/listinfo/devel
> > >>>>>>>
> > >>>>>> _______________________________________________
> > >>>>>> Devel mailing list
> > >>>>>> Devel at ovirt.org
> > >>>>>> http://lists.ovirt.org/mailman/listinfo/devel
> > >>>>>>
> > >>>> _______________________________________________
> > >>>> Devel mailing list
> > >>>> Devel at ovirt.org
> > >>>> http://lists.ovirt.org/mailman/listinfo/devel
> > >>>>
> > >>
> > 
> _______________________________________________
> Devel mailing list
> Devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/devel
>