Re: Review Request: Delete as a service call

12 Mar 2010

On Thu, Mar 11, 2010 at 03:27:54PM -0500, Adam Young wrote:
...
I've attached both normal and html diffs.  Reviewboard is not working  
right now.
I'd be fine with just the unified diff. The HTML one doesn't help
me at all especially when using mutt :)
[snip remove annoying html diff]
Comments inline, general theme I see is a LOT of formatting
changes that didn't need to happen. I'd really like to see the
diff without all of that noise, it hides what you really did.
...
index 7b60e02..07e222b 100644
--- a/proxy/src/main/java/org/fedoraproject/candlepin/resource/ConsumerResource.java
+++ b/proxy/src/main/java/org/fedoraproject/candlepin/resource/ConsumerResource.java
@@ -41,6 +41,7 @@ import org.fedoraproject.candlepin.model.Consumer;
 import org.fedoraproject.candlepin.model.ConsumerCurator;
 import org.fedoraproject.candlepin.model.ConsumerFacts;
 import org.fedoraproject.candlepin.model.ConsumerIdentityCertificate;
+import org.fedoraproject.candlepin.model.ConsumerIdentityCertificateCurator;
 import org.fedoraproject.candlepin.model.ConsumerType;
 import org.fedoraproject.candlepin.model.ConsumerTypeCurator;
 import org.fedoraproject.candlepin.model.Entitlement;
@@ -62,10 +63,10 @@ import com.google.inject.Inject;
  */
 @Path("/consumers")
 public class ConsumerResource {


//@Context
//private UriInfo uriInfo;




// @Context
// private UriInfo uriInfo;


Just remove these lines if they are already commented out.
...
 /**
  * 


* @param ownerCurator interact with owner


* @param consumerCurator interact with consumer


* @param consumerTypeCurator interact with consumerType




* @param ownerCurator


*            interact with owner


* @param consumerCurator


*            interact with consumer


* @param consumerTypeCurator


*            interact with consumerType
* @param productAdapter
* @param entitler
* @param subAdapter
* @param epCurator
* @param entitlementCurator
* @param identityCertService




* @param request servlet request




* @param request


*            servlet request



Don't reformat this comment, I know it's eclipse doing it, I'll
get that fixed, but for this comment just undo the formatting.
...
  */





@Inject
   public ConsumerResource(OwnerCurator ownerCurator,
       ConsumerCurator consumerCurator,
       ConsumerTypeCurator consumerTypeCurator,


   ProductServiceAdapter productAdapter,


   Entitler entitler,


   SubscriptionServiceAdapter subAdapter,


   PoolCurator epCurator,




   ProductServiceAdapter productAdapter, Entitler entitler,


   SubscriptionServiceAdapter subAdapter, PoolCurator epCurator,
   EntitlementCurator entitlementCurator,
   IdentityCertServiceAdapter identityCertService,
   @Context HttpServletRequest request) {



@@ -123,9 +126,10 @@ public class ConsumerResource {
             }
         }
     }




/**
    * List available Consumers
* 
* @return list of available consumers.
*/

@GET

