[ovirt-devel] Custom Properties code duplication

Wed Apr 30 15:01:22 UTC 2014

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.

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
> 