Re: [ovirt-devel] Custom Properties code duplication

----- Original Message ----- > From: "Lior Vernia" <lvernia(a)redhat.com&gt; > To: "Vojtech Szocs" <vszocs(a)redhat.com&gt; > Cc: devel(a)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(a)redhat.com&gt; >>> To: "Lior Vernia" <lvernia(a)redhat.com&gt; >>> Cc: devel(a)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(a)redhat.com&gt; >>>> To: devel(a)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(a)ovirt.org >>>> http://lists.ovirt.org/mailman/listinfo/devel >>>> >>> _______________________________________________ >>> Devel mailing list >>> Devel(a)ovirt.org >>> http://lists.ovirt.org/mailman/listinfo/devel >>> > _______________________________________________ > Devel mailing list > Devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/devel >