MONGOID-5734 Custom polymorphic types (backport to 8.1-stable) #5848
Conversation
* first pass at a global resolver registry * tests * fix problem with interpreting nested attribute data * need to register subclasses, too * raise custom exceptions when failing to resolve models * fix specs to implement functional around(:context) * trailing white space
| # Better practice would be to use unique strings instead of class names to | ||
| # identify these polymorphic types in the database (e.g. 'dept' instead of | ||
| # 'Department::Engineering'). | ||
| class UnrecognizedModelAlias < MongoidError |
There was a problem hiding this comment.
It might be nice to rename these new errors to include the word Polymorphic in their name, e.g. UnrecognizedPolypmorphicAlias. From their names they sound more generic than they actually are--the code docs indicate they are very specific to polymorphism.
| # resolver instance to use when registering the type. If :default, the default | ||
| # `ModelResolver` instance will be used. If any other symbol, it must identify a | ||
| # previously registered ModelResolver instance. | ||
| def identify_as(*aliases, resolver: :default) |
There was a problem hiding this comment.
How about calling this method polymorphic_as instead? indentify_as is quite generic, and may make users think think it has something to do with the _id field (which it does not.)
| @@ -50,12 +50,25 @@ def initialize(association, attributes, options) | |||
| @attributes = attributes.with_indifferent_access | |||
| @association = association | |||
| @options = options | |||
| @class_name = options[:class_name] ? options[:class_name].constantize : association.klass | |||
| @class_name = class_from(options[:class_name]) | |||
There was a problem hiding this comment.
This variable should be refactored to be called @klass instead of @class_name, because it's not a name String--its an actual Class object.
| # identifier. | ||
| # | ||
| # @return [ nil | Mongoid::ModelResolver ] the resolver to use | ||
| def resolver |
There was a problem hiding this comment.
Would recommend to rename to polymorphic_resolver or polymorphic_model_resolver because the name resolver is generic and could be used for many things in the context of a relation (for example, "resolving" that the other side of the relation exists in the DB)
| # | ||
| # @return [ nil | Mongoid::ModelResolver ] the resolver to use | ||
| def resolver | ||
| @resolver ||= Mongoid::ModelResolver.resolver(@options[:polymorphic]) |
There was a problem hiding this comment.
Same here ModelResolver --> PolymorphicModelResolver
|
Jamis this a great feature to have!! Thank you for it, something I've wanted for a long time. Please see comments, I think clear/specific naming could greatly enhance the developer experience and reduce potential for confusion/conflicts/heartbreak down the line. |
|
Hey @johnnyshields! Thanks for taking a look at the PR, and I'm very happy to hear that this is a feature you're excited about. One reason for the more general naming here is that there is some potential for a future PR to apply these methods to the single-table inheritance system. Mongoid's STI implementation already has some support for customizing the "discriminator", but the API is all private and the naming is a bit obtuse. Centralizing on a single system that can be used for both STI and the polymorphic associations would be better, I think, and in that case, having names that don't restrict us to polymorphic uses would be nice. |
|
Hmmm... STI is just another form of polymorphism, so the name I would assert that the names you have today are so generic as to be easily confused with things completely unrelated to "polymorphic relations" or "polymorphic collections". |
This is a backport of #5845 to 8.1-stable. Please see that PR for full information.