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 > _________________________________________ 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]
