Error when flushing object who's property name matches the column name

[guest]

The error is: AttributeError: 'ColumnProperty' object has no attribute 'key'

when a ManyToOne relation is declared with the same property name as the column name:

my = ManyToOne('MyEntity', colname='my')

I'm attaching a patch to elixir/tests that attempts to demonstrate the issue concisely.

[guest]

Okay. So I told myself I was just going to file this and leave it... but it was bugging me, so I did some more investigation.

I think there are at least two factors at play here. I've managed to identify one and provide a workaround for this issue.

I have not figured out how to solve the name collision issue. I run into different issues if I try to accomplish the same thing in sqlalchemy (proper). This led me to find a workaround by passing key="another_name" to the Column arguments (see attached).

When I tried to accomplish the same in Elixir, I ran into a different problem. I found if I supplied key="another_name" in the column_kwargs, the ForeignKeyConstraint wouldn't get created as expected. I tracked down the issue and have made a change to relationships.py that addresses the issue and doesn't seem to break any other tests.

I'm including a new patch that supersedes the previous patch. Applied to the trunk, it fixes the key= issue and provides 3 new test cases related to the column/property name collision. The case still fails where no key= parameter is supplied; this is something that should probably be fixed, but I suspect it's a SQLAlchemy issue more than an Elixir issue.

[guest]

SQLAlchemy - one-to-many with same name test case

[ged]

Honestly, I'm not sure this should be fixed. It seems normal to me that if you try to squeeze two different things into the same attribute, it bombs at you. So I'd consider this part of this ticket "invalid", but since this is really an SQLAlchemy "limitation", and you've provided a nice SA-only testcase, I've forwarded it there to see what Mike thinks about it (http://www.sqlalchemy.org/trac/ticket/1002).

Also, in the "workaround" patch, I fail to see the usefulness of the "test_many_to_two" test. Isn't it a duplicate of the test_multi test?

The "key" part of the patch is fine though, and is applied to trunk (r319). Thanks! And sorry for the rather negative response overall - that's my job as a maintainer to filter what goes in or not.

[guest]

Okay. So after some discussion, I believe I have a better solution.

zzzeek explained that the "correct" way to handle a column property that conflicts with the relation property is to rename the column in the mapper (by setting the mapper property explicitly instead of letting sqlalchemy do it when the class is mapped). For example, see http://www.sqlalchemy.org/trac/wiki/UsageRecipes/PropertyWithSameNameAsColumn.

Elixir already handles this case for regular columns at line 197 of fields.py... and one can take advantage of this behavior by explicitly declaring a Field. For example

class B(Entity):
  a_id = Field(Integer, colname='a')
  a = ManyToOne('A')

However, this is non-intuitive (for the user), and also exposes details of the Elixir framework that could otherwise remain abstracted (that a ManyToOne creates a Field, the type is Integer, but the ManyToOne will create the ForeignKeyConstraint).

To address this issue, I've attached a patch with unit test that I believe concisely and fairly-robustly avoids the naming conflict. It doesn't deterministically avoid naming conflicts. To do so would require a different approach.

[ged]

still needs "fixing", as per the discussion on the mailing list.

[guest]

I learned some more about the problem. While I proposed a workaround, that workaround only happens to work when defined on a new database (where Elixir creates the schema), and not one where the schema is predefined. I've create a new test case that exhibits the error.

For me, this is a blocking issue. I have a legacy database which has an inconsistently-named column, and I need to rename it in my model (to keep it forward-compatible with the next-generation database), but it appears I can't do it with Elixir as it is. Therefore, I'm raising the priority to normal. I'm continuing to work out a solution.

If someone can demonstrate a workable fix to the attached test case that fails, perhaps it would be appropriate to lower the priority again. I've gotten by with 0.5.2 by using the originally proposed patch, but since that's not acceptable, I'll work on another approach.

[guest]

Test case demonstrating one aspect of the issue.

[guest]

A test case demonstrating the error when autoload is used.

[guest]

I've attached ManyToOne_specified_fk_field.patch. Applied against the trunk, this patch is my attempt at addressing the issue described herein per the discussion in the forums. The attached test case demonstrates that it appears to work. I was a little surprised to find that Animal.c.owner_id existed, but it's .key is correct, so perhaps all is well.

Comments welcome.

[ged]

patch applied in r380. I'll wait for the documentation to close the ticket.

[guest]

Ignore that last attachment. It's not correct. I got ahead of myself with the error checking.

[guest]

Documentation and error handling.

[guest]

I've attached a new suggested patch that includes documentation and additional error handling to prevent the case where conflicts will occur.

[ged]

field_param_doc.patch applied (with wording changes) in r384, thanks. in comment:ticket:39:8, you tell me to:

Ignore that last attachment. It's not correct. I got ahead of myself with the error checking.

Which attachment exactly did you refer to? Also, in r380, I didn't commit include test_fk_column_same_name_as_property_autoload.py, as it didn't seem nicely integrated with other tests and I wasn't sure it provided any more test coverage.

I'm closing the ticket now. Please reopen it if I've committed anything wrong/the autoload thing should be committed.