@@ -133,10 +137,12 @@ public class ConsumerResource {
     public List<Consumer> list() {
         return consumerCurator.findAll();
     }




/**
    * Return the consumer identified by the given uuid.


* @param uuid uuid of the consumer sought.




* 


* @param uuid


*            uuid of the consumer sought.
* @return the consumer identified by the given uuid.
*/

@GET

@@ -144,51 +150,57 @@ public class ConsumerResource {
     @Path("{consumer_uuid}")
     public Consumer getConsumer(@PathParam("consumer_uuid") String uuid) {
         Consumer toReturn = consumerCurator.lookupByUuid(uuid);




   if (toReturn != null) {
       return toReturn;
   }





   throw new NotFoundException(


       "Consumer with UUID '" + uuid + "' could not be found"); 




   throw new NotFoundException("Consumer with UUID '" + uuid +


       "' could not be found");

}

Strictly a formatting change.
...




/**
    * Create a Consumer


* @param in Consumer metadata encapsulated in a ConsumerInfo.




* 


* @param in


*            Consumer metadata encapsulated in a ConsumerInfo.
* @return newly created Consumer




* @throws BadRequestException generic exception type for web services


*  We are calling this "registerConsumer" in the api discussions




* @throws BadRequestException


*             generic exception type for web services We are calling this


*             "registerConsumer" in the api discussions
*/

@POST
   @Consumes({ MediaType.APPLICATION_JSON })
   @Produces({ MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML })
   public Consumer create(Consumer in) throws BadRequestException {
       // API:registerConsumer


   Owner owner = ownerCurator.findAll().get(0); // TODO: actually get current owner




   Owner owner = ownerCurator.findAll().get(0); // TODO: actually get


                                                // current owner
   Consumer consumer = new Consumer();







   log.debug("Got consumerTypeLabel of: " + in.getType().getLabel());




   ConsumerType type = consumerTypeCurator.lookupByLabel(in.getType().getLabel());




   ConsumerType type = consumerTypeCurator.lookupByLabel(in.getType()


       .getLabel());
   log.debug("got metadata: ");
   log.debug(in.getFacts().getMetadata());
   for (String key : in.getFacts().getMetadata().keySet()) {
       log.debug("   " + key + " = " + in.getFact(key));
   }







   if (type == null) {




       throw new BadRequestException(


           "No such consumer type: " + in.getType().getLabel());




       throw new BadRequestException("No such consumer type: " +


           in.getType().getLabel());
   }

   try {
       consumer = consumerCurator.create(new Consumer(in, owner, type));





       ConsumerIdentityCertificate idCert =


           identityCertService.generateIdentityCert(consumer);




       ConsumerIdentityCertificate idCert = identityCertService


           .generateIdentityCert(consumer);
       log.debug("Generated identity cert: " + idCert);
       if (idCert == null) {




           throw new RuntimeException("Error generating identity certificate.");




           throw new RuntimeException(


               "Error generating identity certificate.");
       }
       consumer.setIdCert(idCert);




@@ -199,10 +211,12 @@ public class ConsumerResource {
             throw new BadRequestException(e.getMessage());
         }
     }




/**
    * delete the consumer.


* @param uuid uuid of the consumer to delete.




* 


* @param uuid


*            uuid of the consumer to delete.
*/

@DELETE
   @Produces({ MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML })

@@ -210,70 +224,88 @@ public class ConsumerResource {
     public void deleteConsumer(@PathParam("consumer_uuid") String uuid) {
         log.debug("deleteing  consumer_uuid" + uuid);
         try {

       consumerCurator.delete(consumerCurator.lookupByUuid(uuid));




       Consumer toDelete = consumerCurator.lookupByUuid(uuid);


       if (toDelete == null)


           return;


       identityCertService.deleteIdentityCert(toDelete);



       consumerCurator.delete(toDelete);



Should the delete from identity cert service and consumer curator
be done as two calls from the resource? or should they be put
in the curator itself? I guess it makes sense to have it here.
/me not 100% sure yet, but it is the right logic.
...
     }
     catch (RuntimeException e) {
         throw new NotFoundException(e.getMessage());
     }
 }





/**
    * Returns the ConsumerInfo for the given Consumer.
* 
* @return the ConsumerInfo for the given Consumer.
*/




@GET @Path("/info")


@GET
@Path("/info")
@Produces({ MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML })
// TODO: What consumer?
public ConsumerFacts getInfo() {
    ConsumerFacts ci = new ConsumerFacts();

-//        ci.setType(new ConsumerType("system"));

   // ci.setType(new ConsumerType("system"));
   ci.setConsumer(null);



-//        Map<String,String> m = new HashMap<String,String>();
-//        m.put("cpu", "i386");
-//        m.put("hey", "biteme");
-//        ci.setMetadata(m);

   // Map<String,String> m = new HashMap<String,String>();


   // m.put("cpu", "i386");


   // m.put("hey", "biteme");


   // ci.setMetadata(m);
   ci.setFact("cpu", "i386");
   ci.setFact("hey", "foobar");
   return ci;

}

The above was all formatting changes, it confuses the logic
changes you put in.
...




/**
    * removes the product whose id matches pid, from the consumer, cid.


* @param cid Consumer ID to affect


* @param pid Product ID to remove from Consumer.




* 


* @param cid


*            Consumer ID to affect


* @param pid


*            Product ID to remove from Consumer.
*/



-//    @DELETE @Path("{cid}/products/{pid}")
-//    public void delete(@PathParam("cid") String cid,
-//                       @PathParam("pid") String pid) {
-//        System.out.println("cid " + cid + " pid = " + pid);
-//        Consumer c = (Consumer) ObjectFactory.get().lookupByUUID(Consumer.class, cid);
-//        if (!c.getConsumedProducts().remove(pid)) {
-//            log.error("no product " + pid + " found.");
-//        }
-//    }

// @DELETE @Path("{cid}/products/{pid}")
// public void delete(@PathParam("cid") String cid,
// @PathParam("pid") String pid) {
// System.out.println("cid " + cid + " pid = " + pid);
// Consumer c = (Consumer) ObjectFactory.get().lookupByUUID(Consumer.class,
// cid);
// if (!c.getConsumedProducts().remove(pid)) {
// log.error("no product " + pid + " found.");
// }
// }

again more formatting changes that confuse the logic you
put in. You could've just formatted the method you touched
versus the whole file.
...
 /**
  * Returns the product whose id matches pid, from the consumer, cid.


* @param cid Consumer ID to affect


* @param pid Product ID to remove from Consumer.




* 


* @param cid


*            Consumer ID to affect


* @param pid


*            Product ID to remove from Consumer.
* @return the product whose id matches pid, from the consumer, cid.
*/




@GET @Path("{cid}/products/{pid}")


@GET
@Path("{cid}/products/{pid}")
@Produces({ MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML })
public Product getProduct(@PathParam("cid") String cid,


                  @PathParam("pid") String pid) {




   @PathParam("pid") String pid) {
   return null;

}
/**
    * Return the content of the file identified by the given filename.



* @param path filename path.




* 


* @param path


*            filename path.
* @return the content of the file identified by the given filename.




* @throws Exception if there's a problem loading the file.




* @throws Exception


*             if there's a problem loading the file.
*/

public byte[] getBytesFromFile(String path) throws Exception {
       InputStream is = this.getClass().getResource(path).openStream();


   byte [] bytes = null;




   byte[] bytes = null;
   try {
       bytes = IOUtils.toByteArray(is);
   }



@@ -286,19 +318,19 @@ public class ConsumerResource {
     /**
      * Return the client certificate for the given consumer.
      *

* @param consumerUuid UUID of the consumer




* @param consumerUuid


*            UUID of the consumer
* @return list of the client certificates for the given consumer.
*/

@GET
   @Path("{consumer_uuid}/certificates")
   @Produces({ MediaType.APPLICATION_JSON })
   public List<ClientCertificate> getClientCertificates(


   @PathParam("consumer_uuid") String consumerUuid, 




   @PathParam("consumer_uuid") String consumerUuid,
   @QueryParam("serials") String serials) {




   

   log.debug("Getting client certificates for consumer: " + consumerUuid);





   if (serials != null) {
       log.debug("Requested serials: " + serials);
       for (String s : serials.split(",")) {



@@ -307,8 +339,8 @@ public class ConsumerResource {
         }
     List<ClientCertificate> allCerts = new LinkedList<ClientCertificate>();



   //FIXME: make this look the cert from the cert service or whatever





   // FIXME: make this look the cert from the cert service or whatever
   // Using a static (and unusable) cert for now for demo purposes:
   try {
       byte[] bytes = getBytesFromFile("/testcert-cert.p12");



@@ -319,8 +351,9 @@ public class ConsumerResource {
             stream.close();
         // FIXME : these won't be a pkcs12 bundle



       // FIXME: This isn't quite right even for demo purposes, we're taking an





       // FIXME: This isn't quite right even for demo purposes, we're


       // taking an
       // entire PKCS12 bundle and cramming it into just the cert portion,
       // no key is set.
       ClientCertificate cert = new ClientCertificate();



@@ -335,21 +368,22 @@ public class ConsumerResource {
             cert2.setKey(baos.toByteArray());
             cert2.setCert(baos.toByteArray());
             allCerts.add(cert2);




       return allCerts;
   }
   catch (Exception ex) {
       throw new RuntimeException(ex);
   }

}





/**
    * Return the client certificate metadata for the given consumer.
    *


* This is a small subset of data clients can use to determine which certificates


* they need to update/fetch.




* This is a small subset of data clients can use to determine which


* certificates they need to update/fetch.
* 




* @param consumerUuid UUID of the consumer




* @param consumerUuid


*            UUID of the consumer
* @return list of the client certificate metadata for the given consumer.
*/

@GET

@@ -358,12 +392,12 @@ public class ConsumerResource {
     public List<ClientCertificateSerial> getClientCertificateSerials(
         @PathParam("consumer_uuid") String consumerUuid) {

   log.debug("Getting client certificate serials for consumer: " + consumerUuid);




   log.debug("Getting client certificate serials for consumer: " +


       consumerUuid);



   List<ClientCertificateSerial> allCerts = new LinkedList<ClientCertificateSerial>();





   List<ClientCertificateSerial> allCerts =


       new LinkedList<ClientCertificateSerial>();



   //FIXME: make this look the cert from the cert service or whatever




   // FIXME: make this look the cert from the cert service or whatever
   // Using a static (and unusable) cert for now for demo purposes:
   try {
       ClientCertificateSerial cert = new ClientCertificateSerial();



@@ -378,32 +412,35 @@ public class ConsumerResource {
             for (ClientCertificateSerial md : allCerts) {
                 log.debug("   " + md.getSerial());
             }




       return allCerts;
   }
   catch (Exception ex) {
       throw new RuntimeException(ex);
   }

}





/**
    * Entitles the given Consumer with the given Product.


* @param consumerUuid Consumer identifier to be entitled


* @param productId Product identifying label.




* 


* @param consumerUuid


*            Consumer identifier to be entitled


* @param productId


*            Product identifying label.
* @return Entitled object
*/

private Entitlement entitleByProduct(String consumerUuid, String productId) {





   Consumer consumer = consumerCurator.lookupByUuid(consumerUuid);
   if (consumer == null) {
       throw new BadRequestException("No such consumer: " + consumerUuid);
   }







   Product p = productAdapter.getProductById(productId);
   if (p == null) {
       throw new BadRequestException("No such product: " + productId);
   }







   // Attempt to create an entitlement:
   Entitlement e = entitler.entitle(consumer, p);
   // TODO: Probably need to get the validation result out somehow.



@@ -418,16 +455,19 @@ public class ConsumerResource {
     /**
      * Grants entitlements based on a registration token.
      *

* @param consumerUuid Consumer identifier.


* @param registrationToken registration token.




* @param consumerUuid


*            Consumer identifier.


* @param registrationToken


*            registration token.
* @return token
*/




private Entitlement entitleToken(String consumerUuid, String registrationToken) {

   //FIXME: this is just a stub, need SubscriptionService to look it up




private Entitlement entitleToken(String consumerUuid,
   String registrationToken) {



   // FIXME: this is just a stub, need SubscriptionService to look it up
   Consumer consumer = consumerCurator.lookupByUuid(consumerUuid);





   //FIXME: getSubscriptionForToken is a stub, always "works"





   // FIXME: getSubscriptionForToken is a stub, always "works"
   Subscription s = subAdapter.getSubscriptionForToken(registrationToken);
   if (s == null) {
       throw new BadRequestException("No such token: " + registrationToken);



@@ -437,7 +477,7 @@ public class ConsumerResource {
     Entitlement e = entitler.entitle(consumer, p);
     // return it





   if (consumer == null) {
       throw new BadRequestException("No such consumer: " + consumerUuid);
   }



@@ -447,9 +487,11 @@ public class ConsumerResource {
 /**
  * Request an entitlement from a specific pool.


*


* @param consumerUuid Consumer identifier to be entitled


* @param poolId Entitlement pool id.




* 


* @param consumerUuid


*            Consumer identifier to be entitled


* @param poolId


*            Entitlement pool id.
* @return boolean
*/

@POST

@@ -457,13 +499,12 @@ public class ConsumerResource {
     @Produces({ MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML })
     @Path("/{consumer_uuid}/entitlements")
     public Entitlement entitleByPool(

       @PathParam("consumer_uuid") String consumerUuid,


       @QueryParam("pool") Long poolId, 


       @QueryParam("token") String token, 


       @QueryParam("product") String productId) {





   @PathParam("consumer_uuid") String consumerUuid,


   @QueryParam("pool") Long poolId, @QueryParam("token") String token,


   @QueryParam("product") String productId) {


   // TODO: Check that only one query param was set:







   if (token != null) {
       return entitleToken(consumerUuid, token);
   }



@@ -491,13 +532,13 @@ public class ConsumerResource {
     return e;
 }





@GET
   @Produces({ MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML })
   @Path("/{consumer_uuid}/entitlements")
   public List<Entitlement> listEntitlements(
       @PathParam("consumer_uuid") String consumerUuid) {





   Consumer consumer = consumerCurator.lookupByUuid(consumerUuid);
   if (consumer == null) {
       throw new BadRequestException("No such consumer: " + consumerUuid);



@@ -508,28 +549,30 @@ public class ConsumerResource {
 /**
  * Unbind entitlements by serial number, or unbind all.


*


* @param consumerUuid Unique id for the Consumer.


* @param serials comma seperated list of subscription


* numbers.




* 


* @param consumerUuid


*            Unique id for the Consumer.


* @param serials


*            comma seperated list of subscription numbers.
*/

@DELETE
   @Path("/{consumer_uuid}/entitlements")
   public void unbindAllOrBySerialNumber(


       @PathParam("consumer_uuid") String consumerUuid,


       @QueryParam("serial") String serials) {




   @PathParam("consumer_uuid") String consumerUuid,


   @QueryParam("serial") String serials) {

   if (serials == null) {




       //FIXME: just a stub, needs CertifcateService (and/or a


       //CertificateCurator) to lookup by serialNumber




       // FIXME: just a stub, needs CertifcateService (and/or a


       // CertificateCurator) to lookup by serialNumber
       Consumer consumer = consumerCurator.lookupByUuid(consumerUuid);

       if (consumer == null) {




           throw new NotFoundException(


                   "Consumer with ID " + consumerUuid + " could not be found.");




           throw new NotFoundException("Consumer with ID " + consumerUuid +


               " could not be found.");
       }





       for (Entitlement entitlement : entitlementCurator.listByConsumer(consumer)) {




       for (Entitlement entitlement : entitlementCurator


           .listByConsumer(consumer)) {
           entitler.revokeEntitlement(entitlement);
       }




@@ -540,15 +583,17 @@ public class ConsumerResource {
             // entitlement pool)
         }
         else {

       throw new RuntimeException("Unbind by serial number not yet supported.");




       throw new RuntimeException(


           "Unbind by serial number not yet supported.");
   }


}
/**
    * Remove an entitlement by ID.



*


* @param dbid the entitlement to delete.




* 


* @param dbid


*            the entitlement to delete.
*/

@DELETE
   @Path("/{consumer_uuid}/entitlements/{dbid}")

@@ -562,9 +607,8 @@ public class ConsumerResource {
             entitlementCurator.delete(toDelete);
             return;
         }

   throw new NotFoundException(


       "Entitlement with ID '" + dbid + "' could not be found");




   throw new NotFoundException("Entitlement with ID '" + dbid +


       "' could not be found");

}




}

diff --git a/proxy/src/main/java/org/fedoraproject/candlepin/service/IdentityCertServiceAdapter.java b/proxy/src/main/java/org/fedoraproject/candlepin/service/IdentityCertServiceAdapter.java
index efe6d48..1126e82 100644
--- a/proxy/src/main/java/org/fedoraproject/candlepin/service/IdentityCertServiceAdapter.java
+++ b/proxy/src/main/java/org/fedoraproject/candlepin/service/IdentityCertServiceAdapter.java
@@ -40,4 +40,12 @@ public interface IdentityCertServiceAdapter {
      */
     ConsumerIdentityCertificate generateIdentityCert(Consumer consumer)
         throws GeneralSecurityException, IOException;


/**
* Deletes the certificate assocaited with the consumer


* @param consumer


*/


public void deleteIdentityCert(Consumer consumer) ;

+1
...
diff --git a/proxy/src/main/java/org/fedoraproject/candlepin/service/impl/DefaultIdentityCertServiceAdapter.java b/proxy/src/main/java/org/fedoraproject/candlepin/service/impl/DefaultIdentityCertServiceAdapter.java
index 7e0accb..87d9eda 100644
--- a/proxy/src/main/java/org/fedoraproject/candlepin/service/impl/DefaultIdentityCertServiceAdapter.java
+++ b/proxy/src/main/java/org/fedoraproject/candlepin/service/impl/DefaultIdentityCertServiceAdapter.java
@@ -53,6 +53,14 @@ public class DefaultIdentityCertServiceAdapter implements
         this.consumerIdentityCertificateCurator = consumerIdentityCertificateCurator;
     }

public void deleteIdentityCert(Consumer consumer) {
   ConsumerIdentityCertificate certificate = consumerIdentityCertificateCurator


       .find(consumer.getId());


   if (certificate != null) {


       consumerIdentityCertificateCurator.delete(certificate);


   }


}


Not really your fault, but do we have to call it
consumerIdentityCertificateCurator? is there any other type of
identity certificate? I mean we have a declaration already for
the variables, don't need to encode the type in the name too :)
...
 @Override
 public ConsumerIdentityCertificate generateIdentityCert(Consumer consumer)
     throws GeneralSecurityException, IOException {

diff --git a/proxy/src/test/java/org/fedoraproject/candlepin/service/impl/stub/StubIdentityCertServiceAdapter.java b/proxy/src/test/java/org/fedoraproject/candlepin/service/impl/stub/StubIdentityCertServiceAdapter.java
index 10ad4b9..6fb337f 100644
--- a/proxy/src/test/java/org/fedoraproject/candlepin/service/impl/stub/StubIdentityCertServiceAdapter.java
+++ b/proxy/src/test/java/org/fedoraproject/candlepin/service/impl/stub/StubIdentityCertServiceAdapter.java
@@ -32,5 +32,13 @@ public class StubIdentityCertServiceAdapter implements IdentityCertServiceAdapte
         return idCert;
     }

/* (non-Javadoc)
* @see org.fedoraproject.candlepin.service.IdentityCertServiceAdapter#deleteIdentityCert(org.fedoraproject.candlepin.model.Consumer)


*/



I'd remove the auto generated eclipse comment.
...

@Override
public void deleteIdentityCert(Consumer consumer) {
   //No Op.  Pretend to delete the cert


}


}
This makes sense.
-- 
jesus m. rodriguez          | jesusr@redhat.com
principal software engineer | irc: zeus
red hat systems management  | 919.754.4413 (w)
rhce # 805008586930012      | 919.623.0080 (c)
+---------------------------------------------+
|   "Those who cannot remember the past       |
|    are condemned to repeat it."             |
|                        -- George Santayana  |
+---------------------------------------------+


    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

Re: Review Request: Delete as a service call