https://tickets.openmrs.org/browse/TRUNK-2680
On Thu, Sep 22, 2011 at 1:24 PM, Darius Jazayeri <[email protected]>wrote: > 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]
