Skip to content

Add ActiveRecord::Base::normalizes#43945

Merged
jonathanhefner merged 1 commit intorails:mainfrom
jonathanhefner:active_record-normalizes
Dec 21, 2022
Merged

Add ActiveRecord::Base::normalizes#43945
jonathanhefner merged 1 commit intorails:mainfrom
jonathanhefner:active_record-normalizes

Conversation

@jonathanhefner
Copy link
Copy Markdown
Member

This method can be used to declare normalizations for attribute values. Normalizations are applied when attributes are assigned or updated, and the normalized values will be persisted to the database. Normalizations are also applied to matching keyword arguments of finder methods. This allows a record to be created and later queried using an unnormalized value. For example:

class User < ActiveRecord::Base
  normalizes :email, with: -> email { email.strip.downcase }
end

user = User.create(email: " CRUISE-CONTROL@EXAMPLE.COM\n")
user.email                  # => "cruise-control@example.com"

user = User.find_by(email: "\tCRUISE-CONTROL@EXAMPLE.COM ")
user.email                  # => "cruise-control@example.com"
user.email_before_type_cast # => "cruise-control@example.com"

@jonathanhefner
Copy link
Copy Markdown
Member Author

Build failures are related. I will investigate...

@jonathanhefner jonathanhefner force-pushed the active_record-normalizes branch 2 times, most recently from c2ada11 to 9e0030b Compare December 22, 2021 22:41
@jonathanhefner
Copy link
Copy Markdown
Member Author

In the previous revision, I added ActiveModel::Type::Value#serialize_for_query to allow types to avoid double casting when persisting (once when the attribute is assigned, and once again when the attribute is serialized), but still cast values for queries. QueryAttribute#value_for_database would call this method instead of serialize.

I thought it might also be useful for existing cases where we want different serialization behavior for querying vs persisting, e.g. #35336.

This turned out to be unworkable for two reasons:

  1. Existing delegator types like ActiveRecord::Type::Serialized do not implement this method, and so it is delegated to their subtype. This means the default implementation of serialize_for_query calls serialize on the subtype, bypassing the delegator's serialize.

  2. Even if serialize_for_query did call the delegator's serialize, the delegator would still expect the subtype to cast if necessary when serializing, and so the subtype could not avoid double casting anyway.

The solution I have landed on is to let types opt in to wrapping cast results in decorator objects. Then those types can unwrap or cast as necessary when serializing.

Comment thread activerecord/lib/active_record/normalization.rb Outdated
Comment thread activerecord/lib/active_record/normalization.rb Outdated
@seanpdoyle
Copy link
Copy Markdown
Contributor

seanpdoyle commented Dec 23, 2021

Maybe I'm missing some context about the limitations at-play here, but could the normalizes class method be defined at the Active Model level for use outside of database-backed records?

Is the intent that normalizes is only meant to operate during reads and writes, and not during value assignment or instance construction?

@p8
Copy link
Copy Markdown
Member

p8 commented Dec 23, 2021

Love this! Always wanted to have this in Rails.

As with seems always required, what do you think about making it a regular argument?

  normalizes :first_name, -> attr { attr.strip }

Maybe that would get confusing with multiple attributes though...

  normalizes :first_name, :last_name, -> attr { attr.strip }

It seems the most used normalizers would be stripping and downcasing,
Maybe we could have an API similar to validates:

  normalizes :first_name, :last_name, strip: true, downcase: true

Then maybe if we extend the attributes API we could define validations and normalizers on a single line:

  attribute :first_name, validates: { presence: true }, normalizes: { strip: true }

Although it might also be something like:

  attribute :first_name, validates: { presence: true }, normalizes: -> (attr) { attr.strip }

@silva96
Copy link
Copy Markdown
Contributor

silva96 commented Dec 23, 2021

Brilliant!!!

@kjlape
Copy link
Copy Markdown

kjlape commented Dec 23, 2021

I like @p8's suggestion to extend the attributes API.

This feature looks awesome, but I'm hesitant to grow the ActiveRecord/Model API surface area. I'm most concerned about the growing potential for name conflicts and confusion that comes from a larger API surface area.

If there's a reason extending attribute doesn't work, can we make this feature opt-in?

@kaspth
Copy link
Copy Markdown
Contributor

kaspth commented Dec 23, 2021

Very interesting! I think the API needs more tweaking, but I don't know exactly what that would look like yet.

