Added support for ignoring_interference_by_writer to validate_uniqueness_of

[tovodeverett]

I think I've finally managed to figure out how to write the tests, and so I could finally put together this pull request. This should help resolve the sort of issues seen in #786.

[houndci-bot]

Line is too long. [94/80]

[houndci-bot]

Put a comma after the last parameter of a multiline method call.

[houndci-bot]

Put a comma after the last parameter of a multiline method call.

[tovodeverett]

Hound finally stopped barking at me!

[mcmire]

Thanks for doing this -- I appreciate it. The thing is, I don't want to add ignoring_interference_by_writer to every matcher because I don't want people to use it blindly the first time they see an error. Yes, rescuing CouldNotSetAttributeError will make the test pass in most cases -- but the reason I added the exception is because I wanted people to pause a bit -- and in some cases the validation isn't even necessary or feasible. So I'd rather handle these cases individually and come up with specific solutions instead of giving people a shotgun solution. (I have ignoring_interference_by_writer on allow_value because it's already a lower-level matcher and I assume that people kind of know what they're doing if they drop down to that level.)

I just wrote this in #799, but I'm actually considering adding a cast_to qualifier to ValidationMatcher and the inclusion matcher so that you can tell the matcher, if the model typecasts incoming values to a certain class, then everything is okay. (In fact, perhaps you could either pass a class, or a lambda if you have some custom serialization code that isn't represented by a class.) What do you think about this?

[tovodeverett]

I think the idea of passing in a lambda works, but anything else is likely to fall short. This isn't so much serialization as normalization, and the limits of normalization are only bounded by what the developer can invent. The developer may trim leading and trailing spaces, may eliminate long lengths of white space, may proper case things, etc. I've written code in the past that would accept both Metric and US Customary unitized values and would store them as metric (i.e. 3.3 ft would get stored as 1.0 in a floating point field and be displayed as either 1.0 m or 3.3 ft depending upon the user's preferences). The issue there is that a) you're type casting to some type but b) the transformation may be fairly opaque (think of code that can accept -10.5, 10.5 S, -10 30', 10 30' S, as well as optional degree symbols, decimal minutes, seconds instead decimal minutes, etc.).

I'm using https://github.com/mdeering/attribute_normalizer for normalization, and I added custom normalizers for upcase and downcase that use the logic I used in the test suite for ignoring_interference_by_writer (i.e. value.respond_to?(:upcase) ? value.upcase : value). I do validate my normalizer in my application's test suite.

Personally, I'm happy with ignoring_interference_by_writer because the part of my test suite that validates the normalizer is now independent of the part of my test suite that validates the uniqueness constraints, allow_value, etc. Furthermore, I can divide that allow_value testing into 4 categories:

• to allow_value - values that are acceptable and require no cleanup by the normalizer
• not_to allow_value - values that don't get cleaned up by the normalizer, but that don't pass muster
• to allow_value ignoring_interference_by_writer - values that wouldn't be acceptable if the normalizer didn't clean them up, but that once cleaned up are legal
• not_to allow_value ignoring_interference_by_writer - values that get cleaned up by the normalizer, but that still don't pass muster

But it would be easy enough for me to define a lambda in one place with let and then to pass it in wherever I need it, and if the logic in the normalizer changes than I can just update that lambda in one place. I'll be the first to admit to having too much fun with let lambdas in RSpec - I have places in my spec where I use lambdas defined with let that implement a whole set of expectations (in this case, it's validating that image manipulation is doing the correct thing), and in one case a lambda to return a custom lambda for validating a given aspect of an image.

So I can go either way on this one - I agree that ignoring_interference_by_writer is a potentially dangerous tool in the hands of the uninformed, but it's also an easy way to say, "Hey, I do a bunch of normalization on this, so expect that."

[mcmire]

This isn't so much serialization as normalization, and the limits of normalization are only bounded by what the developer can invent.

Sigh... see... I feel like this is one of those cases where doing things the "Rails way" and stuffing all of this behavior into your ActiveRecord model goes a little too far. If it were me, I'd make a facade that was responsible for normalization and wouldn't even have to consider the possibility of typecasting or any of that jazz. I realize that not everyone shares or is going to share my opinion, though.

Anyway :) I'm a little split on this, but this seems like a bit of an edge case to me. My feeling is that cast_to will work in most cases, and it would be better to wait to see how badly ignoring_interference_by_writer would be needed. So I think I'll leave this open for now, but implement cast_to and release that, and then see how it goes from there.

[mcmire]

After some more thought on this, I think that adding ignoring_interference_by_writer is the best way to handle this issue without making tests that use this qualifier confusing (see #820 for more comments on this). I think it would be still useful, however, to track changes to values, but only bring them to the user's attention if the matcher in question ends up failing -- this way, the user (and I) will have better insight as to why that matcher is failing.

So here are some remaining tasks for this PR:

• Add tests for all of the validation matchers (I know -- maybe there's a way we can make a shared example group or something).
• Modify allow_value so that if the value set on the attribute and the value obtained from the attribute are different, an extra section is added to the failure message (and this is tested for every matcher -- maybe this could go in the shared example group as well)

I can begin working on this, although if you want to pitch in on the tests that's fine too.

[mcmire]

@tovodeverett I took the liberty of adding ignoring_interference_by_writer to every matcher myself (it turned out it took a lot of work -- you can see it the results in 1189934 and 5532f43). As a bonus, it's now enabled by default, but if an attribute changes incoming values, that's still tracked, so if the matcher fails for some reason, then we can still let the user know about that changing value. I'm going to close this, but would you be willing to point to the GitHub repo for your project and let me know if that resolves the issue you were seeing?

[tovodeverett]

That is beautiful! It sounds like you totally rethought ignoring_interference_by_writer along the way, which wasn't trivial. I feel a little guilty that you went to all that work, but I hope I'm not the only happy camper so you can amortize that work over a bunch of happy users! 👍