Review Board

beta

Add a generic WebAPI app to Djblets

Updated 10 months, 4 weeks ago

Christian Hammond Reviewers
trunk reviewboard
None Navi 
Moved much of our Review Board JSON code to Djblets and changed it to be more generic. While it only outputs JSON right now, it will be updated in an upcoming change to output XML files, if specified. We'll be able to include our webapi urls.py twice, once with a JSON path and once with an XML path, and we should be able to access the data both ways.

To make this more generic, we no longer hard-code an object encoder. Now we use the ones specified in the project's settings.py file, using the WEB_API_ENCODERS variable.

There's a base WebAPIEncoder class that can be overridden to provide custom encoders. We provide a BasicAPIEncoder, but projects can include their own and add it to the WEB_API_ENCODERS list. The serializer code will iterate through all registered encoders, trying to find a suitable encoder for the particular object.
Updated Review Board to use these new classes. It appears to be working so far.
David Trowbridge
Posted 11 months ago (February 9th, 2008, 5:27 p.m.)

   

  

   
    
    
   




   



    

     
     

      

       
       Loading diff fragment...
      

     

    

    

     
1 blank line between standard libraries and 3rd-party libraries.

     

  

   
    
    

   
  1. 
    
    
     
    
      
      Christian Hammond
      11 months ago (February 9th, 2008, 5:43 p.m.)
      
     
    
     
    Done.
    
    
    
   
    2. 


   



    

    

    

     
     

      

       
       Loading diff fragment...
      

     

    

    

     
This should probably be named something like BasicJSONEncoder

     

  

   
    
    

   
  1. 
    
    
     
    
      
      Christian Hammond
      11 months ago (February 9th, 2008, 5:36 p.m.)
      
     
    
     
    While it's deriving from a JSONEncoder, we're going to use it for the XML stuff as well. There's nothing JSON-specific in it. I could call it BasicAPIEncoder.
    
    
    
   
    2. 



   
  2. 
    
    
     
    
      
      David Trowbridge
      11 months ago (February 9th, 2008, 9:54 p.m.)
      
     
    
     
    I'd like an explanation of how it's not JSON-specific in a comment.
    
    
    
   
    3. 


   



    

    

    

     
     

      

       
       Loading diff fragment...
      

     

    

    

     
I don't like this going into settings.  We really ought to try to keep settings entirely focused on actual user deployments, and not let implementation details slip in.

How about you construct a WebAPIResponse with an encoder?

     

  

   
    
    

   
  1. 
    
    
     
    
      
      Christian Hammond
      11 months ago (February 9th, 2008, 5:38 p.m.)
      
     
    
     
    Then you'd have to do it with every single WebAPIResponse. That's *really* ugly and a pain to do. I'd rather define it in one place. Though I agree that settings may not be the best place, I can't think of another option that I'm okay with.

I see this as being no different from TEMPLATE_LOADERS, MIDDLEWARE_CLASSES, TEMPLATE_CONTEXT_PROCESSORS, INSTALLED_APPS, AUTH_PROFILE_MODULE, or TEST_RUNNER.
    
    
    
   
    2. 



   
  2. 
    
    
     
    
      
      David Trowbridge
      11 months ago (February 9th, 2008, 9:57 p.m.)
      
     
    
     
    I guess.  It makes me kind of sad, but oh well.
    
    
    
   
    3. 


   



    

    

    

     
     

      

       
       Loading diff fragment...
      

     

    

    

     
Either remove this or explain why it's commented out.

     

  

   
    
    
   



    

    

    

     
     

      

       
       Loading diff fragment...
      

     

    

    

     
404 isn't a useful error here, and won't help anyone trying to implement something other than JSON.

     

  

   
    
    

   
  1. 
    
    
     
    
      
      Christian Hammond
      11 months ago (February 9th, 2008, 5:42 p.m.)
      
     
    
     
    The idea is that you'll include a site's api's urls.py file and have api_format set to "json" or "xml" or something. It'll be part of the URL, so if it's an unknown type, the URL for it shouldn't exist. So you'd have, say, /api/json/* and /api/xml/*. Thus a /api/fdshkjshfa/* would be invalid, a 404.

This infrastructure isn't designed for people to implement whatever API format they want on top of this. I'll be putting in JSON and XML, but beyond that I don't consider it very important to let users define some custom format. If I'm convinced otherwise, I can try making it more scalable, but it's not a priority for me, as this is going to get us the 90%.
    
    
    
   
    2. 



   
  2. 
    
    
     
    
      
      David Trowbridge
      11 months ago (February 9th, 2008, 9:54 p.m.)
      
     
    
     
    OK.
    
    
    
   
    3. 


   



    

    

   


  
 




  

 


















 

  




 

 

  
Ship it!

  
David Trowbridge

  
Posted 10 months, 4 weeks ago (February 12th, 2008, 1:29 a.m.)

 

 

   
Looks good.

I wonder if we could maybe move some of the encoder logic into the models themselves.  Do something like admin/meta and have a "class WebAPI" that defined a method to encode itself.  This way the basic encoder could check that, and you'd keep model-related smarts closer to the model.

   

  

   
    
    

   
  1. 
    
    
     
    
      
      Christian Hammond
      10 months, 4 weeks ago (February 12th, 2008, 1:35 a.m.)
      
     
    
     
    Thanks for the quick review.

Yeah, we could do that. It's nice having a separation so that you can encode models from third party apps, but we could make BasicAPIEncoder check for the WebAPI class in the model and call encode on it. I'll get to that in a later change though.
    
    
    
   
    2.