subject:"\[ovirt\-devel\] Custom Properties code duplication"

Re: [ovirt-devel] Custom Properties code duplication

2014-05-08 Thread Gilad Chaplik

- 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.

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.

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.

4. Optionally change the implementation of custom properties

Re: [ovirt-devel] Custom Properties code duplication

2014-05-08 Thread Gilad Chaplik

- 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

Re: [ovirt-devel] Custom Properties code duplication

2014-05-07 Thread Lior Vernia

Hey guys (and gals),

A few patches are up for review starting at:
http://gerrit.ovirt.org/#/c/27383

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: 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.

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.

gwt-user-{version}-sources.jar/com/google/gwt/emul/java/lang/String.java

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.

2. Modify KeyValueModel to delegate to the common utilities instead of
duplicating code, e.g. for validation.

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.

4. Optionally change the implementation of custom properties members in
entities (e.g. VM) from String to MapString, String, which would
obviate the need for different conversion methods between String/Map -
(de)serialization would only be required in DB interaction.

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

Re: [ovirt-devel] Custom Properties code duplication

2014-05-07 Thread Gilad Chaplik

- 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

Hey guys (and gals),

A few patches are up for review starting at:
http://gerrit.ovirt.org/#/c/27383

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: 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.

Lior mentioned above the possibility of using String methods, I assume
by this he means java.lang.String.matches(String) and similar methods.

gwt-user-{version}-sources.jar/com/google/gwt/emul/java/lang/String.java

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.

2. Modify KeyValueModel to delegate to the common utilities instead of
duplicating code, e.g. for validation.

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.

3,4 Agreed - good points.

The main argument against this would be that the frontend is to be
moved
over