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

ActiveRecord integration design #1

Closed
geekq opened this issue Jan 7, 2019 · 6 comments
Closed

ActiveRecord integration design #1

geekq opened this issue Jan 7, 2019 · 6 comments

Comments

@geekq
Copy link
Owner

geekq commented Jan 7, 2019

The goals are:

  • to support different ActiveRecord versions at the same time
  • to enable further development without regressions (without breaking applications using different versions of ActiveRecord)
  • convenience

For that we need

  • to have clear responsibilities for workflow and workflow-activerecord (separation of concerns)
  • single direction for dependencies

Plan is: workflow would contain changes to DSL defining the states and events, and workflow-activerecord would depend on particular ActiveRecord API

+------------------+     +-----------------------+
|    workflow      <-----+ workflow-activerecord |
+------------------+     +------+----------------+
                                |
                                |
+------------------+            |
| ActiveRecord     <------------+
+------------------+

We have some design options to choose from. Once chosen, it would be hard to change without breaking things. I've highlighted my current preference in bold.

Versioning workflow-activerecord gems

Imagine, Rails 6.0 brings some incompatible API changes. Think something like protected_attributes ;-) In this case workflow-activerecord for 4.1, 4.2, 5.0, 5.1, 5.2 would be based on the same code base, while workflow-activerecord for ActiveRecord 6.0 would require different implementation.

Option 1. Release workflow-activerecord version only when it's code base changes.

Easy to build/release. Harder to reference the right version. If your application is for Rails 5.2, use

gem 'workflow-activerecord', '>= 4.1', '< 6.0'

Option 2. Release workflow-activerecord version for every ActiveRecord version.

Makes easy for users to reference the right version. If your application is for Rails 5.2, use

gem 'workflow-activerecord', '~> 5.2'

But more work to release. Multiple versions, that are binary equal.

Referencing Gems

Option 1. workflow-activerecord defines dependency on workflow itself so user
just needs a single line:

gem 'workflow-activerecord', '>= 4.2', '< 6.0'

But which version of workflow should workflow-activerecord depend on? Latest? Some particular?

Consider this article about the differences in dependency versions for libraries vs. applications. https://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/

Option 2. User to write both dependencies in her Gemfile

gem 'workflow', '~> 2.0'
gem 'workflow-activerecord', '>= 4.2', '< 6.0'

require

In Rails you do not need to write require nowadays since it uses require 'bundler/setup' and always requires everything?

include

Option 1. Multiple includes

class Article < ApplicationRecord
  include Workflow
  include WorkflowActiverecord
  workflow do
    # list states and transitions here
  end

Option 2. Single include Workflow

When used in conjunction with workflow-activerecord and used inside a class inheriting from
ApplicationRecord, modify the behaviour of include Workflow to also activate the persistence.
But this is probably too much magic

Option 3. Single include WorkflowActiverecord

Probably the best trade-off the explicitness and convinience.

@voltechs What do you think?

@voltechs
Copy link

voltechs commented Jan 7, 2019

@geekq Looks good. I have two comments/suggestions.

Dependency Management

I'd encourage leveraging the built-in dependency management that bundler goes through. What you'll get from this is that workflow-activerecord will have a runtime dependency on activerecord, with a range of versions that it is known to support:
spec.add_runtime_dependency :activerecord, [">= 4.2", "< 6.0"]

You can't really support activerecord 6.0 until it's been released, so this is reasonable. You'll also specify a dependency to workflow, which will likely be semantic:
spec.add_runtime_dependency :workflow, '~> 2.0'

Now users who wish to use workflow with the activerecord adapter, they simply need to require one gem. The gem (workflow-activerecord) based on it's dependencies will automatically resolve to the correct (usually latest, unless locked) version for the user.

This also eliminates the need for the user to specify—redundantly—both gem 'workflow' and gem 'workflow-activerecord' in their Gemfile.

A user will have already specified gem 'rails', '~> x.x' in their Gemfile, and that will act as the limiting factor around which other gems jostle to find an appropriate version.

Let me know if I did a poor job of explaining that.

Activation

