-
Notifications
You must be signed in to change notification settings - Fork 14
Allow the foreign keys generated for relationships to be DEFERRABLE. #13
base: master
Are you sure you want to change the base?
Conversation
This commit adds a new option for relationships -- :constraint_deferrable. The values available for it are documented in the README. If set, it will cause the migrations that generate the relationship's constraint to include DEFERRABLE and INITIALLY DEFERRED or IMMEDIATE. Caveat: the Oracle adapter change has not been tested.
@@ -128,6 +137,8 @@ module DataMapper | |||
# name of the foreign key constraint | |||
# @param [String] constraint_type | |||
# type of constraint to ALTER source_storage_name with | |||
# @param [String,nil] deferrable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely nitpicking here, but this should be [String, NilClass]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to change it if that's the project's standard, but YARD's documentation says to use [String, nil]
. See the first example under Declaring Types in YARD's Getting Started guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right you are. NilClass
is widely used as a YARD type declaration in the DM codebase, but that may be leftover from earlier YARD conventions.
Thanks for the correction!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's also true you might find stuff like [TrueClass, FalseClass]
, but I'm told that the official way is [Boolean]
.
For singletons I would probably rather use the more common name rather than the class name.
I have only used dm-constraints with MySQL (where I sorely missed DEFERRABLE constraints), and thus haven't had motivation to implement deferrability for an RDBMS that supports it. That said, this looks good to me. In the long run, DM2 will handle constraints completely differently than DM1 does, so we may have to revisit this issue at some point in the future (after DM2 is more complete). In the medium term—depending on motivation and how long DM2 takes—I would like to reify constraints as objects in their own right (instead of burdening class Source
include DataMapper::Resource
has n, :targets
constrain targets, :update => :cascade, :destroy => :protect, :deferred => true
end I have mixed feelings about constraints being defined on the parent, while the constraint itself is applied on the child, but I'll leave that concern for another day. Reifying Constraints into their own objects would require refactoring the SQL DDL generation code in the Adapter layer (specifically in @rsutphin do you have any interest in tackling such an undertaking? |
I'm not sure how necessary I can see changing the internals so that all the elements of a constraint are represented as a single object associated with a relationship instead of as separate attributes (this would simplify the internal interfaces, too, I think), but I would vote to keep the configuration on the relationship directly.
Maybe at some point, but unfortunately I don't have any time to allot to it now. I also have fairly limited experience with DM (this patch comes out of my first project using it) and I don't use it in what I think is its usual context (I use it as a component of a command-line based reporting & data translation system in which the models are generated from an outside specification, not a webapp with hand-written models). I might not be the best choice to undertake a developer-facing refactoring. |
True. Despite my proposal to that effect, I'm not convinced that separating constraint declaration out of the Relationship options is necessarily a good idea. That said, in the places I've used constraints I've also had many other options passed into my Relationship definition, which got to the point that it feels like a design smell indicating that Relationship definitions have too many responsibilities.
I'm fairly certain extracting constraint-related responsibilities into a single object would simplify the internals, including the DDL generation (one hints is the number of args that
I totally understand this. In fact, my inquiry was based on my shortage of time too (though I seem to have no shortage of project ideas).
Welcome to the DM community!
Thanks for jumping in and contributing!
Glad to hear about other uses to which DM is being put. So far I've only had occasion to use it in the 'common use-case' (hand-written models in a webapp). But, the 'drive everything from Ruby' aspect is one of the things I like best about DM: it facilitates automated model definition and other uses that I haven't ever considered or heard of.
Fair enough. Since you're the other recent contributor to dm-constraints (aside from myself), I may contact you if/when I undertake to extract constraint logic out of Relationship, if for nothing else than to ask for your help verifying that I didn't break anything that you're using. Cool, I'll check this out some time. Thanks again! |
This commit adds a new option for relationships —
:constraint_deferrable
. It can be set to one of these values:false
— constraint is always evaluated immediately; it may not be deferred (default)true
or:initially_deferred
— constraint may be deferred and is deferred by default:initially_immediate
— constraint may be deferred but is immediate by defaultIf set, it will cause the migrations that generate the relationship's constraint to include DEFERRABLE and INITIALLY DEFERRED or INITIALLY IMMEDIATE.
Caveat: I've only tested this on PostgreSQL. From reading documentation, I believe it will work on Oracle as well. It will not work on MySQL (scroll all the way to the end), but I have implemented it such that if the option isn't provided, the generated DDL is as it was without this patch.