- attached composite_fixes.patch
Bug fixes for composites
(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)
-
Account Deleted -
repo owner - marked as critical
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. -
repo owner - changed status to resolved
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.
-
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.
-
repo owner - removed milestone
Removing milestone: 0.5.0 (automated comment)
- Log in to comment
(original author: dmiller) Patch generated against 5017