Re: Fellow Reports - May 2021

[Mariusz Felisiak]

Week ending May 30, 2021

*Triaged: *
   https://code.djangoproject.com/ticket/32774 - cache_page decorator 
doesn't work for cross-browser (needsinfo) 
   https://code.djangoproject.com/ticket/32778 - Avoided unnecessary 
recompilation of token regex in _sanitize_token(). (accepted) 
   https://code.djangoproject.com/ticket/27659 - Arbritrary file overrides 
in startproject/app template extraction (fixed) 
   https://code.djangoproject.com/ticket/32779 - Add tablespace support to 
UniqueConstraint class (accepted) 
   https://code.djangoproject.com/ticket/32780 - Make covering and 
deferrable UniqueConstraint on SQLite a noop. (accepted) 
   https://code.djangoproject.com/ticket/32786 - Make the subquery ordering 
clearing optimization more selectively (accepted) 
   https://code.djangoproject.com/ticket/32783 - Auoreloader crashes in a 
conda env. (accepted) 
   https://code.djangoproject.com/ticket/32787 - ContentTypes are created 
instead of renamed when using SeparateDatabaseAndState (wontfix) 
   https://code.djangoproject.com/ticket/32788 - Transaction APIs do not 
consult the DB router to choose DB connection (needsinfo) 
   https://code.djangoproject.com/ticket/32789 - adding support for self 
closing tags in syndication feeds (accepted) 
   https://code.djangoproject.com/ticket/32792 - admin.display decorator 
boolean flag not working with @property (duplicate) 
   https://code.djangoproject.com/ticket/32794 - Get rid of boolean traps 
in Engine (wontfix) 
   https://code.djangoproject.com/ticket/32793 - Problem with decimal field 
when upgrade version (needsinfo) 
   https://code.djangoproject.com/ticket/15819 - Admin searches should use 
distinct, if query involves joins (fixed) 
   https://code.djangoproject.com/ticket/32795 - Reject requests earlier if 
the CSRF token is missing or has the wrong format (accepted) 

*Reviewed/committed: *
   https://github.com/django/django/pull/14434 - Fixed #32777 -- Passed 
table reference as a string to DatabaseSchemaEditor._index_columns(). 
   https://github.com/django/django/pull/14381 - Refs #24121 -- Added 
__repr__() to Lookup. 
   https://github.com/django/django/pull/14442 - Fixed #32778 -- Avoided 
unnecessary recompilation of token regex in _sanitize_token(). 
   https://github.com/django/django/pull/1 - Fixed #32780 -- Made 
Add/RemoveConstraint operations a noop for covering/deferrable unique 
constraints on SQLite. 
   https://github.com/django/django/pull/14420 - Fixed #32543 -- Added 
search_help_text to ModelAdmin. 
   https://github.com/django/django/pull/14447 - Fixed #32772 -- Made 
database cache count size once per set. 
   https://github.com/django/django/pull/14311 - Fixed #32669 -- Fixed 
detection when started non-django modules which aren't packages with 
"python -m" in autoreloader. 
   https://github.com/django/django/pull/14451 - Fixed #32789 -- Made feeds 
emit elements with no content as self-closing tags. 
   https://github.com/django/django/pull/14211 - Fixed #32596 -- Added 
CsrfViewMiddleware._check_referer(). 
   https://github.com/django/django/pull/14458 - Refs #24121 -- Added 
__repr__() to PermWrapper. 
   https://github.com/django/django/pull/14459 - Refs #32779 -- Changed 
DatabaseSchemaEditor._unique_sql()/_create_unique_sql() to take fields as 
second parameter. 
   https://github.com/django/django/pull/14324 - Fixed #32676 -- Prevented 
migrations from rendering related field attributes when not passed during 
initialization. 
   https://github.com/django/django/pull/14461 - Refs #32778 -- Renamed the 
regex object detecting invalid CSRF token characters. 

*Authored: *
   https://github.com/django/django/pull/14438 - Refs #24121 -- Improved 
Value.__repr__(). 
   https://github.com/django/django/pull/14449 - Fixed #32783 -- Fixed 
crash of autoreloader when __main__ module doesn't have __spec__ attribute.

Best,
Mariusz

>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/50285cd6-39c0-43fb-a95d-82bd762b867an%40googlegroups.com.

Re: Transaction APIs do not consult the DB router to choose DB connection

['Adam Johnson' via Django developers (Contributions to Django itself)]

