auto_strip_attributes icon indicating copy to clipboard operation
auto_strip_attributes copied to clipboard

Auto strip attributes...automatically?

Open vfonic opened this issue 3 years ago • 7 comments

I have similar issue like @skandragon in #4: "strip all non-serialized text and string fields" I want all of the string columns to be stripped automatically.

This is what I came up with:

# frozen_string_literal: true

module StripAttributes
  extend ActiveSupport::Concern

  included do
    extend AutoStripAttributes

    string_columns = self.columns.filter { |c| c.type == :string }.map(&:name)
    auto_strip_attributes(*string_columns)
  end
end

I'd prefer if this would be a configuration option so that I don't need to include this in every model.

Why? I think it's a good developer experience. Just like how I don't have to do many other things that come by default with rails (I don't have to convert "1" to 1 for integer columns, checkbox => boolean, etc.) It would be great if this functionality came with rails for those of us who are spoiled by rails. :) ...but it doesn't

If it's a configuration option, it shouldn't bother users who don't want to use this, while it would make my life easier and I'd probably include this gem in every rails project of mine.

vfonic avatar Apr 02 '21 15:04 vfonic

Couldn't you include that module in the ApplicationRecord class so that every model would have it automatically by inheritance?

No need to have it as a configuration option if its still just a small piece of code in a single file?

Although that seems like an elegant way to do that. In that sense if it works with ApplicationRecord it might be good to add to wiki for others to copypaste.

I don't think having all texts stripped by default is a good start although it works for many projects. Personally I wan't to specify stripping only happening on user inputs. Having something magic happening to texts that are only machine processed might cause making or missing some bugs.

holli avatar Apr 06 '21 15:04 holli

Can't remember from the top of my head, but I think self doesn't evaluate correctly when I put this in ApplicationRecord

vfonic avatar Apr 06 '21 15:04 vfonic

I want all of the string columns to be stripped automatically.

Doing things automatically seems aligned with a gem named auto_* 😉

pboling avatar Oct 18 '21 02:10 pboling

In my project I added this to ApplicationRecord:

def self.inherited(subclass)
  super

  # Run auto_strip_attributes on all :string columns.
  auto_strip_attributes(
    *subclass.columns.select { |c| c.type == :string }.map(&:name), nullify: false, squish: true
  )
end

I haven't yet figured out how to make this apply to external models (such as Delayed::Job or ActiveAdmin::Comment), but I probably just have to use an initializer to do something like ::ActiveRecord::Base.send :include, AutoStripAttributeExtension and setup AutoStripAttributeExtension to have the inherited classmethod.

It still would be nice to perhaps define that extension in this gem and document how to use it effectively. To echo what @pboling said, it would put a little more emphasis on the auto_ part of this gem's name.

gregschmit avatar Jan 04 '22 02:01 gregschmit

Update on my solution; I decided to put it in a concern and include it into specific models where I needed it. Obviously I used extra options that work with my particular application.

module AutoStrippable
  extend ActiveSupport::Concern

  included do
    # Strip string and squish to remove excess inline whitespace.
    string_columns = self.columns_hash.select { |_k, v| v.type == :string }.keys
    auto_strip_attributes(*string_columns, nullify: false, squish: true)

    # Strip text without squishing to allow multiple newlines in-between lines.
    text_columns = self.columns_hash.select { |_k, v| v.type == :text }.keys
    auto_strip_attributes(*text_columns, nullify: false, squish: false)
  rescue ActiveRecord::StatementInvalid
  end
end

I wonder if a good API for this is just a kwarg for all_string_columns and possible all_text_columns, perhaps also with an except kwarg. Then the user would still be in charge of determining the other options (squish, nullify, etc) on a per-model basis.

gregschmit avatar Jan 11 '22 05:01 gregschmit

How do you feel about having a documented approach to apply it to multiple models in a few different ways?

For example:

  • Applied to all models through ApplicationRecord
  • Applied to specific models where you include a concern to each model
  • Either of the above cases with an option to exclude specific fields in your model

The third bullet could be a use case when you're using Markdown. It supports adding 2 trailing spaces as valid syntax to add a <br> which you may want to keep on certain text fields.

nickjj avatar Dec 04 '22 17:12 nickjj

Yea having those in wiki (and a link in the front readme) would probably be most elegant way to do it? I guess anyone can create a new wiki page and a pull request to add a link.

btw. that Markdown example is a good addition on why adding automatically can cause some bugs.

holli avatar Dec 08 '22 12:12 holli