Need to take care with first_or_create

[pavoni]

Perhaps obvious to others - but I found quite a few issues in our code base where developers were using first_or_create with the gem - and it was causing production issues.

The problem is that the strip is done on validation, which means that the database search is executed with the unstripped string.

So if you have Thing(name: 'pavoni') in the database and name is unique, then:-

the_thing = Thing.where(name: '   pavoni    ').first_or_create

Will try to create a duplicate, because the where is executed before the strip, it will fail to find the record, and then try and create new one - but because of the strip this will be a duplicate and fail validation.

I suspect this may not be an easy fix, but perhaps a warning in the docs to take care - and perhaps expose the stripper in a way that it can easily be called on strings in the where clause.

I worked round the issue by stripping strings before calling the first_or_create, although it's important to duplicate the stripping logic in strip_attributes - or you can still get problems. So something like

the_thing = Thing.where(name: '   pavoni    '.strip_attributes ).first_or_create

Of course would love it if someone can think of a proper fix.

[rmm5t]

I don't think this gem should take on the responsibility of ensuring clean query params. I believe that's a job better done in the controller or in a specialized model method (in your application) that takes on the behavior of searching and returning the first record or creating a new one (based on your business rules). There's just too much to consider when building queries that I don't believe there's enough benefit in having this done generically in a gem like this one.

perhaps expose the stripper in a way that it can easily be called on strings in the where clause.

I actually think the stripper in strip_attributes is already overkill. It handles a lot of edge cases for blank UTF8 characters well beyond String#strip trying to avoid malicious data entry.

In the real world, good ol' String#strip should more than suffice for your scenario:

the_thing = Thing.where(name: '   pavoni    '.strip ).first_or_create

[pavoni]

@rmm5t The issue is that if you're using first_or_create - then you need to EXACTLY reproduce the edge cases or you get failures.

To start I'd found that you need squeeze(' ').strip, but am now fining production bugs where the strings (from CSV files and external APIs) include some of the exotic characters that the gem strips.

It's great that the gem fixes these - but If you want first_or_create to work then you need to strip all the characters that the gem strips, or you can have duplicate record issues.

I've now added a method to do this - but would be much better to be able to call the one in strip_attributes so it will evolve inline.

At a minimum I think you need to add a warning to the docs. There was a team of pretty experienced developers who didn't think twice about using first_or_create - and I've now found quite a lot of production issues caused by this.

Really useful gem, but needs a bit more caution that people realised.

[rmm5t]

At a minimum I think you need to add a warning to the docs. There was a team of pretty experienced developers who didn't think twice about using first_or_create - and I've now found quite a lot of production issues caused by this.

There's nothing wrong with using first_or_create. What's wrong is the query used before chaining first_or_create. Big difference here.

Getting first_or_create to work against incompatible where clauses is not the responsibility of this gem, and it's almost impossible to cover all possible query scenarios and edge cases.

This gem doesn't even have a dependency on activerecord (and it never will). It depends on activemodel only. Having this gem understand anything about activerecord or arel queries is completely beyond its scope.

Really useful gem, but needs a bit more caution that people realised.

Thanks. You're welcome to write a wiki page (I just re-enabled the wiki for this repository) about "gotchas" with certain ActiveRecord scenarios, but this kind of warning will never be part of the underlying code base or README. It isn't appropriate.

I've now added a method to do this - but would be much better to be able to call the one in strip_attributes so it will evolve inline.

Refactoring out the strip & collapse behavior of the current StripAttributes.strip(record, options) method into distinct methods (e.g. StripAttributes.strip_record and StripAttributes.strip_string) is something that I'm more willing to consider. Let me noodle on that. Open to further suggestions there.

[rmm5t]

@pavoni Please see 652618416018afc62c17826a21efe2da4cbb609f

You can now call something like:

StripAtributes.strip(" foo  bar \t", collapse_spaces: true) #=> "foo bar"

or

the_thing = Thing.where(name: StripAttributes.strip('   pavoni    ')).first_or_create

You can test this out in your project by changing your Gemfile to:

gem "strip_attributes", github: "rmm5t/strip_attributes", branch: "master"

Afterwards, please chime in here again, and I'll release an official version.

[pavoni]

That sounds like a good approach. Will try in the next couple of days.

Assume that'll strip the various exatic characters too.

Thanks for this

(do you want to re-open the issue in case there are other views)

[rmm5t]

Assume that'll strip the various exatic characters too.

Yes, correct.

Thanks for this

You're welcome.

(do you want to re-open the issue in case there are other views)

No. We're good here. For now, I'm just considering this a refactor...with benefits.

[pavoni]

:+1: Works perfectly. Thanks. Let me know when you release - and I'll switch.

I will write some words for the wiki.

[rmm5t]

v1.6.0 released.

Added a quick snippet to the README about direct usage patterns: https://github.com/rmm5t/strip_attributes#using-it-directly

[pavoni]

Have updated, and done the PR for our app. Thanks again.

[pavoni]

@rmm5t Expect you get notified - but draft wiki page here https://github.com/rmm5t/strip_attributes/wiki/Avoid-issues-when-using-the-gem-with-ActiveQuery-and-first_or_create--by-using-StripAttributes.strip

[rmm5t]

Thanks!! I don't get notified on wiki updates.

On Friday, January 23, 2015, Greg Dowling notifications@github.com wrote:

@rmm5t https://github.com/rmm5t Expect you get notified - but draft wiki page here https://github.com/rmm5t/strip_attributes/wiki/Avoid-issues-when-using-the-gem-with-ActiveQuery-and-first_or_create--by-using-StripAttributes.strip

— Reply to this email directly or view it on GitHub https://github.com/rmm5t/strip_attributes/issues/26#issuecomment-71217862 .

[god-anubis]

Sorry for "reopening" this one. However I believe to have found another way to deal with this issue that may be worth to put into "Usage patterns" of the README.

`class MyController < ApplicationController before_action :strip_params

private def strip_params params.transform_values! { |v| StripAttributes.strip(v) } end end`

You can even put this in the ApplicationController to have it work on all your parameters if you wish.

[rmm5t]

@god-anubis Thanks. Good tip. Feel free to add a page to the wiki about this. https://github.com/rmm5t/strip_attributes/wiki