Michael Pasternak has posted comments on this change. Change subject: restapi: default version to 'general' ......................................................................
Patch Set 3: On 01/21/2013 02:56 PM, eco...@redhat.com wrote: >> > if you want to force users specifying version, leave only one constructor > we cannot do that, as the class is marked as Serializable [since it is being > serialized/deserialized between client (javascript) and server], therefore it > must contain a default c'tor. i was not talking about of default constructor, but about GetConfigurationValueParameters(ConfigurationValues cVal), i.e without the version ... > >> > and replace 'String version' with 'Version version', > not sure how it will help for our issue: if Version can be set as 'null' - it > doesn't help; if Version cannot be set as 'null', then again - we will only > cover-up the problem (unless implementing *along with* your c'tor suggestion > above, which again - is impossible). it's only for physiological effect that consumers of this action will understand that they have to supply the version, but both String & Version are reference types indeed. > >> > in addition maybe we should add validation to make sure version get >> > specified, > if you mean that the GetConfigurationValue query should *fail* in case a > 'null' Version value has been provided: That is a great suggested and indeed > can help, however can result in many regressions. So before doing that - need > to make sure first that we are passing a non-null version in the existing > GetConfigurationValue calls in the code (in order to avoid these regressions > as much as we can), which is what this patch is all about. > >> > current solution is only temporary resolution of the problem as you have >> > no tool to prevent someone passing NULL as version tomorrow. > Indeed; I am all for the validation solution that you suggested (if I > understood you correctly), but it can (and should, IMO) be handled in a > separate patch; in any case - no matter how we decide to ultimately solve it > - need to make sure first that the GetConfigurationValue calls provide a > non-null version value. i understand this Einav, my only problem is that we still don't have any tool to prevent this from happen and as such i think that setting defaultConfigurationVersion in GetConfigurationValueParameters.privateVersion will do much better job (for now till we have validation) to reach the goal of this patch, don't you think? (btw change introduced in this patch could be done when we have query validation) -- To view, visit http://gerrit.ovirt.org/11173 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I946c9925f3e0b94a9dad86f0958c2f9e1d0ab5c8 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