In the past I've wanted this on params since the permitting is also a form of sanitizing/normalizing/canonicalizing. E.g. params.require(:user).normalized(email: …).

@MatheusRich
Copy link
Copy Markdown
Contributor

MatheusRich commented Dec 23, 2021

@kaspth For reference, the gem dry-rails offers a DSL (backed by dry-schema) to do something similar. I know that there's a lot of things on those gems, but maybe we can pull some inspiration from there.

@p8
Copy link
Copy Markdown
Member

p8 commented Dec 23, 2021

We do something somewhat similar when coercing types.
For example for booleans, where "false", "f" , "0" are 'normalized' to
false in finder methods and upon assignment.

user = User.new(admin: 'F')
user.admin? # false
User.where(admin: 'F') # SELECT `users`.* FROM `users` WHERE `users`.`admin` = FALSE

https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/activemodel/lib/active_model/type/boolean.rb

@yuki24
Copy link
Copy Markdown

yuki24 commented Dec 24, 2021

In the past I used the AR Attribute API to normalize an attribute value (an example gist could be found here). It would be great if the same normalization is applied to find_by and where (or any case attribute values are evaluated):

# app/attribute_types/phone_number_type.rb
class PhoneNumberType < ActiveRecord::Type::String
  def cast(value)
    value.present? ? Phonelib.parse(value.to_s).to_s : super # https://github.com/daddyz/phonelib
  end

  def serialize(value)
    value.present? ? Phonelib.parse(value.to_s).to_s : super
  end
end

# app/models/user.rb
class User < ApplicationRecord
  attribute :phone_number, PhoneNumberType.new
end

# Somewhere in the app:
User.new(phone_number: "+13478941111").phone_number    # => "+13478941111"
User.new(phone_number: "+1(347)894-1111").phone_number # => "+13478941111"
User.new(phone_number: "+1 347.894.1111").phone_number # => "+13478941111"

user = User.create!(phone_number: "+1347.894.1111")
user.phone_number # => "+13478941111"

User.find_by(phone_number: "+1(347)894-1111").phone_number     # => "+13478941111"
User.where(phone_number: "+1 347.894.1111").first.phone_number # => "+13478941111"

Copy link
Copy Markdown
Contributor

@palkan palkan 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 feature itself (have been doing similar things via Attribute API for a long time, more below).

However, the amount of internal changes and new abstractions/entities added to the codebase doesn't correlate with my gut feeling (and previous experience).

