Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1380 (closed defect: fixed)

Opened 11 years ago

Last modified 10 years ago

[PATCH] TG database.py oddity preventing multiple SA databases from being used

Reported by: ian Owned by: faide
Priority: normal Milestone: 1.0.3
Component: TurboGears Version: 1.0.1
Severity: normal Keywords: sqlalchemy
Cc:

Description

as of TG 1.0.2.2, database.py contains this bit of code (lines 48-54):

def create_session():
    "Creates a session with the appropriate engine"
    return sqlalchemy.create_session(bind_to=get_engine())

metadata = activemapper.metadata
session = activemapper.Objectstore(create_session)
activemapper.objectstore = session

Typically, only the metadata needs to be bound to an engine. Remember, metadata keeps track of tables and calls to the database, and session keeps track of class instances and which ones are dirty. When session needs to flush some objects (like when session.flush() is called), it normally just defers to the engine associated with metadata. Since turbogears.database.metadata is already connected to an engine, the custom version of create_session in turbogears.database is unnecessary.

Not only that, but it is preventing us from using DynamicMetaData's connect method, which would allow us to support multple engines. If we just used sqlalchemy's create_session instead of our custom version, we'd be able to call turbogears.metadata.connect("sqlite:///another.sqlite") anytime we wanted. That would let people write decorators that switched out the database on a per-connection basis, for instance to support multiple sites with the same app and let each site have its own database.

Attached is a patch of database.py from TG 1.0.2.2 that removes the unnecessary custom version of create_session, and uses SQLAlchemy's create_session instead.

Attachments

multiple_databases.diff Download (647 bytes) - added by ian 11 years ago.
Patches database.py to support multiple databases
multiple_databases2.diff Download (583 bytes) - added by ian 11 years ago.
Fix for ActiveMapper? bug pointed out by Paul Johnston
multiple_databases3.diff Download (741 bytes) - added by iancharnas 11 years ago.
multiple_databases4.diff Download (718 bytes) - added by iancharnas 11 years ago.

Change History

Changed 11 years ago by ian

Patches database.py to support multiple databases

comment:1 Changed 11 years ago by alberto

  • Status changed from new to closed
  • Resolution set to fixed

Applied in [3003]. Thanks!

Alberto

Changed 11 years ago by ian

Fix for ActiveMapper? bug pointed out by Paul Johnston

comment:2 Changed 11 years ago by ian

  • Status changed from closed to reopened
  • Resolution fixed deleted

On the turbogears-trunk list, Paul Johnston pointed out that my patch broke apps using ActiveMapper. The reason for this is that the SQLAlchemy code in database.py was written for ActiveMapper, which means it uses ActiveMapper's version of metadata, which was not connected to a database.

Because ActiveMapper was a proof-of-concept project that is now discontinued in favor of Elixir, I believe we should eventually move away from code that is ActiveMapper-specific. This is difficult to do however, without breaking apps that use ActiveMapper.

In the meantime, a fine solution is just to call bind_meta_data as soon as metadata is defined. I have uploaded another patch (this time from rev 3003) which does just this. I will ask Paul to test this before we apply this patch. I cannot test it, because controllers.py seems to be messed up right now (someone put in tabs instead of spaces!) and I can't run the version of TG in svn.

Changed 11 years ago by iancharnas

Changed 11 years ago by iancharnas

comment:3 Changed 11 years ago by iancharnas

I was finally able to reproduce the problems that Dennis and Paul Johnston were reporting on the turbogears-trunk google group, and this latest patch (multiple_databases4.diff) fixes them.

At some point in the future, we should consider removing the ActiveMapper code from database.py. There are many SQLAlchemy extensions, why support just one in particular? Also, and perhaps more to the point, ActiveMapper is now discontinued in favor of Elixir. So what can people do who want to use these extensions? Easy! They just have to take care of setting up the extension themselves. For ActiveMapper and Elixir this just involves some particular glue for session and metadata. We should probably wait for a major revision change for this however, because some breakage is sure to happen until people who are still using ActiveMapper find and fix their code.

Then again, my only motive for moving the ActiveMapper "glue" code out of database.py is to have cleaner code in TG. Maybe this is too much of a "purist's" motive? Comments requested.

-ian charnas

comment:4 Changed 11 years ago by paj

The patch multiple_databases4.diff works for me!

comment:5 Changed 11 years ago by iancharnas

Ok, this latest patch (multiple_databases4.diff) is working for Paul, and it fixes the problem I was able to reproduce on my machines, let's commit this!

-Ian Charnas

comment:6 Changed 11 years ago by faide

  • Status changed from reopened to new
  • Owner changed from anonymous to faide

comment:7 Changed 11 years ago by faide

  • Status changed from new to closed
  • Resolution set to fixed

comment:8 Changed 11 years ago by faide

Thanks to Ian Charnas, Paul and Dennis.

comment:9 Changed 11 years ago by faide

Applied in r3010 (1.0) and r3011 (trunk)

Note: See TracTickets for help on using tickets.