Add sync_backref param to relationship

[zzzeek]

We have two complaints that the change in #5149 prevents the creation of two bidirectional viewonly relationships using the backref() function. So we will propose new keyword argment viewonly_backref=backref() which explicitly sets up a relationship in the other direction as it does now, except that it doesn't set up the mutation events. viewonly_back_populates will also be added to suit the case where the bidrectionality is set up in separate relationships however we still want the lazy loader to be able to use the many-to-one loader optimization case as well as having the two relationships be introspectable as related.

[zzzeek]

see #5149 (comment) and #5229 (comment)

[zzzeek]

Here are the steps:

1. relationship(..., viewonly=True) can feature a viewonly backref:

   relationship(..., viewonly=True, viewonly_backref=backref(...))

2. this can also work with a non-viewonly relationship, it will set up the reverse property and just not set up the backref events:

   relationship(..., viewonly_backref=backref(..))

3. viewonly_back_populates will allow for either / both sides being viewonly:

   bar = relationship(Bar, viewonly=True, viewonly_back_populates='foo')
   foo = relationship(Foo, viewonly=True, viewonly_back_populates='bar')

while this is kind of verbose, unfortunately there's not another way to do this without breaking backwards compatibility right now as back_populates still sets up the population events even if viewonly=True.

4. _reverse_property etc. gets set up as it does now with the above. this actually has an impact on how the lazy loader works so this is actually sort of important which is part of why i think viewonly_back_populates needs to be there also (this for the case that @maparent requests).

5. viewonly props should not send the "backref" keyword into register_attribute_impl which is the part that sets up the write events, this is in orm/strategies.py, but ONLY in the above cases; it should NOT change backwards compat behavior with applications that are currently getting the warning, because we don't want to change any behavior yet. that is, an existing app that sets viewonly on one side and not the other, and generates the warning, should still be setting up the backref events.

All of the above must have tests, likely adding a new suite in test/orm/test_backref_mutations.py. There need to be tests against: o2m -> m2o, m2m -> m2m, o2o->m2o. there only needs to be one collection type tested. The test should likely use automated combinations which might be generated as:

for left, right in [
    ("o2m", "m2o"),
    ("m2m", "m2m"),
    ("o2o", "m2o")
]:
    for lw, rw in [
        ("writable", "viewonly"),
        ("viewonly", "writeable"),
        ("viewonly", "viewonly")
    ]:
        for back_populates_or_backref in [
            "viewonly_backref",
            "viewonly_back_populates"
        ]:
            print("test_%s_%s_%s_%s_%s" % (
                left, lw,
                right, rw,
                back_populates_or_backref
            ))

to produce:

test_o2m_writable_m2o_viewonly_viewonly_backref
test_o2m_writable_m2o_viewonly_viewonly_back_populates
test_o2m_viewonly_m2o_writeable_viewonly_backref
test_o2m_viewonly_m2o_writeable_viewonly_back_populates
test_o2m_viewonly_m2o_viewonly_viewonly_backref
test_o2m_viewonly_m2o_viewonly_viewonly_back_populates
test_m2m_writable_m2m_viewonly_viewonly_backref
test_m2m_writable_m2m_viewonly_viewonly_back_populates
test_m2m_viewonly_m2m_writeable_viewonly_backref
test_m2m_viewonly_m2m_writeable_viewonly_back_populates
test_m2m_viewonly_m2m_viewonly_viewonly_backref
test_m2m_viewonly_m2m_viewonly_viewonly_back_populates
test_o2o_writable_m2o_viewonly_viewonly_backref
test_o2o_writable_m2o_viewonly_viewonly_back_populates
test_o2o_viewonly_m2o_writeable_viewonly_backref
test_o2o_viewonly_m2o_writeable_viewonly_back_populates
test_o2o_viewonly_m2o_viewonly_viewonly_backref
test_o2o_viewonly_m2o_viewonly_viewonly_back_populates

[zzzeek]

cc @CaselIT @maparent @leorochael

[leorochael]

Hi @zzzeek, thanks for this.

I'm still confused by the need of viewonly_back_populates. I imagine it will require the respective relationship on the target model to already exist, just like back_populates. In that case:

• Will it accept a backref() result? If yes, won't it maybe conflict with the definition of the relationship in the target model?
• If not shouldn't viewonly= be defined in the respective relationship instead?
• And how could you even "back populate" something that is viewonly? Or maybe I misunderstand and viewonly only affects the explicit side of the relationship where it's mentioned, not the possibility of another relationship to modify it indirectly.

In general I'm unclear why viewonly_backref= can't simply be done as backref=backref(..., viewonly=True) and viewonly_backpopulates= be done by adding adding viewonly=True directly to the other side of the relationship.

Unless of course it's a matter of preserving (potentially incongruent) backward compatibility behaviors. I might simply not have completely grokked what you explained above.

Anyway, thanks again for allowing for the convenience of specifying both sides of a relationship in a single stroke even in viewonly situations. I'm happy no matter which way this goes!

[zzzeek]

Hi @zzzeek, thanks for this.

I'm still confused by the need of viewonly_back_populates. I imagine it will require the respective relationship on the target model to already exist, just like back_populates. In that case:

there are two things that back_populates does that don't relate to copying data between the two relationships. one is the case where one wants to look at the relationship.back_populates attribute in order to help with introspection of a model. the code example in #5149 (comment) refers to this use case. the other thing that can happen is if one is using a particular combination of eager and lazy loading, the lazy loader can in some cases make an optimization to not emit eager loads for a many-to-one relationship when it emits SQL as it knows the target objects will be locally present in the session.

* Will it accept a `backref()` result? If yes, won't it maybe conflict with the definition of the relationship in the target model?

there's two flags proposed here, one is "viewonly_backref" which accepts the "backref()" just the way "backref=backref()" does now. The other is "viewonly_back_populates" which accepts a string and works like "back_populates" does now.

* If not shouldn't `viewonly=` be defined in the respective relationship instead?

ideally however we are making an API here that will not conflict with the existing one that is still copying data into viewonly relationships. also it's more explicit for one to say "viewonly_back_populates" rather than "back_populates", as these two flags have different behavior; one of them sets up event listeners that copy data and the other one will not. the most critical thing is to avoid confusion and behaviors that silently switch on and off based on other observations.

* And how could you even "back populate" something that is `viewonly`? 

right, it doesn't, the "viewonly_back_populates" can be named differently if some other name is feasible...mostly though this is to suit folks who want to still use backref().

In general I'm unclear why viewonly_backref= can't simply be done as backref=backref(..., viewonly=True) and viewonly_backpopulates= be done by adding adding viewonly=True directly to the other side of the relationship.

because it would be a silent behavioral change to the current behavior, no longer cross-populating the collections/attributes, and also, it is better that a differently-behaving option have a different name.

Unless of course it's a matter of preserving (potentially incongruent) backward compatibility behaviors. I might simply not have completely grokked what you explained above.

Anyway, thanks again for allowing for the convenience of specifying both sides of a relationship in a single stroke even in viewonly situations. I'm happy no matter which way this goes!

[maparent]

Thank you very much for this as well, this is excellent news!

[leorochael]

@zzzeek. Thank you for the replies!

Just wanted to add a reminder to change the wording of the currently existing viewonly warnings to point to the newly introduced keywords.

[zzzeek]

yes, not having those warnings lead to more issues here would be the goal :)