As far as I can see, most of the new code tries to solve the problem of double-casting. We introduce new (internal) API methods (#value_was_cast), entities (CastResult), callbacks.

And I doubt we need all this complexity.

Because normalizations may be applied multiple times, they should be idempotent

Shouldn't normalization be idempotent itself? Like in the example: email.strip.downcase. That the most popular use-case.

I think, non-idempotent and expensive normalizations (or conversions) should be out of scope of the model attributes; they belong to another abstraction layer (handling user/external data, probably). It's not necessary to put everything into a model.

And we already have normalization—handling boolean values. I can't see a reason why this user-defined normalization shouldn't behave the same 🤷🏻‍♂️

Here is an example of achieving it w/o any changes to the internals: https://gist.github.com/palkan/b8b6e8b310030acc6d4660fc5de8c790

P.S. The double-casting problem worth exploring on its own, but, IMO, it's out-of-scope for this feature.

Comment thread activerecord/lib/active_record/normalization.rb Outdated
Comment thread activerecord/lib/active_record/normalization.rb
Comment thread activerecord/lib/active_record/normalization.rb
@jonathanhefner
Copy link
Copy Markdown
Member Author

@seanpdoyle

Maybe I'm missing some context about the limitations at-play here, but could the normalizes class method be defined at the Active Model level for use outside of database-backed records?

I would very much like to, but the type decoration mechanism for Active Record attributes does not exist for Active Model attributes. My past efforts to port code from Active Record to Active Model, or to refactor code with that goal in mind, haven't met with much success. Perhaps in the future, though!

Is the intent that normalizes is only meant to operate during reads and writes, and not during value assignment or instance construction?

No, it should work during assignment and construction. See this test, for example.

@jonathanhefner
Copy link
Copy Markdown
Member Author

@p8

Love this! Always wanted to have this in Rails.

❤️

As with seems always required, what do you think about making it a regular argument?
Maybe that would get confusing with multiple attributes though...

Yes, I wanted a clear distinction. Also, for the natural language reason I mentioned earlier.

It seems the most used normalizers would be stripping and downcasing, Maybe we could have an API similar to validates:

  normalizes :first_name, :last_name, strip: true, downcase: true

I would prefer to not limit the API to strings only. The value passed to the normalizer is a cast value, so you can call whatever methods you need on it.

If you only need to call one method, there is a shorthand:

normalizes :first_name, :last_name, with: :squish

(:point_up: Another reason to use a :with kwarg, rather than a regular arg.)

Then maybe if we extend the attributes API we could define validations and normalizers on a single line:

  attribute :first_name, validates: { presence: true }, normalizes: { strip: true }

Although it might also be something like:

  attribute :first_name, validates: { presence: true }, normalizes: -> (attr) { attr.strip }

Yes, that's an interesting idea.

@jonathanhefner
Copy link
Copy Markdown
Member Author

@kaspth

In the past I've wanted this on params since the permitting is also a form of sanitizing/normalizing/canonicalizing. E.g. params.require(:user).normalized(email: …).

I did consider this, but all the params APIs that I came up with felt more cumbersome. I think, in part, because params can be any shape. Whereas a model API can focus on attributes. Additionally, it allows the normalizations to be applied / re-used outside of the request cycle. Also, it allows normalizations to use type-cast values.

@jonathanhefner
Copy link
Copy Markdown
Member Author

@yuki24

In the past I used the AR Attribute API to normalize an attribute value (an example gist could be found here). It would be great if the same normalization is applied to find_by and where (or any case attribute values are evaluated):

# app/attribute_types/phone_number_type.rb
class PhoneNumberType < ActiveRecord::Type::String
  def cast(value)
    value.present? ? Phonelib.parse(value.to_s).to_s : super # https://github.com/daddyz/phonelib
  end

  def serialize(value)
    value.present? ? Phonelib.parse(value.to_s).to_s : super
  end
end

# app/models/user.rb
class User < ApplicationRecord
  attribute :phone_number, PhoneNumberType.new
end

Yes, that would be possible!:

class User < ActiveRecord::Base
  normalizes :phone_number, with: -> (string) { string.present? ? Phonelib.parse(string).to_s : string }
end

Or, maybe just:

class User < ActiveRecord::Base
  normalizes :phone_number, with: -> (string) { Phonelib.parse(string).to_s }
end

And User.new, User.create, User.find_by, and User.where will work as you describe.

@jonathanhefner
Copy link
Copy Markdown
Member Author

@palkan

As far as I can see, most of the new code tries to solve the problem of double-casting. We introduce new (internal) API methods (#value_was_cast), entities (CastResult), callbacks.

And I doubt we need all this complexity.

The bulk of the code is activerecord/lib/active_record/normalization.rb. There are only 13 lines of (actual) code outside of activerecord/lib/active_record/normalization.rb that deal with double-casting, and only 1 line of code inside activerecord/lib/active_record/normalization.rb that deals with double-casting.

I put CastResult, wrap_cast_result, and unwrap_or_cast in ActiveModel::Type::Value for other subclasses to use, but I could move them to NormalizedValueType. value_was_cast is the only necessary part of that API. Subclasses could implement it in whatever way they choose.

I felt that solving double-casting was important because I did not want users to have to consider performance when declaring a normalization. See Yuki's use case, for example. Is Phonelib.parse fast or slow? If we only apply it once, it does not matter.

Because normalizations may be applied multiple times, they should be idempotent

Shouldn't normalization be idempotent itself? Like in the example: email.strip.downcase. That the most popular use-case.

I am not sure what you are asking. Maybe it's a difference in terminology? Here I am using the term "normalization" to refer to the value of the :with argument. I am trying to document that the value of :with should be idempotent.

I think, non-idempotent and expensive normalizations (or conversions) should be out of scope of the model attributes

Non-idempotent normalizations are not "supported", but they are not prohibited either. (Prohibiting them would likely be impossible.)

With regards to expensive normalizations, I disagree. Again, see Yuki's use case, for example.

@palkan
Copy link
Copy Markdown
Contributor

palkan commented Dec 27, 2021

Here I am using the term "normalization" to refer to the value of the :with argument

Your definition of "normalization" is "do whatever you want"; technically, we can't avoid it; but semantically it sounds to me as "do some minor transformation", like, remove whitespaces or normalize a phone number (that's fast, for sure; memoizing wouldn't be noticeable).

Non-idempotent normalizations are not "supported", but they are not prohibited either. (Prohibiting them would likely be impossible.)

In Ruby, we can not prohibit anything. In Rails, we can demonstrate best practices though. And not adding hidden expensive calculations to models is one such.

I did not want users to have to consider performance when declaring a normalization

Considering performance lead to better software design. Isn't that something we want for Rails users?

@palkan
Copy link
Copy Markdown
Contributor

palkan commented Dec 27, 2021

Maybe we could have an API similar to validates:

normalizes :first_name, :last_name, strip: true, downcase: true

Then maybe if we extend the attributes API we could define validations and normalizers on a single line:

attribute :first_name, validates: { presence: true }, normalizes: { strip: true }

I like it. This is the Rails way. Having a parity with validations API ❤️

Yeah, the values are not only strings; but we can tell the same about validations.

Maybe it worth starting with the activemodel-normalization gem first instead of adding this right away to Rails?
The benefits are: 1) Rails 6-7.0 users could use it right away; 2) letting more people access to it would help to stabilize the API and cut the rough edges.

