There is already AOP happening on every XyzService.saveXyz call. So we're actually already paying the significant overhead. I think that reflection+iterating over properties only on saveXyz methods is vanishingly small additional overhead to the fact that every service call is AOPd.
-Darius On Thu, Sep 22, 2011 at 12:51 PM, Burke Mamlin <[email protected]>wrote: > On Thu, Sep 22, 2011 at 3:03 PM, Darius Jazayeri > <[email protected]>wrote: > >> So, does that mean you're fine with automagically changing empty String >> properties of OpenmrsObjects to null before saving to the database? > > > Yes. I love magic (I'm no muggle!)… as long as it's not going to add > several milliseconds or more to every API call. If doing it automagically > involves AOP or some other hook that is going to add (or contribute to) a > measurable performance hit, I would rather find a way to manage it > explicitly (via hibernate configuration, a custom hibernate type, and/or > code reviews). > > -Burke > > >> -Darius >> >> On Thu, Sep 22, 2011 at 11:43 AM, Burke Mamlin >> <[email protected]>wrote: >> >>> On Thu, Sep 22, 2011 at 9:18 AM, Darius Jazayeri < >>> [email protected]> wrote: >>> >>>> I still think that we *should* be automagic. >>>> >>>> I cannot think of a single use case where you want "" and null to be >>>> distinguishable. And Ben, your worry that "I fear there might be places >>>> where a column is not nullable but the user doesn't want a value there" >>>> is exactly why I *don't* want to allow "". If the field is required, >>>> the whole point is that you are not supposed to be able to store an empty >>>> value. >>>> >>>> Burke, Roger, others, can anyone give me an example of a real use case >>>> in OpenMRS where we want null and "" to be semantically different in a >>>> database column? >>>> >>> >>> The only case of "" and null being semantically different would be cases >>> where an empty string is a valid response and distinguishing "answered with >>> blank" vs. not answered becomes important. I can't think of a case where an >>> "empty" response would be important to capture. >>> >>> -Burke >>> >>> >>>> >>>> -Darius >>>> >>>> On Thu, Sep 22, 2011 at 5:50 AM, Friedman, Roger (CDC/CGH/DGHA) (CTR) < >>>> [email protected]> wrote: >>>> >>>>> Yes I think this is true.**** >>>>> >>>>> ** ** >>>>> >>>>> However, I think I agree with Darius that in the context of attributes, >>>>> attempting to create a string-valued property with a null value should >>>>> result in no new property being created. This could/should be extended to >>>>> the empty string if the distinction between the two is difficult to >>>>> maintain >>>>> when moving from the gui to the api. This could also be extended to >>>>> saying >>>>> that attempting to update a property value to null would be handled the >>>>> same >>>>> as voiding the property.**** >>>>> >>>>> ** ** >>>>> >>>>> *From:* [email protected] [mailto:[email protected]] *On Behalf Of *Ben >>>>> Wolfe >>>>> *Sent:* Thursday, September 22, 2011 7:54 AM >>>>> >>>>> *To:* [email protected] >>>>> *Subject:* Re: [OPENMRS-DEV] Empty string fields vs null fields**** >>>>> >>>>> ** ** >>>>> >>>>> So our take-away here is to not do automagic but instead tell api users >>>>> to put trimming/nulling into either their save methods or their setters? >>>>> >>>>> Ben**** >>>>> >>>>> On Mon, Sep 12, 2011 at 5:24 PM, Darius Jazayeri < >>>>> [email protected]> wrote:**** >>>>> >>>>> Actually we've been treating the fact that when you save a patient, you >>>>> get a bunch of person attributes with value=empty-string as a bug. (And >>>>> it's >>>>> a bug that occurs precisely because of this null != empty string behavior. >>>>> I.e. >95% of the time the significance of that difference is that someone >>>>> forgets to treat a submitted empty string as null in a controller.)*** >>>>> * >>>>> >>>>> ** ** >>>>> >>>>> -Darius**** >>>>> >>>>> ** ** >>>>> >>>>> On Mon, Sep 12, 2011 at 5:56 AM, Friedman, Roger (CDC/CGH/DGHA) (CTR) < >>>>> [email protected]> wrote:**** >>>>> >>>>> One reason to keep the empty string rather than null is collating >>>>> order; null typically appears last, empty string first. >>>>> >>>>> Re what Ben says about person_attribute, I believe it is conventional >>>>> throughout OpenMRS not to make a record to store a null value (attributes, >>>>> obs, IDs), so if some required data is missing from an otherwise complete >>>>> set of data elements (the patient forgot his national ID card, the vaccine >>>>> manufacturer is missing from the mass campaign report, the patient forgot >>>>> his ID card from another facility), the empty string is needed as a >>>>> placeholder until that information becomes available. >>>>> >>>>> All of which is to agree with Burke that there are sometimes semantic >>>>> reasons to distinguish empty string from null. >>>>> >>>>> I would put this in the API in the setters, along with >>>>> trimming/padding.**** >>>>> >>>>> >>>>> -----Original Message----- >>>>> From: [email protected] [mailto:[email protected]] On Behalf Of Ben Wolfe >>>>> Sent: Monday, September 12, 2011 3:19 AM >>>>> To: [email protected] >>>>> Subject: Re: [OPENMRS-DEV] Empty string fields vs null fields >>>>> >>>>> If it was a runtime property switch that means that users/admins are >>>>> the ones making the decision. But it is not them but the developer >>>>> that should be deciding how to treat empty strings. >>>>> >>>>> I like the automatic solution, but I fear there might be places where >>>>> a column is not nullable but the user doesn't want a value there. >>>>> That would end up with us (or module developer) forcing a space into >>>>> the value to get around it, and that would definitely make things >>>>> worse. (I think person_attribute.value is like that now) >>>>> >>>>> So my vote would be to make it automagic by default but with the >>>>> annotation to remove that magic. >>>>> >>>>> And -1000 for switching to Oracle. Oh wait, Oracle already owns MySQL. >>>>> Crap. >>>>> >>>>> Ben >>>>> >>>>> On Sat, Sep 10, 2011 at 12:06 AM, Saptarshi Purkayastha >>>>> <[email protected]> wrote: >>>>> > +1 for it to be done through the API, without automagic, but probably >>>>> a >>>>> > runtime-property to allow selecting automatic "" or null conversion >>>>> > -0 for switching to Oracle >>>>> > -1 for the annotation in code >>>>> > --- >>>>> > Regards, >>>>> > Saptarshi PURKAYASTHA >>>>> > >>>>> > My Tech Blog: http://sunnytalkstech.blogspot.com >>>>> > You Live by CHOICE, Not by CHANCE >>>>> > >>>>> > >>>>> > On 10 September 2011 02:00, Burke Mamlin <[email protected]> >>>>> wrote: >>>>> >> >>>>> >> I believe this should be done at the API level, in order to benefit >>>>> other >>>>> >> applications that use the API. I don't think it should be done >>>>> >> automagically (e.g., with an OpenmrsObjectSaveHandler) for two >>>>> reasons: (1) >>>>> >> there may be cases were an empty string and null are semantically >>>>> different >>>>> >> and (2) aspect-oriented changes like this are not without a cost >>>>> (they can >>>>> >> add up to real performance hits). >>>>> >> How about an annotation on the property like >>>>> >> @BlankValue(persist=NULL|BLANK)? . or maybe we should just switch to >>>>> Oracle. >>>>> >> ;-) >>>>> >> -Burke >>>>> >> On Fri, Sep 9, 2011 at 3:52 PM, Darius Jazayeri < >>>>> [email protected]> >>>>> >> wrote: >>>>> >>> >>>>> >>> Hi All, >>>>> >>> The way that our webapp, our API, and the DB interact, we >>>>> frequently run >>>>> >>> into the case where we store empty strings in the database for >>>>> values, when >>>>> >>> they really should be null. >>>>> >>> For example if you save a Concept Source and leave the HL7 Code >>>>> field >>>>> >>> blank, you save a row with concept_source.hl7_code = '' rather than >>>>> null. >>>>> >>> (Because the webapp submits an empty input, and we use spring MVC >>>>> to bind >>>>> >>> that to a bean property.) >>>>> >>> Two tickets that I looked at today are related to this (and there >>>>> are >>>>> >>> certainly others): >>>>> >>> https://tickets.openmrs.org/browse/TRUNK-2516 >>>>> >>> https://tickets.openmrs.org/browse/TRUNK-2004 >>>>> >>> We should try to fix this problem in a general way. At first >>>>> thought I >>>>> >>> can't come up with a case where we really want to save the empty >>>>> string in a >>>>> >>> database field rather than null. So some possible solutions are: >>>>> >>> >>>>> >>> (at the web level) use a custom PropertyEditor for the String class >>>>> in >>>>> >>> all our Spring controllers, so that if you submit "" and bind that >>>>> to a >>>>> >>> String property, it sets it to null. >>>>> >>> (at the API level) in OpenmrsObjectSaveHandler iterate over all >>>>> String >>>>> >>> properties and if any of them are "" then set them to null. >>>>> >>> >>>>> >>> (The first would allow API consumers to explicitly set empty string >>>>> >>> properties on objects, while the second wouldn't.) >>>>> >>> Any thoughts on this? >>>>> > >>>>> >>>>> >>> ------------------------------ >>> Click here to >>> unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from >>> OpenMRS Developers' mailing list >>> >> >> ------------------------------ >> Click here to >> unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from >> OpenMRS Developers' mailing list >> > > ------------------------------ > Click here to > unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from > OpenMRS Developers' mailing list > _________________________________________ To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to [email protected] with "SIGNOFF openmrs-devel-l" in the body (not the subject) of your e-mail. [mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l]
