Darius --
Suppose we have provider attribute types Employee Number
(Datatype=string,minOccurs=1,MaxOccurs=1) and
Awards(Datatype=string,minOccurs=0,maxOccurs=infinity). I am saying that for
any given attribute type, the values to be stored comply with the rules of a
set. So we can't have provider attributes {(Employee Number,"12345"),
(Awards,"Doctor of the Year"),(Awards,"Doctor of the Year")}. We have to have
provider attributes {(Employee Number,"12345"), (Awards,"Doctor of the Year
2001"),(Awards,"Doctor of the Year 2002")}.
As to the rest, until I get a chance to glyph it up, my
response is, "who're you calling a dumbain object?"
From: [email protected] [mailto:[email protected]] On Behalf Of Darius Jazayeri
Sent: Monday, September 26, 2011 11:58 AM
To: [email protected]
Subject: Re: [OPENMRS-DEV] Attribute multiplicity
Validation happens in the API, so it applies equally to the API, the webapp,
and web services.
Validation happens whenever you save the parent object (e.g.
VisitService.saveVisit does validate(visit); dao.save(visit).) And the only way
to interact with these child objects (e.g. Visit Attributes) is through the
parent (e.g. we don't do provide a saveVisitAttribute method). So doing
validation at the time of saving the parent is "safe".
The design pattern we try to follow is that the domain objects are "dumb", and
the intelligence in in the API (which includes the validators). Also, the
addAttribute method can't do full validation because the domain object (e.g.
visit) may be in an invalid state between saves.
Burke, Roger, my idea is that "addAttribute" is a plain method that does
exactly what you'd expect. Whereas setAttribute is a convenience method
documented to "Sets an attribute, voiding any existing attributes on the holder
of the same attribute type. Will fail if the relevant attribute type requires
more than one value."
Roger, I don't understand the bit about double iteration, and Doctor of the
Year for different years.
-Darius
On Sun, Sep 25, 2011 at 7:50 PM, Burke Mamlin
<[email protected]<mailto:[email protected]>> wrote:
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]<mailto:[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]>
[mailto:[email protected]<mailto:[email protected]>] On Behalf Of Darius Jazayeri
Sent: Sunday, September 25, 2011 2:19 PM
To:
[email protected]<mailto:[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]<mailto:[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]>
[mailto:[email protected]<mailto:[email protected]>] On Behalf Of Darius Jazayeri
Sent: Friday, September 23, 2011 2:44 PM
To:
[email protected]<mailto:[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]<mailto:[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]>
[mailto:[email protected]<mailto:[email protected]>] On Behalf Of Darius Jazayeri
Sent: Thursday, September 22, 2011 4:36 PM
To:
[email protected]<mailto:[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]<mailto:djazayeri%[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]<mailto:[email protected]>> wrote:
On Thu, Sep 22, 2011 at 3:03 PM, Darius Jazayeri
<[email protected]<mailto:djazayeri%[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
________________________________
Click here to
unsubscribe<mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l>
from OpenMRS Developers' mailing list
________________________________
Click here to
unsubscribe<mailto:[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]
