Bumping this so people can be reminded of it before we discuss on today's design call.
-Darius On Thu, Jul 14, 2011 at 12:26 PM, Burke Mamlin <[email protected]>wrote: > On Thu, Jul 14, 2011 at 1:22 PM, Darius Jazayeri > <[email protected]>wrote: > >> Burke, >> >> Not exactly. Yes, custom datatype strings must uniquely and completely >> describe the datatype. But the java class that stores the value is *not* the >> same thing as the datatype. That was the biggest mistake we made in the >> initial implementation of PersonAttribute, and it's the reason we're >> introducing handlers. >> >> For example, both "free-text" and "regex-validated-text" have their >> value represented as a java.lang.String, but they are different >> datatypes/handlers, with different configuration options and different >> validations rules. >> > > I think describing the datatype (not the handler) is the right thing. As > Ben pointed out, using strings to define the datatype (instead of actual > classes) is preferred so we're not dependent on classes being > loaded/available. When defining the datatype, I was *not* assuming you > would use java.lang.String for anything serialized to a string; rather, it > would be the class you created to handle your datatype. For example: > > - org.openmrs.module.*moduleid*.PhoneNumber > - java.util.Date > - org.openmrs.Person > - java.awt.Image > - org.openmrs.module.*moduleid*.ImageWithMetadata > > I think we can safely draw a line to say that the datatype is *at least* > determined > by the literal payload – i.e., if one handler stores dates in the form > YYYY-MM-DD and another serializes a java.util.Date through a different > mechanism, then they should *not* use the same datatype (even though their > both handling dates, they are different types of dates). Another way to say > it: if you enter in the same value into different handlers that use the same > datatype, the bytes stored in the database should be equal. > > So, handlers for "free-text" and "regex-validated-text" would use the same > datatype if they store the same value in the same way – e.g., even if you > use a regex to restrict the regex-validated-text handler to only accept > "Paul is a dork" as a string, a value that's valid for both handlers – "Paul > is a dork" – would be persisted the same way in the database. > > I've tried to explain it... > > "A handler's datatype should either be the class name of the object it > returns as its payload. When the class of the payload is too generic and > does not define the meaning of the datatype, the handler should refer to a > class name that describes the payload. For example, if a handler for a > module XYZ returns an java.lang.String that refers to a phone number, it's > datatype should be org.openmrs.module.xyz.PhoneNumber or, if there's > already another handler that persists phone number exactly the same way, > then that hander's datatype may be used." > > But I don't think I'm saying it very clearly. > > Regarding your date example: are you asking how we would implement having 3 >> different UI widgets that allow you to choose a date, but they have exactly >> the same configuration options, and the same set of legal values? I.e. some >> dates are chosen from a calendar, and others from a slider? That isn't >> currently handled by my solution--we'd have to add an additional "preferred >> handler" property to attribute types, to allow manually specifying a >> handler, overriding their declared ordering. (Your proposed solution does >> handle this, because you store the handler itself. But the downside of that >> solution is that there's no way to know what other handler to fall back to >> if your chosen handler is unavailable.) >> > > Yes. The example was a date handler that doesn't a typical date field, > another comes along and has a slider option that you want to use for some > concepts but it doesn't offer as good of a typical date widget, and a third > has a cool option that would work really well for one particular person > attribute you are using. > > I see the value of specifying both datatype and handler separately. I > still don't like the Spring ordering annotations because it does not > explicitly define when one handler should replace another and I want to > allow either case: (1) new handler overrides old handler and (2) new handler > extends the old one but does not replace it – i.e. a new option built on the > old one, user can choose between them. > > >> [Tangent: how should we deal with the fact that overriding handlers may >> provide additional config options. E.g. the calendar popup date handler >> might have a config option for controlling how many months are displayed at >> a time, whereas the api-level date handler doesn't have that.] >> > > [Tangent response: the UI handler would take over the configuration, > storing a configuration string that includes the API's configuration > settings in a way that the UI handler could parse them out & pass them down > to the API handler as needed.] > > >> So, stepping back, I think the biggest issue with the current design >> (mine) is that we have this invented string datatype. When I first started >> refactoring attribute types, we didn't introduce this immediately. We added >> it specifically have this. We added it later (see JIRA >> comment<https://tickets.openmrs.org/browse/TRUNK-2243?focusedCommentId=167650&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-167650>) >> to avoid referring to classes that might eventually no longer be on the >> classpath. Three approached spring to mind: >> >> 1. Leave the invented String datatypes as they are. Every >> attribute-type table also needs an extra column to store its preferred >> handler. (We could possibly shoehorn this into the handler_config column.) >> 2. Introduce a proper java interface to represent these datatypes. So >> instead of DateHandler.getDatatype returning "date" it would return a >> DateDatatype class. >> - Pro: >> - datatypes can have class hierarchy, e.g. >> RegexValidatedStringDatatype can extend StringDatatype. >> - We can have the datatype explicitly define the configuration >> options available for handlers that handle it. >> - Con: >> - complex? >> - re-introduces the problem of losing the hierarchy if the >> module defining your datatype is unloaded >> 3. Get rid of datatype entirely, and go back to having the handler >> define the datatype. Replace AttributeType.getDatatype() with >> AttributeType.getHandler(). If you unload a module that provides a handler >> that's explicitly in use by an attribute type, then things break, and the >> sysadmin has to fix them >> >> Perhaps there's a variant on #2 that will let us have the best of all >> worlds... I'm going to just send this email now and percolate on it some >> more. >> > > I think we're getting close to the same idea. I'm trying to get to your #2 > without defining a datatype interface – i.e., create a convention for when > datatypes should be different and then allow any class name to be used as a > datatype where appropriate. > > -Burke > > >> >> -Darius >> >> On Thu, Jul 14, 2011 at 7:48 AM, Burke Mamlin >> <[email protected]>wrote: >> >>> Darius, >>> >>> The custom datatype string should uniquely describe the datatype – i.e., >>> if the literal payload stored by two handlers differ at all, then they >>> should have different datatype strings. Rather than a mess of invented >>> strings, I would propose that the datatype string be the class name for the >>> datatype. For example, "java.util.Date", "java.awt.Image", or >>> "org.openmrs.module.foobar.ImageWithMetadata". >>> >>> I would favor adding *getName(Locale)* & *getDescription(Locale)*methods to >>> the handler interface to allow for human-friendly choices & >>> descriptions in the application. >>> >>> With your approach, how would someone create & use two separate handlers >>> for the same datatype? For example, 2 or 3 different date handlers. Is the >>> only solution for handlers to make up different names for the same datatype? >>> >>> -Burke >>> >>> On Tue, Jul 12, 2011 at 2:08 PM, Darius Jazayeri < >>> [email protected]> wrote: >>> >>>> Burke, >>>> >>>> I don't see how your suggestion makes things easier, and actually I >>>> think it removes a necessary level of indirection. >>>> >>>> As I coded things up, each handler declares its "datatype", and its >>>> priority. Thus an attribute type declares what datatype it uses, and the >>>> framework is responsible for being able to list all datatypes, and fetch >>>> the >>>> top-priority handler for each datatype. E.g. the framework has to answer >>>> "what's the top priority handler for 'text'?"That seems like the correct >>>> flow of control to me. >>>> >>>> You are proposing that we get rid of that datatype and instead have >>>> attribute types declare what handler they use, then the handlers declare an >>>> arbitrarily-complex inheritance tree. So the framework is responsible for >>>> saying "the attribute type declared the handler ApiTextHandler, so now I >>>> need to walk the inheritance tree to find the most-overriding handler". >>>> This >>>> seems *more* prone to errors and confusion. E.g. imagine you have: >>>> B overrides A >>>> C overrides A >>>> D overrides C >>>> Now imagine an attribute type declare its handler is A. What handler >>>> should the framework choose in this case? >>>> >>>> My proposal basically looks like this: >>>> >>>> @Component >>>> @Ordered(Priority.LOW) >>>> class XyzApiHandler extends BaseAttributeHandler { >>>> public String getTypeHandled() { return "xyz" } >>>> // implementations of validate, serialize, and deserialize >>>> } >>>> >>>> @Component >>>> @Ordered(Priority.HIGH) >>>> class XyzFieldGenHandler extends XyzApiHandler implements >>>> FieldGenHandler { >>>> // inherits getTypeHandled, validate, serialize, and deserialize >>>> // implementations of FieldGenHandler methods >>>> } >>>> >>>> Then, when 2.x comes around we'll add this, and merely stop including >>>> the previous ones on the classpath: >>>> >>>> @Component >>>> @Ordered(Priority.HIGH) >>>> class XyzTwoDotXHandler extends XyzApiHandler implements TwoDotXHandler >>>> { >>>> // inherits getTypeHandled, validate, serialize, and deserialize >>>> // implementations of TwoDotXHandler methods, hopefully compatible >>>> with fieldGen >>>> } >>>> >>>> And if a module wants to provide a custom widget >>>> >>>> @Component >>>> @Ordered(Priority.HIGHER) >>>> class XyzCustomHandler extends XyzApiHandler { >>>> // inherits getTypeHandled, validate, serialize, and deserialize >>>> // implementations of displaying the UI >>>> } >>>> >>>> We don't actually need an external set of constants for the datatypes, >>>> rather the XyzApiHandler base class declares the getTypeHandled method, and >>>> none of its children should overwrite it. (Maybe we should mark it >>>> final...) >>>> >>>> -Darius >>>> >>>> On Tue, Jul 12, 2011 at 2:36 AM, Suranga Kasthurirathne < >>>> [email protected]> wrote: >>>> >>>>> >>>>> Howdy, >>>>> >>>>> Sure, lets discuss this on Wednesday's call (hope we can get >>>>> all interested parties at the meeting) >>>>> >>>>> And also, regarding the need for a utility class for handler constants, >>>>> so far I have not come across an explicit need to have one - the >>>>> constants I >>>>> created so far all fitted nicely into the handler classes themselves, so >>>>> maybe we need not worry about the need for a seperate utility class just >>>>> for >>>>> this. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> On Tue, Jul 12, 2011 at 1:22 PM, Ben Wolfe <[email protected]> wrote: >>>>> >>>>>> Sounds good to me. Although I'd suggest it be a string >>>>>> package+classname instead of the actual object. This way there isn't a >>>>>> compile time dependency on the module you're overriding. >>>>>> >>>>>> I'd want to see at least a check for the super-edge case of a loop of >>>>>> dependencies/overridings. >>>>>> >>>>>> Ben >>>>>> >>>>>> >>>>>> On Tue, Jul 12, 2011 at 7:25 AM, Burke Mamlin < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> I've been thinking about how to manage handler types. Currently, >>>>>>> we've got arbitrary strings naming handler types, some of these >>>>>>> harcoded as >>>>>>> constants in a utility class, and Spring annotations to manage the >>>>>>> precedence. While adding constants to a utility class might be trivial >>>>>>> code-wise, it's another dependency on hardcoded values in core code. >>>>>>> >>>>>>> What we're trying to manage is an arbitrary list of handler types >>>>>>> with the ability to optionally override certain types. Which type (or >>>>>>> types) if any should be overridden is known by the overriding handler. >>>>>>> >>>>>>> So instead, how about simply adding a *CustomDatatypeHandler[] >>>>>>> getOverriddenHandlers()* method to the CustomDatatypeHandler >>>>>>> interface? A handler would only need to return the list – if any – of >>>>>>> the >>>>>>> handlers it's replacing. For example, a UI handler could return the API >>>>>>> handler it's extending (and replacing); a newer/better handler could >>>>>>> return >>>>>>> the two old ones it replaces; and, a handler that wants to coexist with >>>>>>> others could return an empty array. I believe this would eliminate the >>>>>>> need >>>>>>> for type strings, constants, and the ordering annotations... and would >>>>>>> be >>>>>>> easier to understand & manage. >>>>>>> >>>>>>> Thoughts? Maybe we could take a few minutes to discuss on >>>>>>> Wednesday's design call? >>>>>>> >>>>>>> Cheers, >>>>>>> >>>>>>> -Burke >>>>>>> >>>>>>> On Wed, Jul 6, 2011 at 12:52 AM, Darius Jazayeri < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>> If the same value is to used in multiple places, it should be a >>>>>>>> constant. >>>>>>>> >>>>>>>> If all handlers for a given type extend an api layer superclass for >>>>>>>> that type, then the constant should live in the superclass. >>>>>>>> >>>>>>>> If non-inheriting classes are expected to be handlers for the same >>>>>>>> type, then they should go in an independent class. >>>>>>>> >>>>>>>> (Do not take the one class per handler comment literally. Having to >>>>>>>> add to a constants- or utility-class is trivial and doesn't affect it.) >>>>>>>> >>>>>>>> -Darius (via his phone) >>>>>>>> >>>>>>>> On Jul 5, 2011 9:22 PM, "Suranga Kasthurirathne" < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>> >>>>>>>> Thanks everyone, >>>>>>>> >>>>>>>> I agree that I should use a constant, that makes a lot of sense, >>>>>>>> >>>>>>>> But Darius, I was wondering about creating a separate constants >>>>>>>> class just for that, because initially we had placed a lot of emphasis >>>>>>>> on >>>>>>>> the fact that the Handler class was a standalone (as in, the dev >>>>>>>> needed to >>>>>>>> create only the handler class, and didn't have to amend anything else) >>>>>>>> >>>>>>>> So moving constants to a separate class means that the Handler class >>>>>>>> coder will also need to edit a second class to get his handler working. >>>>>>>> >>>>>>>> Given this scenario, would u advise going ahead with >>>>>>>> a separate constants class, or keeping constants in the handler itself >>>>>>>> ? >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Jul 6, 2011 at 1:33 AM, Darius Jazayeri < >>>>>>>> [email protected]> wrote: >>>>>>>> > >>>>>>>> > That's corre... >>>>>>>> >>>>>>>> -- >>>>>>>> Best Regards, >>>>>>>> >>>>>>>> Suranga >>>>>>>> >>>>>>>> >>>>> >>>>> -- >>>>> Best Regards, >>>>> >>>>> Suranga >>>>> >>>> > ------------------------------ > 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]
