Bug fixes for composites

Issue #1132 resolved
Former user created an issue

(original reporter: dmiller) The attached patch fixes a bunch of issues with composite values. Specifically, it fixes issues dealing with null values. There is some ambiguity on how a composite should be loaded when all of its component values are null. See the mapper tests included in the patch for more notes on this.

Also, CompositeProperty.setattr violates the composite object protocol. See notes in that method (in the patch) for more details. This patch does not fix that issue.

Comments (5)

  1. Mike Bayer repo owner

    I agree on the __set_composite_values__ thing, we can make it an optional method so that if present it's used, otherwise we fall back to the current behavior.

  2. Mike Bayer repo owner

    my version of the patch is in d55d29329e6cc2f11d9dadb11f01ad6a7e8d258a.

    the "primary keys are all null" use case is not supported (is that what you actually wanted to do?). there is a test for a partially null primary key and i wasn't able to get it working in either mysql or postgres, but tests for sqlite.

    The test for setattr() is instead fulfilled by using regular table columns that have some defaults on them. the __set_composite_values__() method is now supported but is optional.

    The policy for columns all None is that the composite object is populated with NULL values. there can be a flag added at a later point which allows more control of this, if anyone actually wanted that.

    reopen in case i missed anything here.

  3. Former user Account Deleted

    (original author: dmiller) > the "primary keys are all null" use case is not supported (is that what you actually wanted to do? ...

    No, I was not sure how to exercise setattr() without using a primary key so that's how I wrote the test. Your test is much better.

    The policy for columns all None is that the composite object is populated with NULL values. there can be a flag added at a later point which allows more control of this, if anyone actually wanted that.

    I agree that the finer control is not necessary until someone requests it (I don't need it). It probably wasn't very clear, but my notes in the test were attempting to point out that whatever behavior we adopt will probably surprise someone sometime, because no matter which route you go you end up with some unexpected behavior.

    Your patch looks good. Thanks for working on this so quickly.

  4. Log in to comment