Adding the ability to join with OR from a where clause

[carolyncole]

This allows for finding all the objects that are 'this OR that' instead of just 'this AND that'
Added parens around cluases since 'abc' AND 'ddd' OR 'fff' is not the same as 'abc' AND ('ddd' OR 'fff'). This was not an issue before since all the clauses were joined with AND

[carolyncole]

I will also be creating a PR for master once all the kinks are worked out on this one.

[coveralls]

Coverage increased (+0.006%) to 93.927% when pulling e0ae847 on psu-stewardship:join_with into caa8c28 on projecthydra:9.7.x-stable.

[jcoyne]

If you set the default join with to be ' OR ' would that accidentally give you a bigger scope than you intended? Consider you have a belongs_to :collection and has_many :members.
When you build the query for "show me the things that belong to collection X and have a title like foo" like this: collection.members.where(title: 'foo') would that still work, or would it return all the members of the collection + non-members that have a title match?

[jcoyne]

would clauses.first be more efficient? Then we don't have to allocate a new string.

[carolyncole]

@jcoyne The default join_with is still AND. We are only using OR when the user passes it with a join_with option.
I do think the default OR might give us more than we expect, unless it was only used in the array case. I am not familiar with all the places this code is used, so I thought it safer to leave the default the same.

[coveralls]

Coverage increased (+0.008%) to 93.929% when pulling 730f41d on psu-stewardship:join_with into caa8c28 on projecthydra:9.7.x-stable.

[carolyncole]

@jcoyne I changed the default to ' OR ' and there are a bunch of test failures. I think leaving the default as 'AND' in 9.7 is the right move. We could consider changing the default in master.

To answer your question above you would still get an AND in between for example here is an error from the errors I got when I changed it to OR

        expected: ("_query_:\"{!raw f=has_model_ssim}SpecModel::Basic\" AND _query_:\"{!raw f=foo}9\\\" Nails\" AND (_query_:\"{!raw f=baz}7\\\" version\" AND _query_:\"{!raw f=baz}quack\")", {:sort=>["system_create_dtsi asc"]})
              got: ("_query_:\"{!raw f=has_model_ssim}SpecModel::Basic\" AND _query_:\"{!raw f=foo}9\\\" Nails\" AND (_query_:\"{!raw f=baz}7\\\" version\" OR _query_:\"{!raw f=baz}quack\")", {:sort=>["system_create_dtsi asc"]})

[jcoyne]

@Cam156 right, but that looks like a bad expectation to me. I'd like to see the use case that emerged from.

[carolyncole]

@jcoyne I agree the OR makes more sense, but I am trying to make the smallest change here in the 9.7.x branch so we can reasonably stay in the 9.7.x numbering. I can test changing to OR as the default with the sufia test, but I'm worried it will have unforeseen consequences in a patch release.

[awead]

The current expected behavior is AND. I think we should preserve the expected behavior. With this PR, you now have the option of using OR so I think this PR solves the problem as described.

[jcoyne]

To me it is clear that our existing API is not behaving correctly, and we should fix it. It is a bug (even if it causes some test failures). We should not introduce a new API which we must support in perpetuity to do exactly how it should have worked in the first place.

[jcoyne]

When I added this test https://github.com/psu-stewardship/active_fedora/commit/d0397ab800d90e119a9647e8f47b1f19f3b76ff1#diff-d6939355441580118b5afdbbd0a98049R44 , which will fail if we make my suggested change, it was not because I was being intentional about supporting that feature, it was more of just testing the existing behavior.

[carolyncole]

@jcoyne 👌 If this is a bug fix then I am good with it. I will make the change. We can always bug fix again. So you would like to drop the optional join_with and just change the where clause to use OR?

[coveralls]

Coverage increased (+0.006%) to 93.927% when pulling 3a90a44 on psu-stewardship:join_with into caa8c28 on projecthydra:9.7.x-stable.

[carolyncole]

@jcoyne Better?