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

Auto strip attributes...automatically? #33

Open
vfonic opened this issue Apr 2, 2021 · 7 comments
Open

Auto strip attributes...automatically? #33

vfonic opened this issue Apr 2, 2021 · 7 comments

Comments

@vfonic
Copy link

vfonic commented Apr 2, 2021

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.

@holli
Copy link
Owner

holli commented Apr 6, 2021

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.

@vfonic
Copy link
Author

vfonic commented Apr 6, 2021

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

@pboling
Copy link

pboling commented Oct 18, 2021

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

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

@gregschmit
Copy link

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
Copy link

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.

@nickjj
Copy link

nickjj commented Dec 4, 2022

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.

@holli
Copy link
Owner

holli commented Dec 8, 2022

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.

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

5 participants