Given the incredibly simple entry-point for workflow, I recommend sending a :prepend or :include to ActiveRecord::Base upon ::ActiveSupport.on_load(:active_record). Users can immediately then use workflow do within their models.

P.S. I recommend snagging workflow-activerecord as soon as possible. Just throw up a 0.0.0 blank gem for now.

@geekq
Copy link
Owner Author

geekq commented Jan 8, 2019

Gem Dependency Management

I've created a sample application https://github.com/geekq/workflow-rails-sample
which I use in an empty rbenv to test dependency management with bundler.

Gem/bundler patterns and prerelease support work quite well! I was hesitating
a bit to reference workflow gem from workflow-activerecord since referencing an exact
version of a lib from another lib is not recommended. Referencing without version (latest?
any already installed version?) could potentially lead to an incompatible lib pair.
But following ranges feel OK.

Now I am happy with:

And just in case, the developer of an application can still put 2 gem lines and specify exact versions of both libraries. In addition Gemfile.lock ensures a repeatable build.

Gems released as:

Next step: Activation. Currently requires 2 includes, but will try out your suggestion on Thursday @voltechs

geekq pushed a commit to geekq/workflow-rails-sample that referenced this issue Jan 10, 2019
Single `include WorkflowActiverecord` as considered in geekq/workflow-activerecord#1
@geekq
Copy link
Owner Author

geekq commented Jan 10, 2019

I am hesitating with a magic workflow activation for every model @voltechs
since it would put some unneeded functionality to every model class
because including WorkflowActiverecord creates multiple methods, e.g. for the workflow spec and for persistency

module InstanceMethods
def load_workflow_state
read_attribute(self.class.workflow_column)
end
# On transition the new workflow state is immediately saved in the
# database.
def persist_workflow_state(new_value)
# Rails 3.1 or newer
update_column self.class.workflow_column, new_value
end
private
# Motivation: even if NULL is stored in the workflow_state database column,
# the current_state is correctly recognized in the Ruby code. The problem
# arises when you want to SELECT records filtering by the value of initial
# state. That's why it is important to save the string with the name of the
# initial state in all the new records.
def write_initial_state
write_attribute self.class.workflow_column, current_state.to_s
end
end

The Scopes extension would also need to be adjusted since workflow_spec would be nil.

def workflow_with_scopes(&specification)
workflow_without_scopes(&specification)
states = workflow_spec.states.values

At least, I've just reduces number of includes from 2 to 1 742b124

@voltechs
Copy link

voltechs commented Jan 10, 2019

I totally understand that perspective. I have a couple tricks that I use to keep it relatively clean.

Essentially what I do is I include one method onto ActiveRecord::Base which when called, injects into that class the rest of the extension. That way you're only "polluting" the base class with one method. I find this to be a healthy compromise. One could argue that it's a grey area for determining where to draw the line on composition vs inheritance. If you have a class that doesn't use any primary keys, why is primary key functionality in the base class? Why not just include that in all the classes you want a primary key in? I'm sure the line has something to do with majority use-cases, but it's not worth debating in this forum (maybe over beers 😛)

Anyhow, looks like you're making good progress! Looking forward to 2.0.0!

module Core
  # all your goodies
end

module Base
  def self.workflow(*args, &block)
    self.prepend(Workflow::Core) unless ancestors.include?(Workflow::Core)
    Workflow::Core.workflow(*args, &block)
  end
end

@geekq
Copy link
Owner Author

geekq commented Jan 12, 2019

In the real use case, out of, say 10 active model files, a one or two are going to use workflow.
So we are talking about saving 1-2 lines of code by applying magic to all models.
(if workflow-spec-defined-on-this-model then include_persistency)

I would stick to include WorkflowActiverecord for now and concentrate on finishing refactoring, separating READMEs and getting the first workflow 2.0 release and complementing workflow-activerecord release ready.

Please have a look at
https://rubygems.org/gems/workflow-activerecord/versions/4.1.2 (requires https://rubygems.org/gems/workflow/versions/2.0.0) and try it out!

@geekq
Copy link
Owner Author

geekq commented May 31, 2019

Will stick with include WorkflowActiverecord

@geekq geekq closed this as completed May 31, 2019
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

No branches or pull requests

2 participants