Initial patch for the persisting logic token

Design of persisting logic token:

Database

• Two table to store token and token tags attached to one entity LogicToken
• One token + one rule will be mapped into one LogicToken

RuleFactory

• Removed all rule caching in the rule factory
• The first pass will only use hibernate caching
• Caching are performed on LogicToken, not the Rule. Rule will be created using LogicToken when RuleFactory#getRule(String) are called

Circular Dependency

• LogicTokenDAO can't be instantiated using "new" and there's no ApplicationContext reference to get the LogicTokenDAO bean. For this reason LogicTokenDAO can't be hooked to RuleFactory
• LogicTokenDAO are hooked up to LogicServiceImpl to let Spring instantiate LogicTokenDAO. RuleFactory will call some method from the LogicService. But the LogicService hava RuleFactory as the property. So, there's a potential for circular dependency here
• init method in the RuleFactory must be called explicitly to load the default rules (AgeRule, HIVPositiveRule, etc)

Any comments or suggestions?

• RuleFactory.getTokens() should not be calling getLogicService().getAllLogicTokenTags() (it should return tokens, not tags)
• LogicToken methods should not go into the LogicService, since only RuleFactory should know/care about LogicToken objects. Can Spring inject dependency without a public method? i.e., could Spring inject a RuleFactory into LogicServiceImpl without exposing RuleFactory through the API?
• RuleFactory.init() will go away soon, so that may solve the potential circular dependency problem

persist token logic patch

My answer:

• Done.
• Done. Didn't think about making the RuleFactory to a "Spring injected bean".
• Removed

Do you think we need to expose some method to interact with the LogicToken in the service layer? ie. saveLogicToken(), removeLogicToken(), findLogicToken()?

I think we don't need that since we don't need to expose how internally the Rule are being saved and retrieved. User only need to know how to add, update and remove a rule (addRule, removeRule and updateRule).

If we don't expose a save and update method for the logic token, the effect is:

• we need to set the audit fields manually since the AOP for the audit fields only work for save* method in the service layer.

Comments?

Good work Win, this is a lot better than what we had. :-)

I don't really like the name "token"/LogicToken for this. Maybe just associated it with rules: Perhaps LogicRuleToken? Seem unintuitive to me what it is if its just called a token. SerializedRule? RuleKey? I don't have a good replacement suggestion, but I'm hoping someone else does.

Can we move RuleFactory to LogicServiceImpl package and make it private? Can Spring work with that?

Minor TODO items:

1. make the liquibase changeset ids longer so as to minimize the chance for overlap (I prefer to just include the hour/minute/second along with the date for the id)
2. Add preconditions to your first two liquibase updates.
3. Add foreign keys to the logic_token table for creator, changedBy. I think you can do that with a subelement of the createTable element.
4. LogicToken.hbm.xml didn't seem to make it into the patch. Am I missing it somewhere?
5. Be sure to run control-shift-f on your files after editing to keep our formatting all standardized
6. Mods for LogicToken class comment:

   This class will hold a single entry for 
   any registered token from the logic service. 
   Each token will be associated will multiple 
   tags.
   
   Token is not a <code>Rule<code>. Token is 
   the ingredient to instantiate
   a rule. You can think of a LogicToken is 
   a serialized version of a <code>Rule</code>
   

