Fix the order of SQL statements when creating batches of new models.

 






 

 


  Review Request #11167
  —
  Created Sept. 10, 2020 and submitted 

 




















 

  

   Information

  





 
 

  chipx86
 









 
 

  Django Evolution
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  django-evolution
 









 
 

  
 






 








 





  


 
 

  
When creating models, EvolveAppTask was generating and storing the SQL

for an entire model creation, which included not just the CREATE TABLE

but any deferred constraints. Each batch of SQL statements would later

be executed in order. If any of these SQL statements contained

constraints that referenced a table introduced by a subsequent task, the

SQL would fail to execute, because that table wouldn't have existed yet.


What's supposed to happen is that the deferred statements are executed

after the tables are created. Hence the "deferred" part. The way the

logic worked, this wasn't possible. SchemaEditor knows how to do this

correctly, as does our legacy logic for Django 1.6, but they require

having the full list of new models (not just a subset) passed to them.


This change redoes this logic to gather all models up-front and to

generate the SQL in one go, ensuring that the ordering of statements is

correct. The only real behavioral change introduced from this is that we

have to emit all creating_models signals up-front, and then emit all

created_models after all models are created, rather than emitting

these in pairs. This means that callers can't expect to receive them in

that order.


It also means that the evolve command will be listing all

"Creating new database models for ..." up-front, rather than when each

model will be created. It's an annoyance, but hopefully a mostly

harmless one, and is necessary without a much larger reworking of how

this SQL is generated.

 








  


 
 

  
Unit tests pass for all supported databases.


Verified this fixed an issue with setting up a Review Board database.

 







 









 





  

 


 
 

  



 

  
   


    Summary
    ID

   

  
  

   


    

     

      Fix the order of SQL statements when creating batches of new models.
      
When creating models, `EvolveAppTask` was generating and storing the SQL
for an entire model creation, which included not just the `CREATE TABLE`
but any deferred constraints. Each batch of SQL statements would later
be executed in order. If any of these SQL statements contained
constraints that referenced a table introduced by a subsequent task, the
SQL would fail to execute, because that table wouldn't have existed yet.

What's supposed to happen is that the deferred statements are executed
after the tables are created. Hence the "deferred" part. The way the
logic worked, this wasn't possible. `SchemaEditor` knows how to do this
correctly, as does our legacy logic for Django 1.6, but they require
having the full list of new models (not just a subset) passed to them.

This change redoes this logic to gather all models up-front and to
generate the SQL in one go, ensuring that the ordering of statements is
correct. The only real behavioral change introduced from this is that we
have to emit all `creating_models` signals up-front, and then emit all
`created_models` after all models are created, rather than emitting
these in pairs. This means that callers can't expect to receive them in
that order.

It also means that the `evolve` command will be listing all
"Creating new database models for ..." up-front, rather than when each
model will be created. It's an annoyance, but hopefully a mostly
harmless one, and is necessary without a much larger reworking of how
this SQL is generated.

     


    		
    281c6ac14d4064c0485212b190f20553a1cf34b6

   



  
 




 













 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
E303 too many blank lines (2)

      		
      
       reviewbotreviewbot
      
      
       
      
     


    
   

  

 







 






  

 



















 




 




 














 





 

  

   




david




   

   





  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
This is largely rewritten and requires re-review.


    

  • Changed back to generating SQL per-app, but also across all apps. This is to cover the "create these immediate models right now for this app" use case (used when creating initial Django Evolution tables) vs. "create for multiple apps" (standard use case).
    • 

  • Added unit tests
    • 

  • Added more compatible index name generation for unit tests for Django 1.7 through 1.10.
    • 






 



  

   
Commits:

   
    





 

  
   


    

    Summary
    ID

   

  
  

   


    

    

     

      Fix the order of SQL statements when creating batches of new models.
      
When creating models, `EvolveAppTask` was generating and storing the SQL
for an entire model creation, which included not just the `CREATE TABLE`
but any deferred constraints. Each batch of SQL statements would later
be executed in order. If any of these SQL statements contained
constraints that referenced a table introduced by a subsequent task, the
SQL would fail to execute, because that table wouldn't have existed yet.

What's supposed to happen is that the deferred statements are executed
after the tables are created. Hence the "deferred" part. The way the
logic worked, this wasn't possible. `SchemaEditor` knows how to do this
correctly, as does our legacy logic for Django 1.6, but they require
having the full list of new models (not just a subset) passed to them.

This change redoes this logic to gather all models up-front and to
generate the SQL in one go, ensuring that the ordering of statements is
correct. The only real behavioral change introduced from this is that we
have to emit all `creating_models` signals up-front, and then emit all
`created_models` after all models are created, rather than emitting
these in pairs. This means that callers can't expect to receive them in
that order.

It also means that the `evolve` command will be listing all
"Creating new database models for ..." up-front, rather than when each
model will be created. It's an annoyance, but hopefully a mostly
harmless one, and is necessary without a much larger reworking of how
this SQL is generated.

     


    		
    0db50a53b7c1a2364c49a483b8cac811ea82f1f7

   




   


    

    

     

      Fix the order of SQL statements when creating batches of new models.
      
When creating models, `EvolveAppTask` was generating and storing the SQL
for an entire model creation, which included not just the `CREATE TABLE`
but any deferred constraints. Each batch of SQL statements would later
be executed in order. If any of these SQL statements contained
constraints that referenced a table introduced by a subsequent task, the
SQL would fail to execute, because that table wouldn't have existed yet.

What's supposed to happen is that the deferred statements are executed
after the tables are created. Hence the "deferred" part. The way the
logic worked, this wasn't possible. `SchemaEditor` knows how to do this
correctly, as does our legacy logic for Django 1.6, but they require
having the full list of new models (not just a subset) passed to them.

This change redoes this logic to gather all models up-front and to
generate the SQL in one go, ensuring that the ordering of statements is
correct. The only real behavioral change introduced from this is that we
have to emit all `creating_models` signals up-front, and then emit all
`created_models` after all models are created, rather than emitting
these in pairs. This means that callers can't expect to receive them in
that order.

It also means that the `evolve` command will be listing all
"Creating new database models for ..." up-front, rather than when each
model will be created. It's an annoyance, but hopefully a mostly
harmless one, and is necessary without a much larger reworking of how
this SQL is generated.

     


    		
    281c6ac14d4064c0485212b190f20553a1cf34b6

   



  
 







   		
  




  

   
Diff:

   
    
 Revision 2 (+742 -92)
Show changes
 
    django_evolution/evolve.py 
    django_evolution/signals.py 
    django_evolution/management/commands/evolve.py 
    django_evolution/tests/test_evolver.py 
    django_evolution/tests/utils.py 
    django_evolution/tests/db/mysql.py 
    django_evolution/tests/db/postgresql.py 
    django_evolution/tests/db/sqlite3.py 
 

   		
  



 









 

  
Checks run (1 failed, 1 succeeded)



  

 flake8
 failed.
 





  

 JSHint
 passed.
 





 







 
flake8

 


 















  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to master (5ebb605)





 


 










  

 









     

    

   

  

  

   
   Loading...