Storm should support Firebird databases

Bug #130148 reported by Gustavo Niemeyer
2
Affects Status Importance Assigned to Milestone
Storm
Won't Fix
Undecided
Unassigned

Bug Description

Storm should offer support for Firebird databases.

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

I've just linked to this bug the branch that Môshe van der Sterre is working on.

Changed in storm:
assignee: nobody → moshevds
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Thank you very much for working on that backend Moshe. I provide
below some initial comments about the linked branch.

[1]

  try:
      import kinterbasdb
  except ImportError:
      kinterbasdb = dummy

  if kinterbasdb is not dummy:
      install_exceptions(kinterbasdb)
      # this fixes datetime and unicode behavior
      kinterbasdb.init(type_conv=200)

The "if" here may be replaced by an "else" of the preceeding "try"
block.

[2]

  # Considering the above, selects have a greater precedence.
  compile.set_precedence(5, Union, Except, Intersect)

I don't understand exactly why this is needed here. The copied
comment and code refers to a block of comment/code that wasn't
copied over from the sqlite backend.

[3]

  @compile.when(SetExpr)
  def compile_set_expr_firebird(compile, expr, state):

If this is doing precisely what the postgres version is doing,
we can just import compile_set_expr_postgres and use it in place
with something like:

  compile.when(SetExpr)(compile_set_expr_postgres)

We aim at 100% test coverage, so if any changes in postgres would
break the firebird backend, tests should break as well.

[4]

  # Firebird needs parameters to be cast when in a SELECT
  @compile.when(MightCast)
  def compile_mightcast(compile, expr, state):

As we discussed in the mailing list, it's best to have custom
compilers for default types (int, str, etc), and only do the
casting if state.context is COLUMN.

Notice that if I understood it correctly, firebird will break
with things like "SELECT 1+?" as well. The suggested approach
should work in these cases, while the currently implemented one
will not.

[5]

  def create_database(self):
      self.database = create_database(os.environ["STORM_FIREBIRD_URI"])

Ideally the test runner should create and drop the database refered to
in the URI by itself (using kinterbasdb.create_database/drop_database).

[6]

There are a few tab characters in the changeset. If possible, please
replace them by spaces.

[7]

  # We override the following tests because
  # they have static SQL that doesn't work with firebird
  def test_execute_result(self):
      """The base test does a SELECT without a FROM"""
      result = self.connection.execute("SELECT 1 FROM RDB$DATABASE")

The Oracle backend has the same problem. Rather than rewriting every
test, I suggest changing the base class to use "SELECT 1 FROM test"
in these tests. The test goal would continue to be met, and it should
work on all backends, including the upcoming Oracle one.

[8]

+ # these use get_insert_identity, so they don't work

Support for that is coming, as we discussed.

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Moshe has mentioned that he's not interested in signing the copyright assignment, so we
can't merge this branch on Storm. I'll keep the bug open as Firebird should be supported
at some point even then.

Changed in storm:
assignee: moshevds → nobody
Benji York (benji)
Changed in storm:
status: New → Won't Fix
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.