[ovirt-devel] Custom Properties code duplication

Wed May 7 13:02:45 UTC 2014

----- 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?
2) no dependency separation in UI code.
3) complexity.
4) compatibility.

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
> 