@jonathanhefner
Copy link
Copy Markdown
Member Author

I did not want users to have to consider performance when declaring a normalization

Considering performance lead to better software design. Isn't that something we want for Rails users?

I should have worded that more clearly. What I meant to say was: I don't want users to think twice about using normalizes out of concern for performance overhead (compared to applying their normalizations manually).

I like it. This is the Rails way. Having a parity with validations API heart

At its core, all a validation does is check that a value is a member of a set. That set might be described by a range or a regular expression, but the concept is the same. Such logic can be easily configured with data structures, as seen in the validations API. But, even so, we can still specify validations with arbitrary logic using validate.

Normalizations, on the other hand, can be more diverse. A normalization like Phonelib.parse would not be possible without support for arbitrary logic.

In Ruby, we can not prohibit anything. In Rails, we can demonstrate best practices though. And not adding hidden expensive calculations to models is one such.

Normalizations are no more hidden than validations or callbacks. And, as with both validations and callbacks, we may encourage writing fast code, but the user has the freedom to trade performance for correctness when necessary.

@jonathanhefner jonathanhefner force-pushed the active_record-normalizes branch 4 times, most recently from 1ffbcbd to ea2e4e1 Compare January 20, 2022 20:03
@jonathanhefner
Copy link
Copy Markdown
Member Author

I've made a few small changes. Most notably, you can now specify a normalization as an array of symbols:

class User < ActiveRecord::Base
  normalizes :email, with: [:strip, :downcase]
end

@jonathanhefner jonathanhefner merged commit 52146de into rails:main Dec 21, 2022
@szymonlipka
Copy link
Copy Markdown
Contributor

@jonathanhefner One thing to consider.

I was trying to test this new feature and as first I did something like this:

class Client < ActiveRecord::Base
  normalize :email, with: -> email { email.strip.downcase }
end

And it declared class without errors. But the normalization wasn't working. It took me a moment to realize that I have writen normalize instead of normalizes and apparently it hasn't raised NoMethodError because of existing normalize method. I think that might be common thing and maybe would be good to not accept same arguments in normalize as in normalizes :)

@solnic
Copy link
Copy Markdown

solnic commented Dec 22, 2022

What if normalization fails? What if a given value doesn't have expected type yet normalization is applied with unexpected results? What if you do not want to normalize in various contexts?

In general, applying normalization outside of validation process is not a good idea. I think this feature needs more thinking and was merged too soon.

@simi
Copy link
Copy Markdown
Contributor

simi commented Jan 2, 2023

@solnic it is actually not so bad. It is almost shortcut for simplified inline attribute definition. But what I don't understand is the reason to use additional before_validation callback in here. That's inconsistent IMHO with other behaviour (for example with raw attribute usage).

What's the reason to make it this special? Wouldn't make sense to enhance attribute to make it possible to ensure the state before validation and make any usage of attribute benefiting from this? Currently this is handling just one specific use-case. The way to go IMHO is enhancing and promoting attribute usage and make normalizes just shortcut for one common "call sign".

@p8
Copy link
Copy Markdown
Member

p8 commented Jan 2, 2023

I’m also wondering if it makes sense to move the non-database logic to Active Model.

edit: Sorry, this was already discussed extensively.

@solnic
Copy link
Copy Markdown

solnic commented Jan 2, 2023