I'm also -1 on changing anything in Django right now.

I think we should indeed take the "default position" for complex features:
write a third party library, get some traction. If many people find it
useful, we can look at adding something to core. It sounds like you're
already working on such a library Aditya, so part of the way there.

On Mon, 31 May 2021 at 10:43, charettes  wrote:

> Aditya,
>
> > "atomic does already call DB routers" -> Firstly after reading code, I
> don't think the transaction APIs consult the routers. Secondly, I think I
> already answered it in the initial discussion.
>
> Atomic doesn't consult it directly but the ORM does before interacting
> with transactions. While it's true that the transaction is not entirely
> bound to the writes if you're running Django in non-autocommit mode it's
> not the default usage and even discouraged. When you *do* run Django in
> auto-commit mode having db_for_read, db_for_write properly defined work
> just fine without a need for this proposed hook assuming your application
> code retrieves the proper database using db_for_write and use the return
> value as atomic(using). That's what Django does internally and what I was
> referring to in my message.
>
> > Also, if in the future, there is some special info that can be delivered
> to the transaction based on which a decision regd which DB to use could be
> made, then it would be cleanly isolated into its own method.
>
> I don't think this is a strong argument by any mean. To me this is
> basically saying "it might be useful in the future so lets add it now"
> without providing any details about why it might be useful in the first
> place.
>
> > How can we take this forward ?
>
> You'll need to gather positive support for your proposal, I don't know
> where Adam stands but I'm personally -1 on it. I'm still not convinced it's
> worth adding for your detailed usage for the following reasons:
>
> 1. There's multiple existing multi-tenancy applications out there that are
> relatively mature (I've even personally written one [1]) and none of them
> expressed a need for such a hook[2]. From my experience adding a lot of
> entries to DATABASES or using one schema per tenant only scales to a
> certain point when it starts breaking badly. Django assumes `DATABASES` is
> a static constant mapping, many unexpected and subtle things happen when
> it's not the case (connections leak, KeyErrors)
> 2. It's just not common enough to warrant an inclusion in Django core.
> Every single added feature comes with a maintenance burden cost and risks
> introducing bugs when interacting other existing features. This is
> particularly true with niche features interacting with complex systems such
> as ORM transaction handling.
>
> Best,
> Simon
>
> [1] https://github.com/charettes/django-tenancy
> [2] https://djangopackages.org/grids/g/multi-tenancy/
>
> Le lundi 31 mai 2021 à 03:13:16 UTC-4, gojeta...@gmail.com a écrit :
>
>> Hey Adam/Simon,
>>
>> How can we take this forward ?
>>
>> Regards,
>> Aditya N
>>
>> On Friday, May 28, 2021 at 3:04:14 PM UTC+5:30 N Aditya wrote:
>>
>>> Hey Adam,
>>>
>>> Also, after giving it a bit of thought, I figured that integrating this
>>> logic with the routers framework isn't entirely necessary.
>>> So I came up with another perspective to the solution as well which I've
>>> illustrated in message-3 of this thread.
>>>
>>> Both approaches work for me.
>>>
>>> Let me know your thoughts on the same.
>>>
>>> Regards,
>>> Aditya N
>>>
>>> On Friday, May 28, 2021 at 2:55:02 PM UTC+5:30 N Aditya wrote:
>>>
 Hey Adam,

 "atomic does already call DB routers" -> Firstly after reading code, I
 don't think the transaction APIs consult the routers. Secondly, I think I
 already answered it in the initial discussion.

 FYI from message-1:


1. Having a separate method for transaction is good because it need
not be mangled up with the other methods i.e db_for_read / db_for_write 
 as
they clearly indicate certain operational semantics which aren't tied 
 to a
transaction since it can have both reads and writes within it.
2. Also, if in the future, there is some special info that can be
delivered to the transaction based on which a decision regd which DB to 
 use
could be made, then it would be cleanly isolated into its own method.

 Regards,
 Aditya N

 On Friday, May 28, 2021 at 2:03:16 PM UTC+5:30 Adam Johnson wrote:

> Aditya - you didn't answer Simon's first question: "Can you think of
> places where this db_for_transaction hook would differ in any way
> from what db_for_write returns?" I think this is the crux of the
> issue. atomic does already call DB routers - in what case could you need 
> to
> return a different result for atomic() than for a write query?
>
> On Fri, 28 May 2021 at 09:17, N Aditya  wrote:
>
>> Hey Simon,

Re: Transaction APIs do not consult the DB router to choose DB connection

[charettes]

Aditya,

> "atomic does already call DB routers" -> Firstly after reading code, I 
don't think the transaction APIs consult the routers. Secondly, I think I 
already answered it in the initial discussion.

Atomic doesn't consult it directly but the ORM does before interacting with 
transactions. While it's true that the transaction is not entirely bound to 
the writes if you're running Django in non-autocommit mode it's not the 
default usage and even discouraged. When you *do* run Django in auto-commit 
mode having db_for_read, db_for_write properly defined work just fine 
without a need for this proposed hook assuming your application code 
retrieves the proper database using db_for_write and use the return value 
as atomic(using). That's what Django does internally and what I was 
referring to in my message.

> Also, if in the future, there is some special info that can be delivered 
to the transaction based on which a decision regd which DB to use could be 
made, then it would be cleanly isolated into its own method.

I don't think this is a strong argument by any mean. To me this is 
basically saying "it might be useful in the future so lets add it now" 
without providing any details about why it might be useful in the first 
place.

> How can we take this forward ?

You'll need to gather positive support for your proposal, I don't know 
where Adam stands but I'm personally -1 on it. I'm still not convinced it's 
worth adding for your detailed usage for the following reasons:

1. There's multiple existing multi-tenancy applications out there that are 
relatively mature (I've even personally written one [1]) and none of them 
expressed a need for such a hook[2]. From my experience adding a lot of 
entries to DATABASES or using one schema per tenant only scales to a 
certain point when it starts breaking badly. Django assumes `DATABASES` is 
a static constant mapping, many unexpected and subtle things happen when 
it's not the case (connections leak, KeyErrors)
2. It's just not common enough to warrant an inclusion in Django core. 
Every single added feature comes with a maintenance burden cost and risks 
introducing bugs when interacting other existing features. This is 
particularly true with niche features interacting with complex systems such 
as ORM transaction handling.

Best,
Simon

[1] https://github.com/charettes/django-tenancy
[2] https://djangopackages.org/grids/g/multi-tenancy/

Le lundi 31 mai 2021 à 03:13:16 UTC-4, gojeta...@gmail.com a écrit :

> Hey Adam/Simon, 
>
> How can we take this forward ? 
>
> Regards,
> Aditya N
>
> On Friday, May 28, 2021 at 3:04:14 PM UTC+5:30 N Aditya wrote:
>
>> Hey Adam, 
>>
>> Also, after giving it a bit of thought, I figured that integrating this 
>> logic with the routers framework isn't entirely necessary. 
>> So I came up with another perspective to the solution as well which I've 
>> illustrated in message-3 of this thread.
>>
>> Both approaches work for me. 
>>
>> Let me know your thoughts on the same.
>>
>> Regards, 
>> Aditya N
>>
>> On Friday, May 28, 2021 at 2:55:02 PM UTC+5:30 N Aditya wrote:
>>
>>> Hey Adam, 
>>>
>>> "atomic does already call DB routers" -> Firstly after reading code, I 
>>> don't think the transaction APIs consult the routers. Secondly, I think I 
>>> already answered it in the initial discussion.
>>>
>>> FYI from message-1:
>>>
>>>
>>>1. Having a separate method for transaction is good because it need 
>>>not be mangled up with the other methods i.e db_for_read / db_for_write 
>>> as 
>>>they clearly indicate certain operational semantics which aren't tied to 
>>> a 
>>>transaction since it can have both reads and writes within it. 
>>>2. Also, if in the future, there is some special info that can be 
>>>delivered to the transaction based on which a decision regd which DB to 
>>> use 
>>>could be made, then it would be cleanly isolated into its own method.
>>>
>>> Regards, 
>>> Aditya N
>>>
>>> On Friday, May 28, 2021 at 2:03:16 PM UTC+5:30 Adam Johnson wrote:
>>>
 Aditya - you didn't answer Simon's first question: "Can you think of 
 places where this db_for_transaction hook would differ in any way from 
 what db_for_write returns?" I think this is the crux of the issue. 
 atomic does already call DB routers - in what case could you need to 
 return 
 a different result for atomic() than for a write query?

 On Fri, 28 May 2021 at 09:17, N Aditya  wrote:

> Hey Simon, 
>
> It would be possible to write a method as you've suggested above. But 
> it entails the following problems:
> 1. There would have to be a wrapper for each of the transaction APIs, 
> not just transaction.atomic since all of them take the using kwarg.
> 2.  I'm trying to build a library that helps achieve multi-tenancy 
> (the isolated DB approach) by dynamically routing DB queries to the 
> appropriate databases at runtime. For more info, check this out -> 

Re: Transaction APIs do not consult the DB router to choose DB connection

[N Aditya]

Hey Adam/Simon, 

How can we take this forward ? 

Regards,
Aditya N

On Friday, May 28, 2021 at 3:04:14 PM UTC+5:30 N Aditya wrote:

> Hey Adam, 
>
> Also, after giving it a bit of thought, I figured that integrating this 
> logic with the routers framework isn't entirely necessary. 
> So I came up with another perspective to the solution as well which I've 
> illustrated in message-3 of this thread.
>
> Both approaches work for me. 
>
> Let me know your thoughts on the same.
>
> Regards, 
> Aditya N
>
> On Friday, May 28, 2021 at 2:55:02 PM UTC+5:30 N Aditya wrote:
>
>> Hey Adam, 
>>
>> "atomic does already call DB routers" -> Firstly after reading code, I 
>> don't think the transaction APIs consult the routers. Secondly, I think I 
>> already answered it in the initial discussion.
>>
>> FYI from message-1:
>>
>>
>>1. Having a separate method for transaction is good because it need 
>>not be mangled up with the other methods i.e db_for_read / db_for_write 
>> as 
>>they clearly indicate certain operational semantics which aren't tied to 
>> a 
>>transaction since it can have both reads and writes within it. 
>>2. Also, if in the future, there is some special info that can be 
>>delivered to the transaction based on which a decision regd which DB to 
>> use 
>>could be made, then it would be cleanly isolated into its own method.
>>
>> Regards, 
>> Aditya N
>>
>> On Friday, May 28, 2021 at 2:03:16 PM UTC+5:30 Adam Johnson wrote:
>>
>>> Aditya - you didn't answer Simon's first question: "Can you think of 
>>> places where this db_for_transaction hook would differ in any way from 
>>> what db_for_write returns?" I think this is the crux of the issue. 
>>> atomic does already call DB routers - in what case could you need to return 
>>> a different result for atomic() than for a write query?
>>>
>>> On Fri, 28 May 2021 at 09:17, N Aditya  wrote:
>>>
 Hey Simon, 

 It would be possible to write a method as you've suggested above. But 
 it entails the following problems:
 1. There would have to be a wrapper for each of the transaction APIs, 
 not just transaction.atomic since all of them take the using kwarg.
 2.  I'm trying to build a library that helps achieve multi-tenancy (the 
 isolated DB approach) by dynamically routing DB queries to the appropriate 
 databases at runtime. For more info, check this out -> PyCon India 
 2021 proposal 
 .
  
 If that be the case, for any new code base, I could expose a bunch of 
 methods as suggested above and ask them to incorporate it. But for 
 older/existing code bases which already directly use various transaction 
 APIs, it would be hard to replace all lines with another method. 

 By inspecting code, I figured that there is this method named 
 get_connection in the transaction.py module which is being used by all the 
 other transaction APIs for fetching the DB connection. Currently I've 
 patched this method with my own to achieve the functionality mentioned 
 above. Might I suggest an alternate implementation for this method as 
 follows:

 #settings.py

 TRANSACTION_DB_SELECTOR = "path_to_some_callable"

 #transaction.py
 ...
 transaction_db_selector = 
 import_string(settings.TRANSACTION_DB_SELECTOR)
 def get_connection():
 if transaction_db_selector:
 using = transaction_db_selector()
 if using is None:
 using = DEFAULT_DB_ALIAS
 return connections[using]

 Let me know your thoughts on this. 

 Regards, 
 Aditya N

 On Friday, May 28, 2021 at 10:05:26 AM UTC+5:30 charettes wrote:

> Ticket that directed to the mailing list for wider feedback 
> https://code.djangoproject.com/ticket/32788
>
> ---
>
> Can you think of places where this db_for_transaction hook would 
> differ in any way from what db_for_write returns? That's what Django 
> uses internally in such instances
>
>1. ​
>
> https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/base.py#L745-L760
>  
>
> 
>  
>2. ​
>
> https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/base.py#L950
>  
>
> 
>  
>3. ​
>
> https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/deletion.py#L400
>  
>
>