Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add polymorfic associations #61

Merged

Conversation

HolyWalley
Copy link
Contributor

@HolyWalley HolyWalley commented Jun 3, 2020

#58

@HolyWalley HolyWalley force-pushed the add_polymorfic_associations branch from 63c622c to 9f06ee6 Compare Jun 3, 2020
@HolyWalley
Copy link
Contributor Author

HolyWalley commented Jun 4, 2020

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 :)

@HolyWalley HolyWalley marked this pull request as ready for review Jun 4, 2020
Copy link
Owner

@DmitryTsepelev DmitryTsepelev left a comment

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)

lib/store_model.rb Show resolved Hide resolved
lib/store_model/types/polymorphic_array_type.rb Outdated Show resolved Hide resolved
lib/store_model/types/polymorphic_type.rb Outdated Show resolved Hide resolved
lib/store_model/types/polymorphic_array_type.rb Outdated Show resolved Hide resolved
@HolyWalley HolyWalley requested a review from DmitryTsepelev Jun 5, 2020
@DmitryTsepelev
Copy link
Owner

DmitryTsepelev commented Jun 8, 2020

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

Copy link
Owner

@DmitryTsepelev DmitryTsepelev left a comment

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 🙂

lib/store_model/types/polymorphic_type.rb Outdated Show resolved Hide resolved
lib/store_model/types/polymorphic_array_type.rb Outdated Show resolved Hide resolved
@HolyWalley
Copy link
Contributor Author

HolyWalley commented Jun 9, 2020

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
Copy link
Owner

DmitryTsepelev commented Jun 9, 2020

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)

...

@HolyWalley HolyWalley requested a review from DmitryTsepelev Jun 9, 2020
Copy link
Owner

@DmitryTsepelev DmitryTsepelev left a comment

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? 🙂

lib/store_model/types/base_array_type.rb Outdated Show resolved Hide resolved
lib/store_model/types/base_array_type.rb Show resolved Hide resolved
lib/store_model/types/base_array_type.rb Show resolved Hide resolved
@HolyWalley
Copy link
Contributor Author

HolyWalley commented Jun 11, 2020

@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.

  1. 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)

@HolyWalley HolyWalley requested a review from DmitryTsepelev Jun 11, 2020
@DmitryTsepelev
Copy link
Owner

DmitryTsepelev commented Jun 11, 2020

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.

Copy link
Owner

@DmitryTsepelev DmitryTsepelev left a comment

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

@DmitryTsepelev DmitryTsepelev merged commit 8b414a6 into DmitryTsepelev:master Jun 11, 2020
@Spone
Copy link

Spone commented Dec 28, 2020

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
Copy link
Owner

DmitryTsepelev commented Dec 29, 2020

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
Copy link
Contributor Author

HolyWalley commented Dec 29, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants