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

Allow Invalid Values to be Converted to nil (optional) #46

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

johnnyb
Copy link

@johnnyb johnnyb commented Apr 22, 2022

This brings closer parity to how PostgreSQL handles UUIDs.

Copy link
Member

@markoudev markoudev left a comment

Choose a reason for hiding this comment

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

Thank you very much for your PR! I left a few requests. This work may also take care of a few other cases where people asked for not having an error thrown when a find_by is performed with an invalid UUID, but nil instead.

Could you please process the requests and add unit and integration tests for this new option? Thanks!

Comment on lines +5 to +9
attr_accessor :options

def initialize(opts = {})
self.options = opts
end
Copy link
Member

Choose a reason for hiding this comment

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

I really like to have default options available, also as an opportunity to see which options there are and what their default values are.

Suggested change
attr_accessor :options
def initialize(opts = {})
self.options = opts
end
attr_reader :options
DEFAULT_OPTIONS = {
# By default an +InvalidUUID+ error is raised when a UUID is invalid. If you want to get a
# nil value instead, register the attribute with `nil_invalid_values: true`.
nil_invalid_values: false
}.freeze
def initialize(opts = {})
@options = DEFAULT_OPTIONS.merge(opts)
end

Comment on lines +136 to +154
## Invalid Values

By default, this will raise an exception if an invalid value is attempted to be stored.
However, the PostgreSQL driver nils invalid UUID values.
To get PG-friendly behavior, pass in the option `:nil_invalid_values => true` when creating the attribute.

Additionally, as a convenience function, you can add the following to `ApplicationRecord` to get parity between PG and MySQL:

```
def self.uuid_fields(*fields)
if ActiveRecord::Base.connection.adapter_name.downcase.starts_with?("mysql")
fields.each do |fld|
attribute fld, MySQLBinUUID::Type.new(:nil_invalid_values => true)
end
end
end
```

Then, in your models, just begin them by marking the fields which are UUIDs.
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this to as a section under # Usage (after ## Tell your model how to handle..) please? I think we could just describe the options there, but don't need to specifically mention parity between Postgres and MySQL:

## Options
You can pass in the following options to `MySQLBinUUID::Type.new(option: "value")` when
registering the attribute:

  * `nil_invalid_values: true` (default: `false`) - When set to true, invalid UUIDs will be stored
    as `nil` values. When set the false (the default) an `InvalidUUID` error is raised. If you
    don't want `nil` values to be stored you can use a presence validator.


Then, in your models, just begin them by marking the fields which are UUIDs.

## UUID Type in migrations
Copy link
Member

Choose a reason for hiding this comment

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

I would like to leave this out as it was intentionally not included with this gem.

It's nice of you though to inform people how they could achieve this. I'd propose moving the example code you wrote into a Gist, and link to it in a new final line of the ## No uuid column in database migrations chapter in the README:

If you'd like to have a `uuid` type available in your migrations, take a look at this gist:
---gist-link-goes-here---

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.

2 participants