@simi it is actually that bad 🙂 it adds a new dimension of complexity to ActiveRecord and I bet it will become more complicated over time with stuff like "if/unless" options etc. AR models shouldn't be bothered with this type of logic and it could be a great opportunity to think about how to enhance strong_parameters instead.

@fxn
Copy link
Copy Markdown
Member

fxn commented Jan 2, 2023

I think this feature needs more thinking and was merged too soon.

One can disagree with the feature as written, but looking at the history of this PR, I don't think it can be said it was merged too soon.

adds a new dimension of complexity to ActiveRecord and I bet it will become more complicated over time with stuff like "if/unless" options etc.

Adding features adds complexity always. If this was a valid argument to reject stuff, software would stay frozen at version 1.0. Given one particular change, you decide whether you want it or not, acknowledging the implications, which are always trade-offs.

it could be a great opportunity to think about how to enhance strong_parameters instead.

Using your argument, that would add complexity to strong parameters!

Complexity is not the point.

I honestly think there are different ways to look at this feature. You would have written it in a different way, and that is totally fine.

The point is at which boundary you want to act? There are several. For you, much higher. Totally legit! In this design, in the central path to persistence. Yet another point of view would have written a database trigger so that no matter how is this field set, you get it normalized 100% guaranteed.

@solnic
Copy link
Copy Markdown

solnic commented Jan 3, 2023

Using your argument, that would add complexity to strong parameters!

Yes but it's incomparable to ActiveRecord. Possible codepaths multiple much more significantly when a feature like this is used in AR vs strong_params.

Complexity is not the point.

It's 100% the point. AR is known to cause so much trouble in so many Rails codebases and this makes it worse.

The point is at which boundary you want to act? There are several. For you, much higher. Totally legit! In this design, in the central path to persistence.

It's just more sensible to handle normalization outside of persistence. It's something that's context-specific, sometimes you need it, sometimes you don't.

@fxn
Copy link
Copy Markdown
Member

fxn commented Jan 3, 2023

It's just more sensible to handle normalization outside of persistence. It's something that's context-specific, sometimes you need it, sometimes you don't.

Rails Contributors needs to apply Unicode normalization to people's names. No exceptions. It wouldn't be a good design to identify all external entry points, and make sure all of them normalize. In this use case, you want to funnel everything down to one central normalization point, no matter the origin of the data, be it a UI, a batch process, or a test fixture.

Features are designed for certain use cases.

If your application has a concept of normalization that depends on context, you just don't implement it this way. That is all. If your framework had a normalization feature more decoupled and at a higher level, I would not use it for Rails Contributors because I need to dig it as deep in the edit chain as possible.

To me, this feature is perfectly good for its use cases.

@simi
Copy link
Copy Markdown
Contributor

simi commented Jan 3, 2023

Rails Contributors needs to apply Unicode normalization to people's names. No exceptions. It wouldn't be a good design to identify all external entry points, and make sure all of them normalize. In this use case, you want to funnel everything down to one central normalization point, no matter the origin of the data, be it a UI, a batch process, or a test fixture.

🤔 And how do you benefit from the before_validation part? Wouldn't be introducing custom attribute enough for your usecase?

@fxn
Copy link
Copy Markdown
Member

fxn commented Jan 3, 2023

And how do you benefit from the before_validation part? Wouldn't be introducing custom attribute enough for your usecase?

That normalization was written some 10 years ago, it is a simple wrapper around write_attribute. We are talking here about the boundary and its use cases, not much about the fine details of the implementation at this boundary.

@simi
Copy link
Copy Markdown
Contributor

simi commented Jan 8, 2023

We are talking here about the boundary and its use cases, not much about the fine details of the implementation at this boundary.

I'm actually referring to the implementation details. Currently it is shortcut to configure before_validation + attribute API. That seems super specific one case which could be implemented easily on user side (in the Rails app itself). I would expect more generic solution in Rails itself (for example inline attribute + configuration to hook in attribute lifecycle, so any attribute can benefit from this, not only this super specific case) combined with documented example to use it for normalization for example

If I understand it well, you're referring to this https://github.com/rails/rails-contributors/blob/f5fc1ed3fd42867b3daa20df3f64801e47f31635/lib/nfc_attribute_normalizer.rb#L3. That's "normalizing" values only on assignment. That is exactly what I would expect in here as well.

@collimarco
Copy link
Copy Markdown
Contributor