7. Make the comments on LogicToken into javadoc comments (via /** .... */) so it shows up in javadoc
8. RuleFactory.getTokens is still returning dao.getAllLogicTokenTags()
9. RuleFactory.getTokens should be getAllTokens according to API standards
10. Whats the difference between RuleFactory.findTokens(String) and RuleFactory.findLogicTokens(String) ?
11. Why did RuleFactory.addRule(String token, String[] tags, Rule rule) get deleted? Why is it going away? At the very least it needs to just be deprecated, never delete public api methods (unless we're releasing 2.0)
12. Always iterating over all tokens is not the fastest thing to do. Create dao methods for findTags, getTagsByToken, getTagsByToken, getTokensByTag
13. Change LogicServiceImpl.saveOrUpdateRule to just saveRule to match API standards
14. Add javadoc to LogicServiceImpl.saveOrUpdateRule (and make it private?)
15. When is updateRules used?
16. Change HibernateLogicTokenDAO.get* methods to return lists instead of sets. That way you can just return createCriteria...list(); instead of having to turn a set into a list (which wastes memory)
17. Use createCriteria in HibernateLogicTokenDAO.searchLogicTokenWithTag to make that faster and more efficient than looping over all tags
18. Add explicit "public" modifiers to the LogicTokenDAO methods so it matches the rest of the API (yeah, picky, I know, sorry!)
19. LogicBaseContextSensitiveTest.resetLogicServiceTokens should not be needed now if you can persist tokens. Make a base logic dataset that has the token/tags/rules set up for the data that is in the standardTestDataset.xml file. (might be easiest to use the updateRules() method and then dump the xml, see CreateInitialDataSet.java for an example of dumping dbunit xml files)
20. Now that you've done the previous point, delete that hacky LogicService.updateRules()! :-)
21. Add at least one unit test method for each method you've added on the service (get the eclipse plugin working and use that for @shoulds!)

Regarding comment's by bwolfe:

I don't really like the name "token"/LogicToken for this. Maybe just associated
it with rules: Perhaps LogicRuleToken? Seem unintuitive to me what it is if its
just called a token. SerializedRule? RuleKey? I don't have a good replacement
suggestion, but I'm hoping someone else does.

In the logic service, tokens serve only one purpose: a unique reference to a rule. But "token" is a pretty generic term, so LogicToken or LogicRuleToken is fine with me. Or a more descriptive name might be RegisteredToken.

Can we move RuleFactory to LogicServiceImpl package and make it private?
Can Spring work with that?

I like that idea. RuleFactory should be hidden as much as possible; consumers of the API need not know it exists.

6. Mods for LogicToken class comment:

This class will hold a single entry for any registered token from the logic
service. Each token will be associated will multiple tags. Token is not a
<code>Rule<code>. Token is the ingredient to instantiate a rule. You can
think of a LogicToken is a serialized version of a <code>Rule</code>

This class represents a registered token within the logic service.  It links 
a token (a simple string) to an instance of a <code>Rule</code>.  Any 
registered token may also be associated with 1-to-n tags (descriptive 
strings used for categorizing or labeling tokens).

10. Whats the difference between RuleFactory.findTokens(String)
and RuleFactory.findLogicTokens(String) ?

LogicTokens (or RegisteredTokens...whatever they end up being named) are used by the RuleFactory, so should only be referenced in the DAO methods used by RuleFactory. RuleFactory should not need to expose these objects to the logic service, right?

11. Why did RuleFactory.addRule(String token, String[] tags, Rule rule)
get deleted? Why is it going away? At the very least it needs to just be
deprecated, never delete public api methods (unless we're releasing 2.0)

This method should not be deprecated and should be the way rules are registered -- i.e., the logic service would use this method to register tokens with the RuleFactory.

logic rule token patch

In the patch:

• No new method in the LogicService
• Major changes in the RuleFactory class:
  • The class can't be instantiated using "new" keyword.
  • All method in the RuleFactory are now re-implemented to save and retrieve Rule trough LogicRuleTokenDAO.
  • getTokens is deprecated and will be replaced by getAllTokens
• New class HibernateLogicRuleTokenDAO:
  • Implemented using HQL because there's a limitation for the Hibernate's Criteria in which they can't perform operation on collection of values (for the tags).

1. You're missing a few class comments. :-)
2. RuleFactory.getAllTokens() should return a list (and then the implementation can be easier)
3. "find" should only be used in method names if a fuzzy search is happening. If only one or null is going to be returned, the method should be named "get"
4. logicToken.setToken(token); isn't needed on line 175 in saveRule, right?
5. more get vs find cleanup: RuleFactory.findTags calls dao.getTags ? getTagsByToken calls dao.findTagsByToken?
6. Some of your classes need a control-shift-o to clean up now-unused imports. Do any of them need a control-shift-f too?

Love the simplified unit testing! Can you just get rid of that LogicBaseContextSensitiveTest and do that execute in each class that was extending it?

Thats about it! Mostly just my nitpicky cleanup issues. :-) Strong work Win!

persisting logic token rule

Modified the code and attaching v4 of the patch. Hope I did all nitpicky things from Ben :)

Major changes:

• RuleFactory are now private
• Deprecate some method returning Set

persisting logic token rule

AOP-ed the LogicRuleToken saving process. The AOP method is called on the DAO layer instead of the service layer for the following reason:

• the LogicService doesn't have visible "save" method. LogicService have "add" method that will persist (save) the rule to the database
• adding the "add" signature to the list of services method that will AOP-ed is not an (easy) option because the method signature is addRule(String, Rule) instead of addRule(LogicRuleToken)
• LogicRuleToken implements Auditable. So, we must use available AOP classes as much as possible to fill out some of the properties in the LogicRuleToken

Very cool. A quick review came up with this minor cleanup/fixes:

1. Your logic_rule_token table doesn't have any foreign keys to the user table. Can you add those in in the createTable element, or do you have to make a separate changeset?
2. Can you put a comment onto the HibernateLogicRuleTokenDAO.getTags(String partialTag) methods saying why you are doing the search in java? That way when someone is looking at it in the future they know your reasoning.
3. You need an @Deprecated annotation and an "@deprecated use #getAllTokens()" on the HashSet methods that were replaced with List methods. (and both deprecation marks on each method in the interface too)
4. Is the hack in ReferenceRule line 66 still needed? Remind me why thats there...

After you fix those, make sure all the unit tests run (I think the logic ones still have just printing of output instead of asserting), and then you can commit it to trunk.

Ben,

Line 66 to add the key to obs data source. User can only create ReferenceRule on that keys. For encounter the key is datetime. For person the key is dead, birthdate etc.

If we don't load the key to the obs datasource, then we won't be able to create any ReferenceRule for obs because of the following line (in the ReferenceRule.java):

if (key == null || !dataSource.hasKey(key))

Committed on [10504]. Followed by [10505] to revert log4j config :)

Replying to nribeka:

Ben,

Line 66 to add the key to obs data source. User can only create ReferenceRule on that keys. For encounter the key is datetime. For person the key is dead, birthdate etc.

If we don't load the key to the obs datasource, then we won't be able to create any ReferenceRule for obs because of the following line (in the ReferenceRule.java)

It's still a hack. Data sources should manage their own keys -- i.e., this hack could move to ObsDataSource before going away completely.

The other option that I can think of is explicit call to ObsDataSource.addKey(String) when we need to get certain type of obs. Something like this call is needed then.

String name = c.getBestName(Context.getLocale()).getName();
obsDataSource.addKey(name);

Replying to nribeka:

The other option that I can think of...

The hack is having an addKey() method for a data source. Data sources are supposed to manage their own keys. We can talk about it. This discussion probably shouldn't take place on a closed ticket. ;-)

Agree Burke. We'll talk about it tomorrow then :)

Reopen to commit auto increment in the liquibase, reverting reference rule and fix on the foreign key declaration

Follow up fix committed on [10534]