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

Introduce support for the Attributes API typing #1159

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bsimpson
Copy link

The column type itself is only a uuid in certain database
implementations like PostgreSQL

MySQL uuids are supportable using the Rails 5 Attributes API and
registering a new :uuid type. The column type is not a :uuid, but the
registered attribute type is a :uuid

e.g.

module ActiveRecord
  module Type
    class Uuid < ActiveRecord::Type::Binary
      def type
        :uuid
      end

      ...
    end
  end
end

ActiveRecord::Type.register(:uuid, ActiveRecord::Type::Uuid, adapter: :mysql2)

class Model < ApplicationRecord
  attribute :id, :uuid
end

Model.attribute_types[:id].type # => :uuid

Example gem making using of the Attributes API for uuids in MySQL: https://github.com/mathieujobin/activerecord-mysql-uuid-column

This is related to a closed issue "Support UUID columns that end in 'f'": 81034ea . This fix did not identify the Rails 5 attributes API types however.

The column type itself is only a uuid in certain database
implementations like PostgreSQL

MySQL uuids are supportable using the Rails 5 Attributes API and
registering a new :uuid type. The column type is not a :uuid, but the
registered attribute type is a :uuid

e.g.

module ActiveRecord
  module Type
    class Uuid < ActiveRecord::Type::Binary
      def type
        :uuid
      end

      ...
    end
  end
end

ActiveRecord::Type.register(:uuid, ActiveRecord::Type::Uuid, adapter: :mysql2)

class Model < ApplicationRecord
  attribute :id, :uuid
end

Model.attribute_types[:id].type # => :uuid
Copy link

@nilbus nilbus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the direction this is going, and this would be useful for me. It relies on a convention of the application or a gem defining a type with the name uuid, but I think it's pretty reasonable to assume that a type named uuid can be treated as a UUID without worrying about false positives.

I see the activerecord-mysql-uuid-column gem uses such a type named uuid, so this change should work perfectly with that gem.

👍 Nice work.

codenamev added a commit to doximity/shoulda-matchers-uuid that referenced this pull request Feb 6, 2020
Since we're taking advantage of storing UUIDs in MySQL as `binary`, the shoulda-matchers has an issue when the UUID ends in an `f`.  This has lead to some flakey tests.  [We made an effort to fix this upstream](thoughtbot/shoulda-matchers#1159), which doesn't appear to be gaining traction.  This extracts the fix into a gem so we don't have to support a hard fork for this.

Pivotal Story: https://www.pivotaltracker.com/story/show/164603806
Merges #1
LGTM given by: @bsimpson
codenamev added a commit to doximity/shoulda-matchers-uuid that referenced this pull request Feb 25, 2020
Since we're taking advantage of storing UUIDs in MySQL as `binary`, the shoulda-matchers has an issue when the UUID ends in an `f`.  This has lead to some flakey tests.  [We made an effort to fix this upstream](thoughtbot/shoulda-matchers#1159), which doesn't appear to be gaining traction.  This extracts the fix into a gem so we don't have to support a hard fork for this.

* Adds rubocop and fix linter issues
* Adds circleci config and ingore gem build artifacts
* Adds CHANGELOG.md with latest change
* Ensure database.yml is correctly setup for CI
* Ensure CI builds with mysql to ensure tests run
* Adds license to gemspec and updates readme with license info
* Prepare README for public consumption
* Update gem publishing to use Rubygems
* Removes dox-style in favor of raw rubocop configuration to make open-source compatable
codenamev added a commit to doximity/shoulda-matchers-uuid that referenced this pull request Feb 25, 2020
Since we're taking advantage of storing UUIDs in MySQL as `binary`, the shoulda-matchers has an issue when the UUID ends in an `f`.  This has lead to some flakey tests.  [We made an effort to fix this upstream](thoughtbot/shoulda-matchers#1159), which doesn't appear to be gaining traction.  This extracts the fix into a gem so we don't have to support a hard fork for this.

* Adds rubocop and fix linter issues
* Adds circleci config and ingore gem build artifacts
* Adds CHANGELOG.md with latest change
* Ensure database.yml is correctly setup for CI
* Ensure CI builds with mysql to ensure tests run
* Adds license to gemspec and updates readme with license info
* Prepare README for public consumption
* Update gem publishing to use Rubygems
* Removes dox-style in favor of raw rubocop configuration to make open-source compatable
@mcmire
Copy link
Collaborator

mcmire commented May 6, 2020

Hey folks. In an effort to lighten our load as maintainers and be able to serve you better in the future, the shoulda-matchers team is working on cleaning out the cobwebs in this repo by pruning the backlog. As there are few of us, there are a lot of items that will simply never earn our attention in a reasonable time frame, and rather than giving you an empty promise, we think it makes more sense to focus on more recent issues. That means, unfortunately, that we must close this PR.

Don't take this the wrong way: our aim is not to diminish the effort people have made or dismiss problems that have been raised. If you feel that we should reopen this PR, then please let us know so that we can reprioritize it. Thanks!

@mcmire mcmire closed this May 6, 2020
@nilbus
Copy link

nilbus commented May 6, 2020

@mcmire This is a useful contribution. I nominate it as a candidate for reopening and triage. Thanks!

@mcmire
Copy link
Collaborator

mcmire commented May 6, 2020

Ok! I've reopened it.

@mcmire mcmire reopened this May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants