Bug 1320690 - GetConsumer(uuid) & regenerateIdentityCertificates(uuid) call consumerCurator.verifyAndLookupConsumer(uuid) redundantly
Summary: GetConsumer(uuid) & regenerateIdentityCertificates(uuid) call consumerCurator...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Candlepin
Classification: Community
Component: candlepin
Version: 2.0
Hardware: Unspecified
OS: Unspecified
medium
low
Target Milestone: ---
: ---
Assignee: William Poteat
QA Contact: Katello QA List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-03-23 18:06 UTC by Craig Stanford
Modified: 2016-07-22 14:39 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-07-22 14:39:55 UTC


Attachments (Terms of Use)

Description Craig Stanford 2016-03-23 18:06:19 UTC
These methods need to be refactored to prevent extra calls to the database to retrieve a Consumer.

Within the ConsumerResouce.java Class, getConsumer(uuid):
calls the method consumerCurator.verifyAndLookupConsumer(uuid) to retrieve a Consumer. 
The method then proceeds to check the consumer object for imminent certificate expiration in the next 90 days or less. If it discovers certs are to expire in 90 days or less the method then calls:
regenerateIdentityCertificates(uuid)   

the regenerateIdentityCertificates(uuid) method is not a utility method, but instead an endpoint definition that takes a uuid as an argument rather then taking a consumer object as the argument so of course the method calls: consumerCurator.verifyAndLookupConsumer(uuid)  
to get the consumer that the method the calling method (getConsumer) already has creating additional calls to the db for no gain whatsoever. 

In addition: consumerCurator.verifyAndLookupConsumer(uuid)  verifies nothing. Why its name implies it does.... 

I suggest that the described code should be refactored as follows:


public Consumer GetConsumer(..., String uuid){
   Consumer consumer lookupConsumer();  // rename the verifyAndLookupConsumer

   // remove this line, you can never get here because the
   // verifyAndLookupConsumer(uuid) checks for null and throws an exception
   // if it finds one. Besides the next block makes it moot
   if (consumer != null)

   // move the cert check code to the consumer class so that certs
   // are checked every time the Consumer class is constructed. This was the consumer   
   // class is always verified at construction. That way all rules for what is 
   // a valid consumer are stored and validated in the consumer Class 
   if (consumer.updateCertsRequired()) {
      this.updateCerts(consumer);
   }
 
    // enrich with subscription data
    consumer.setCanActivate(subAdapter.canActivateSubscription(consumer));
    // enrich with installed product data
    addDataToInstalledProducts(consumer);

   return verifiedConsumer;
}





public Consumer regenerateIdentityCertificates(..., String uuid) { 

  Consumer consumer lookupConsumer(); 
  
  if (consumer.updateCertsRequired()) {
      this.updateCerts(consumer);
  }

}


private updateCerts(Consumer consumer){

    // and so on and so forth
  

}

Comment 1 William Poteat 2016-04-12 14:53:22 UTC
master commit 1d1727455abefc0479c0a5ef26a730d87af6ee55

Comment 2 Barnaby Court 2016-07-22 14:39:55 UTC
Moving to closed per the updated candlepin process for bugs that have been merged or have been taken care of.


Note You need to log in before you can comment on or make changes to this bug.