Does normalization run before validation?

@ghiculescu
Copy link
Copy Markdown
Member

ghiculescu commented Jan 17, 2023

@collimarco yeah it runs when the attribute is set.

@szymonlipka I agree it's subtle. But it's tricky to fix, because technically this is a valid thing to do: User.normalize(:email, with: -> email { email.strip.downcase }) (the name is :email and the value is a lambda).

One idea would be to log a warning if the name isn't recognised. Something like this:

diff --git a/activerecord/lib/active_record/normalization.rb b/activerecord/lib/active_record/normalization.rb
index c77630f1bd..4c8ae10585 100644
--- a/activerecord/lib/active_record/normalization.rb
+++ b/activerecord/lib/active_record/normalization.rb
@@ -94,6 +94,10 @@ def normalizes(*names, with:, apply_to_nil: false)
       #   User.normalize(:email, " CRUISE-CONTROL@EXAMPLE.COM\n")
       #   # => "cruise-control@example.com"
       def normalize(name, value)
+        unless self.normalized_attributes.include?(name.to_sym)
+          Rails.logger.warn "#{name} is not a normalized attribute. Did you mean to use `normalizes`?"
+        end
+
         type_for_attribute(name).cast(value)
       end
     end

@jonathanhefner do you have any thoughts on this? Now I write it out, we could even raise in this situation, since I'm not sure why you'd use User.normalize on an attribute that isn't being normalised.

@jonathanhefner
Copy link
Copy Markdown
Member Author

@ghiculescu @szymonlipka

Now I write it out, we could even raise in this situation, since I'm not sure why you'd use User.normalize on an attribute that isn't being normalised.

The reason I omitted that kind of error checking from ::normalize was because I wanted to support use cases like:

# Cat(id: integer, name: string, breed: string, lives: integer)
class Cat < ActiveRecord::Base
  normalizes :breed, with: -> { _1.upcase }
end

def normalize_cat_params(cat_params)
  cat_params.to_h { |name, value| [name, Cat.normalize(name, value)] }
end

normalize_cat_params({ name: "Stripes", breed: "Tabby", lives: "7" })
# => { name: "Stripes", breed: "TABBY", lives: 7 }

If we added the proposed error checking to normalize, normalize_cat_params would have to check whether an attribute has a normalization declared. Furthermore, for consistency, normalize_cat_params would have to include an else branch to type cast attributes that don't have normalizations, e.g. lives.

But, if there is confusion, we can rename ::normalize. In #47034, I've renamed ::normalize to ::normalize_value_for and added test coverage for the above use case. Let's continue the discussion there.

@jrochkind
Copy link
Copy Markdown
Contributor

Exciting feature! I am curious about any current thoughts as to supporting for use in non-ActiveRecord ActiveModel::Attributes classes too.

I know that @jonathanhefner addressed this in this comment saying:

I would very much like to, but the type decoration mechanism for Active Record attributes does not exist for Active Model attributes. My past efforts to port code from Active Record to Active Model, or to refactor code with that goal in mind, haven't met with much success. Perhaps in the future, though!

But that was a year ago, so it kind of is the future, and with this closer to actually dropping into a Rails release, I was curious if there were any updated thoughts to share.

It doesn't seem like it should be too hard to make ActiveModel::Attributes attribute implementation support "type decoration" by checking attribute_types for an existing base type... but I haven't actually tried, and @jonathanhefner has, so!

chrislo added a commit to alphagov/signon that referenced this pull request Sep 11, 2023
Organisations with unnecessary whitespace don't sort in an expected
way. Before fixing exisiting records with this problem, this commit
prevents any new occurrences.

Rails 7.1 introduces ActiveRecord::Base::normalizes[1] which could be
used to simplify this code. That hasn't yet been released and it seems
a bit unconventional within GDS to depend on edge rails, but when 7.1
is out, we should be able to adopt that pattern.

[1] rails/rails#43945
chrislo added a commit to alphagov/signon that referenced this pull request Sep 11, 2023
Organisations with unnecessary whitespace don't sort in an expected
way. Before fixing exisiting records with this problem, this commit
prevents any new occurrences.

Rails 7.1 introduces ActiveRecord::Base::normalizes[1] which could be
used to simplify this code. That hasn't yet been released and it seems
a bit unconventional within GDS to depend on edge rails, but when 7.1
is out, we should be able to adopt that pattern.

[1] rails/rails#43945
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.