Add polymorfic associations

[HolyWalley]

#58

[HolyWalley]

Hi @DmitryTsepelev
I'm not sure about putting .one_of directly into lib/store_model.rb, maybe you have any suggestion?

Also, maybe you can point on tests which are missing?

I don't have many experience contributing in gems, so, I want to apologize if I did something wrong :)

[DmitryTsepelev]

Hi @HolyWalley, looks good so far! I've added some ideas, let's try to move things a bit and see where it goes. Also, it would be great to have some kind of "integration" test with more complex block (e.g., StoreModel.one_of { |json| json[:v] == 2 ? Configuration : OldConfiguration }.to_type)

[DmitryTsepelev]

Sorry for the radio silence, I'll do my best to re–review in the next few days

[DmitryTsepelev]

Great work on specs! I have a couple more questions below and please add yourself to the changelog.

I feel like we could simplify ArrayType/PolymorphicArrayType and JsonType/PolymorphicType using inheritance or composition, please let me know if you want to try it out, or I'll merge the PR as is, the initial task is solved 🙂

[HolyWalley]

I'll check the comments later today.

Sure, I'm interested in refactoring this classes, I've got the main point. I'll try to deliver it in a couple days.

I'm not quite familliar with versioning, so I'm not sure, if I should add myself to 0.7.0 or create 0.7.1 line instead?

[DmitryTsepelev]

Sure, I'm interested in refactoring this classes, I've got the main point. I'll try to deliver it in a couple days.

Perfect, thank you so much! Probably creating a base classes for singular and multiple cases should do the trick

I'm not quite familliar with versioning, so I'm not sure, if I should add myself to 0.7.0 or create 0.7.1 line instead?

Oh, I've accidentally removed ## master line—it should be above 0.7.0 and you should add your line to it (after merge I'll add the version number before releasing):

# Change log

## master

- your line

## 0.7.0 (2019-11-19)

...

[DmitryTsepelev]

Sorry for another round of review, but I feel like we could bring some more OOP love here: the idea is that the parent class should define a generic behaviour and let implementations to dig into the details. Let's change this class to this:

# frozen_string_literal: true

require "active_model"

module StoreModel
  module Types
    # Implements ActiveModel::Type::Value type for handling an array of
    # StoreModel::Model
    class BaseArrayType < ActiveModel::Type::Value
      # Returns type
      #
      # @return [Symbol]
      def type
        raise NotImplementedError
      end

      # Casts +value+ from DB or user to StoreModel::Model instance
      #
      # @param value [Object] a value to cast
      #
      # @return StoreModel::Model
      def cast_value(value)
        case value
        when String then decode_and_initialize(value)
        when Array then ensure_model_class(value)
        when nil then value
        else
          raise_cast_error(value)
        end
      end

      # Casts a value from the ruby type to a type that the database knows how
      # to understand.
      #
      # @param value [Object] value to serialize
      #
      # @return [String] serialized value
      def serialize(value)
        case value
        when Array
          ActiveSupport::JSON.encode(value)
        else
          super
        end
      end

      # Determines whether the mutable value has been modified since it was read
      #
      # @param raw_old_value [Object] old value
      # @param new_value [Object] new value
      #
      # @return [Boolean]
      def changed_in_place?(raw_old_value, new_value)
        cast_value(raw_old_value) != new_value
      end

      protected

      def ensure_model_class(array)
        raise NotImplementedError
      end

      def cast_model_type_value(value)
        raise NotImplementedError
      end

      def raise_cast_error(value)
        raise NotImplementedError
      end

      private

      # rubocop:disable Style/RescueModifier
      def decode_and_initialize(array_value)
        decoded = ActiveSupport::JSON.decode(array_value) rescue []
        decoded.map { |attributes| cast_model_type_value(attributes) }
      end
      # rubocop:enable Style/RescueModifier
    end
  end
end

In this case it doesn't know how to handle cast error and knows nothing about model_class. All the methods that are not implemented should be protected. I guess we could apply the same idea to the singular hierarchy. What do you think? 🙂

[HolyWalley]

@DmitryTsepelev I've got the idea. Base classes contains only implementation of completely similar (for both cases methods, ex: changed_in_place?)

I'm curious about two situations:

1: imagine that we need to add a new array type, but the base implementation of changed_in_place? doesn't feet it. What should we do? Reimplement this method in this new class move it out from the base class? if we should reimplement it what's the difference between my option (when I put some implementation of ArrayType into BaseArrayType and overwrote it in polymorphic one? And if we should then move it out of Base class it will cause placing same implementation to two child classes.

2. Imagine that we found a case when we need to change changed_in_place? implementation for polymorphic array. What should we do? Override it in polymorphic or move out of base class + same questions as above.

I'm not trying to argue, of course, I think this implementation is good enough (and better than my first one), I'm just trying to understand inheritance a bit more, because that's the problem I sometimes face with. When some new subclass must override all the parent methods (and there is no obvious reason to make it a subclass except of code structure)

[DmitryTsepelev]

These questions are definitely reasonable 🙂I guess the rule to follow here is that "classes should be open for extension and closed for modification", in our case it means that methods that are not a subject for change (at least for now) should be in the base class. Since this API is private we will be able to change the hierarchy as soon as new requirement appears.

[DmitryTsepelev]

I guess we can call it a day. Great job, thank you so much!

[Spone]

Hi @HolyWalley @DmitryTsepelev could you add documentation about this feature, or give me some code examples so I can open a PR to add documentation?

Thanks!

[DmitryTsepelev]

Hi @Spone, great question, for some reason I was sure there is a piece of docs 😬I believe you could start with specs (one, two) and ask any questions you have, I'll be extremely happy to review such PR.

[HolyWalley]

Hi @Spone,
I can provide a small example, maybe we can polish it, or use as it is, but it shows the idea anyway

Imagine we have a quiz app, we have a model Card which represents a single piece of the whole quiz.

A simple card should have a pair of question and answer. ex: Q: "Answer to the Ultimate Question of Life, the Universe, and Everything", A: 42

but also we can have a lot of other different question types, one of it is mapping (you should map 4 events with 4 dates), etc.

So, for this case we can write something like this:

class Card < ApplicationRecord
  CARD_TYPES = %i[
    single
    mapping
  ].freeze

  attribute :data, (StoreModel.one_of do |json|
    card_type = json.symbolize_keys[:question_type]
    card_type = CARD_TYPES[card_type] if card_type.is_a?(Integer)

    case card_type.to_s
    when 'single'
      CardTypes::SingleAnswer
    when 'mapping'
      CardTypes::MappingAnswer
    else
      CardTypes::BaseType
    end
  end).to_type
end

# frozen_string_literal: true

module CardTypes
  class SingleAnswer
    include StoreModel::Model

    enum :question_type, Card::CARD_TYPES

    attribute :question
    attribute :answer

    validates :question_type, :question, :answer, presence: true
  end
end

# frozen_string_literal: true

module CardTypes
  class MappingAnswer
    include StoreModel::Model

    enum :question_type, Card::CARD_TYPES

    attribute :mapping

    validates :question_type, presence: true
    # add some validations for array of mappings
  end
end

From now we are able to do next thing:

mapping_card = Card.new(data: { question_type: :mapping, mapping: [['Beginning of WWII', '1939'], ['13th amendment', '1865']] })


single_question_card = Card.new(data: { question_type: :single, question: 'Answer to the Ultimate Question of Life, the Universe, and Everything', answer: 42 })

and it will be correct

Hope it helps