Fix #238: handling version_table from option method

[indiVar0508]

Hi,

Raising PR in reference to issue #238, as per solution suggested by @aditya051293 .

complete commit message: Fix #238: handling hardcoded string with options.tablename attribute via version_manager

Along with above change i was getting an SAWarning showing

sqlalchemy_continuum/relationship_builder.py:51: SAWarning: implicitly coercing SELECT object to scalar subquery; please use the .scalar_subquery() method to produce a scalar subquery. return getattr(self.remote_cls, tx_column) == (

have used same solution used by @TomGoBravo in PR #269 ,

complete commit message: fix for relationship_builder.py:51 SAWarning: implicitly coercing SELECT object to scalar subquery

could you please review if changes are ok?

Thanks

[indiVar0508]

Hi @marksteward , Could review this PR ?

Travis does not seem to be running it says

1 workflow awaiting approval
First-time contributors need a maintainer to approve running workflows.

if you could run them i could fix the new issues coming in Build, it's running in my local.

[marksteward]

Thanks for this, unfortunately tests are currently failing and I'll need to find some time to fix that before I can merge this.

[AbdealiLoKo]

@marksteward This seems to be a blocker for me right now

Cause I have a case where I have 2 tables: mytable and mytable_version already present in my application.
And using sqlalchemy-continuum is causing the tablenames to clash

When I set the options['table_name'] - alembic and so on work. And I can even make a util function in my application for myself to use.

But VersioningClauseAdapter and VersionExpressionReflector don't work cause they use .utils.version_class()
Meaning sqla-continuum is failing when handling many-to-one assocs

This seems to be a significant bug - and I could not find a workaround for it
Is there any chance we can fix and push this to pypi ?

[indiVar0508]

Added test file for version_table method didn't find a testfile specific for this method, so created a new test file similar to version_class already available under tests/utils folder

[marksteward]

I tidied this up only to find the first commit breaks tests. Could you take a look at these errors?

[indiVar0508]

Hi,

Can you check now i updated this branch with latest merge from master to use updated latest testcases,
Made changes to VersioningClauseAdapter and VersionExpressionReflector as they had the model object available to access table_name attribute.

could you reivew this?

Pytest result with upstream master branch
345 failed, 309 passed, 14 skipped, 302 warnings, 90 errors in 451.09s
and pytest result for this branch with new changes + upstream changes
345 failed, 311 passed, 14 skipped, 302 warnings, 90 errors in 446.94s

[marksteward]

I appreciate this is a breaking bug for you, and I'd like to get the fix out. However, this also changes the interface of a publicly documented function so it should ideally go in the next major release. Are you still able to work around this issue in your own code?

I've cherry-picked the scalar_subquery bit to the main branch.

[marksteward]

Oops, just seen the scalar_subquery bit is duplicated by #300.

[indiVar0508]

Hi @marksteward ,

However, this also changes the interface of a publicly documented function so it should ideally go in the next major release.

I think we can avoid going this route, as #299 helps address this issue better it seems, if it gets merged then we can modify the version_table method to something like this and it should work

def version_table(table):
    """
    Return associated version table for given SQLAlchemy Table object.

    :param table: SQLAlchemy Table object
    """
    table_suffix = option(table, 'table_name') % table.name
    if table.schema:
        return table.metadata.tables[
            table.schema + '.' + table_suffix
        ]
    elif table.metadata.schema:
        return table.metadata.tables[
            table.metadata.schema + '.' + table_suffix
        ]
    else:
        return table.metadata.tables[
            table_suffix
        ]

Can you merge #299 so i'll pull and make changes and test locally for this one which could address and close #238

[AbdealiLoKo]

Yep - I agree.
The actual issue here is that there is no way to know the manager for association tables.
(Which is what #299 solves)
This code can even simplify other parts in TableBuilder and Relationship builder in my opinion

[indiVar0508]

I have updated the PR with solution used in #299 and it seems to work added more testcases to cover version_table scenerios you can review the changes once.

[AbdealiLoKo]

There are now 3 cases:

1. Input is a model
2. Input is a table which belongs to a model
3. Input is an assoc table (doesn't have a model)

In scenario 2 - we are not handling it correctly, because we should be using the model's options (if available)

This would be clearer to read and use I think:

if isinstance(model_or_table, sa.Table):
    table = model_or_table
    if table in self.association_tables:
        return self.options[name]
    else:
        try:
            model_or_table = GET_MODEL(table)
        except NoModelFound:
            raise TypeError('Table %r is not versioned.' % table)
else:
    model = model_or_table

if not hasattr(model, '__versioned__'):
    raise TypeError('Model %r is not versioned.' % model_or_table)
try:
    return model_or_table.__versioned__[name]
except KeyError:
    return self.options[name]

[indiVar0508]

Hi @AbdealiJK ,

Tried to address this added testcase as well added a model attr to table in table_builder for this

[ldthorne]

hey @indiVar0508, is there anything i can do to help this get merged and released? my team would love to be able to customize our table names but we're blocked by this

[ldthorne]

whoops, meant to tag @marksteward - let me know if there's anything i can do to help with either this or #280

[gnu-lorien]

I was also running into this problem locally and this branch has fixed it for me. I'd love to see this merged as well.