It might be cleaner to handle repeats either (1) within the datatype or (2) through a separate interface (e.g., RepeatingCustomDataType) that would have add/remove methods for single values and use List<T> for get/set.
Trying to manage get/set vs. add/remove within a single interface, as Roger suggests, could make things more complicated than necessary. -Burke On Sun, Sep 25, 2011 at 6:03 PM, Friedman, Roger (CDC/CGH/DGHA) (CTR) < [email protected]> wrote: > I have forked this part of the thread Empty String Fields v. Null Fields. > **** > > ** ** > > You are assuming that all class updates come through form controllers. But > we have updates coming from REST posts and bulk load processes as well. So > I think the validation has to be at the API level (perhaps the form > controller as well).**** > > ** ** > > I don’t know that you’ve got the double iteration (over attribute values > within attribute types) down quite right. AFAIK, attribute values for any > one attribute for any one object constitute a set; we can’t have a “Doctor > of the Year” award, we have to have “Doctor of the Year 2001”, “Doctor of > the Year 2002”, etc. to populate the provider object’s awards attribute if > Burke is to win it every year. **** > > ** ** > > Surely we still have methods like visit_attribute.setAttribute(value) or > visit.attributes(x).setAttribute(value); we’d need to do the void if null > thing; we’d also have to do the void thing if the value is being set to the > value of another existing attribute (or we could throw an exception and let > the caller void the attribute value with an appropriate reason).**** > > ** ** > > I don’t see how visit.setAttribute(attrType, value) works. Is that method > included for compatibility’s sake and throws an exception for attribute > types whose multiplicity is not (0,1) (or whose attribute type count is not > 0 or 1)? Or are you saying that visit.addAttribute adds another attribute > of type attrType while visit.setAttribute voids all existing attributes of > type attrType for that visit and adds a new attribute with the given value? > Seems like a lot of extra voiding. What’s its inverse operation – > visit.getAttribute(attrType)? what does it return, maybe an iterator?*** > * > > ** ** > > Duplicate detection and voiding when null seem both to be doable when the > add/set call is made; the count will be available due to duplicate > detection, so we could check multiplicity then. I understand your wanting > to do validation at the object rather than the object-property level, but > the error detection could come a long way from error introduction. Maybe > the object needs to store a working copy of the attribute ids and values and > void reasons in addition to the attribute collection, let the attribute > changes/validations affect the working copy, then have the save object > method actually update the hibernate objects in the attribute collection > from the working copy.**** > > ** ** > > *From:* [email protected] [mailto:[email protected]] *On Behalf Of *Darius > Jazayeri > *Sent:* Sunday, September 25, 2011 2:19 PM > *To:* [email protected] > *Subject:* Re: [OPENMRS-DEV] Empty string fields vs null fields**** > > ** ** > > Multiplicity constraints on attributes are enforced by the validator of the > class that owns those attributes. I.e. if you have a VisitAttributeType with > minOccurs=1 and maxOccurs=3, that will be checked when you validate the > Visit it belongs to.**** > > ** ** > > I'd hope things would work like this:**** > > (1) visit.setAttribute(attrType, null) --> semantically equivalent to "void > any existing attributes of the given type"**** > > (2) visit.addAttribute(attrType, null) --> this should fail when you try to > save the visit, because a null-valued attribute is invalid. (The db has a > not-null constraint on the value column.) To get a pretty error in this > case, we'd probably need to handle this in the controller for the edit visit > page. But at least the AOP would prevent us from storing empty values for > attributes (unintentionally sidestepping the not-null constraint.)**** > > ** ** > > -Darius**** > > ** ** > > On Sun, Sep 25, 2011 at 8:09 AM, Friedman, Roger (CDC/CGH/DGHA) (CTR) < > [email protected]> wrote:**** > > I actually agree that something should be put in the field, but I have seen > so many variants (blank, ?, ??, ???, Unknown, Uknown, Unown, Not Known, Not > Available, Missing, Left Card At Home) that I have come to believe that the > easiest thing to say to users is “if you don’t know it, leave it blank”. > The form controller normally will know whether an unknown value is required > and if so what its representation should be; however, that doesn’t apply to > our 3 formentry modules. In the case of OpenMRS objects, we could use ID=-1 > to indicate unknown, perhaps providing a default entry in the underlying > table (to permit inner rather than left joins).**** > > **** > > But you still haven’t said where the multiplicity constraints on attributes > should be enforced and whether that’s compatible with using AOP to convert > empty strings to nulls and what the effect of updating an attribute value to > null should be. **** > > **** > > *From:* [email protected] [mailto:[email protected]] *On Behalf Of *Darius > Jazayeri > *Sent:* Friday, September 23, 2011 2:44 PM**** > > > *To:* [email protected] > *Subject:* Re: [OPENMRS-DEV] Empty string fields vs null fields**** > > **** > > I agree with Burke here, I think that allowing people to store "" in a > required field, and using that to represent "yes, but unknown" isn't right. > You should store an explicit and recognizable unknown value, e.g. "Unknown". > So for the guardian case, no guardian means do not store a row in the > person_attribute table for the non-existent guardian. And an > existent-but-unknown guardian would be stored as an attribute whose value is > a pointer to an "Unknown Person" person.**** > > **** > > Also, at a web UI level, the standard widgets don't distinguish between > these things (i.e. if you have an attribute type whose datatype is "person", > the out-of-the-box widget is just going to show a person chooser, not have > an extra "click for unknown")...**** > > **** > > -Darius**** > > > On Friday, September 23, 2011, Burke Mamlin wrote:**** > > Roger,**** > > **** > > As you suggest, treating all empty strings as null will enforce that > required fields cannot be blank, since we won't be able to distinguish > between answered with an empty string from not answered at all. That said, > when is it "okay" to respond to a required question with nothing? Wouldn't > your example be better handled by recording the "Unknown" (in the case that > the orphan has a legal guardian who is currently unknown) instead of > treating unknown the same as if the field were skipped? And/or record "N/A" > when the field doesn't apply (in OpenMRS, we would probably just omit > storing an observation altogether if it didn't apply).**** > > **** > > -Burke**** > > On Fri, Sep 23, 2011 at 8:59 AM, Friedman, Roger (CDC/CGH/DGHA) (CTR) < > [email protected]> wrote:**** > > Darius –**** > > I believe I gave several examples during the earlier round of this > discussion. Here’s one – an orphan has a legal guardian, but the person > bringing the orphan to the clinic doesn’t know who, so the field is left > blank; a non-orphan does not have a legal guardian. So when it comes time > to report, you do something patient left join person_attribute where > attribute_type=”Legal Guardian” and format the nulls as “N/A” and print the > empty strings.**** > > I don’t think the automagic works with the new attribute paradigm. > Take a required attribute like National ID. The patient does have her card > with her when she comes to the clinic. Now we can’t save the patient info. > So I withdraw my tentative conclusion about properties. Is the > implementation of the minimum constraint still undecided? Let’s make sure > that’s well-understood before moving on to automagic.**** > > **** > > *From:* [email protected] [mailto:[email protected]] *On Behalf Of *Darius > Jazayeri > *Sent:* Thursday, September 22, 2011 4:36 PM**** > > > *To:* [email protected] > *Subject:* Re: [OPENMRS-DEV] Empty string fields vs null fields**** > > **** > > 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 > _________________________________________ 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]
