Skip to content

Commit

Permalink
Ensure in-memory evaluation of QualifierOperatorContains selector has…
Browse files Browse the repository at this point in the history 
… the same result as generated SQL fetch.

When using EOQualifier.QualifierOperatorContains selector with a keyPath that has multiple manyToMany and/or toMany keys
the in-Memory evaluation will incorrectly return false in the cases where the result of the keyPath is nested arrays rather
than a flattened array that represents the effective SQL evaluation for the same qualifier. Thus in-Memory evaluation does
not match the behavior of SQL evaluation in this scenario.

This fix flattens the array object resulting from the valueForKeyPath so that the qualifier will correctly evaluate to true
when checking if the flattened array contains the object being evaluated.
  • Loading branch information
@kierankelleher
kierankelleher committed Aug 24, 2012
1 parent 58ddfea commit ea23f97
Showing 1 changed file with 40 additions and 0 deletions.
40 changes: 40 additions & 0 deletions Frameworks/Core/ERExtensions/Sources/er/extensions/qualifiers/ERXKeyValueQualifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,18 @@

import com.webobjects.eocontrol.EOKeyValueQualifier;
import com.webobjects.eocontrol.EOQualifier;
import com.webobjects.eocontrol.EOQualifierVariable;
import com.webobjects.eocontrol.EOQualifier.ComparisonSupport;
import com.webobjects.foundation.NSArray;
import com.webobjects.foundation.NSKeyValueCoding;
import com.webobjects.foundation.NSKeyValueCodingAdditions;
import com.webobjects.foundation.NSMutableArray;
import com.webobjects.foundation.NSSelector;
import com.webobjects.foundation._NSStringUtilities;

import er.extensions.eof.ERXQ;
import er.extensions.foundation.ERXArrayUtilities;
import er.extensions.foundation.ERXProperties;

/**
* ERXKeyValueQualifier is a chainable extension of EOKeyValueQualifier.
Expand All @@ -20,6 +27,11 @@ public class ERXKeyValueQualifier extends EOKeyValueQualifier implements IERXCha
* <a href="http://java.sun.com/j2se/1.4/pdf/serial-spec.pdf">Java Object Serialization Spec</a>
*/
private static final long serialVersionUID = 1L;

// Lazy static initialization
private static class PROPERTIES {
static boolean shouldFlattenValueObject = ERXProperties.booleanForKeyWithDefault("er.extensions.ERXKeyValueQualifier.Contains.flatten", true);
}

public ERXKeyValueQualifier(String key, NSSelector selector, Object value) {
super(key, selector, value);
Expand Down Expand Up @@ -62,4 +74,32 @@ public <T> T one(NSArray<T> array) {
public <T> T requiredOne(NSArray<T> array) {
return ERXQ.requiredOne(array, this);
}

/**
* Overridden to handle case of in-memory evaluation of QualifierOperatorContains selector and a keyPath that has multiple toMany and/or manyToMany-flattened relationships resulting in arrays of arrays rather than
* an array of discrete objects. In that case the object is evaluated against a flattened array which gives the same result as SQL evaluation.
*
* Since legacy code may depend on workarounds to the incorrect behavior, this patch can be disabled by setting the property <code>er.extensions.ERXKeyValueQualifier.Contains.flatten</code> to <code>false</code>
*/
@Override
public boolean evaluateWithObject(Object object) {
Object objectValue = NSKeyValueCodingAdditions.Utility.valueForKeyPath(object, _key);

if (_value instanceof EOQualifierVariable) {
throw new IllegalStateException("Error evaluating qualifier with key " + _key + ", selector " + _selector + ", value " + _value + " - value must be substitued for variable before evaluating");
}

if (_selector.equals(EOQualifier.QualifierOperatorCaseInsensitiveLike)) {
if (_lowercaseCache == null) {
_lowercaseCache = (_value != NSKeyValueCoding.NullValue) ? (_value.toString()).toLowerCase() : "";
}
return _NSStringUtilities.stringMatchesPattern(((objectValue != null) && (objectValue != NSKeyValueCoding.NullValue)) ? objectValue.toString() : "", _lowercaseCache, true);
}

// Flatten in case we have array of arrays
if (_selector.equals(EOQualifier.QualifierOperatorContains) && PROPERTIES.shouldFlattenValueObject && objectValue != null && objectValue instanceof NSArray) {
objectValue = ERXArrayUtilities.flatten((NSArray<?>) objectValue);
}
return ComparisonSupport.compareValues((objectValue != null) ? objectValue : NSKeyValueCoding.NullValue, _value, _selector);
}
}

4 comments on commit ea23f97

@MathieuClusiau
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Kieran,

I have a new solution for this issue and it's more general (not only for QualifierOperatorContains).

With this fix we can evaluate this kind of qualifier in-memory: A.QualY.dot(B.QualX).dot(C.QualW).eq(1)

Can you give me your feedback on it?

thanks

@OverRide
public boolean evaluateWithObject(Object object){
if(super.evaluateWithObject(object)){
return true;
}
Object objectValue = NSKeyValueCodingAdditions.Utility.valueForKeyPath(object, _key);
if (objectValue instanceof NSArray) { for (Object value : (NSArray) objectValue) {
NSDictionary<String, Object> newObject = new NSDictionary<String, Object>(value, _key);
if(evaluateWithObject(newObject)){
return true;
}
}
}
return false;
}

@kierankelleher
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm .... not sure if your logic is something that is desired with the end result of the key-path is a toMany. At first glance I don't feel comfortable with such a general change since it seems to me it is changing fundamentally how EOKeyValueQualifier, which is used ubiquitously, functions and could introduce all kinds of unexpected problems. A general, aka large impact, change like this needs serious testing to guarantee backwards compatability, hence why the original change was so very very specific. My vote would be no since I don't see advantage to it or need it.

@MathieuClusiau
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback. I will try to better explain my solution.

Advantages and need

Effectively your solution is good and I’m not trying to find a better way to solve the problem, but actually if you try to evaluate qualifier in-memory like this: A.QualB.dot(B.QualX).eq(1), it’s impossible because WebObjects returns an empty array. So I tried to find a way to solve the problem. After I found the solution, I realize that my Wonder was not up-to-date and I saw your solution. I made a few tests and realized that my solution worked with QualifierOperatorContains in-memory too. Can you just confirm that my fix could replace yours, wherever you used it?

Two use cases of the fix
NSArray list = ERXQ.filtered(listA, A.QualB.dot(B.QualX).eq(1)) or simply a fetch with the MockEditingContext.

Risks

To evaluate the risks that could possibly introduce all kinds of unexpected problems, I will comment each line of my solution.

@Override
public boolean evaluateWithObject(Object object){  
// First we call the parent method evaluateWithObject from WebObjects
    // if the result is true, we do nothing and we return true
    if(super.evaluateWithObject(object)){
        return true;
    }

    // Now we will check if the false result is due to a relationship. Relationships resulting in arrays of arrays.
    Object objectValue = NSKeyValueCodingAdditions.Utility.valueForKeyPath(object, _key);
    if (objectValue instanceof NSArray) {
        // We loop around the arrays of arrays to evaluate the discrete objects
        for (Object value : (NSArray) objectValue) {
            // We create a new object to be able to evaluate just like a discrete object 
            NSDictionary newObject = new NSDictionary(value, _key);
            // Recursive call with the new object
            if(evaluateWithObject(newObject)){
                return true;
            }
        }
    }
    return false;
}

I don’t think that I changed how EOKeyValueQualifier works fundamentally. I used exactly the code of WebObjects without a copy. To guarantee backwards compatibility, we can add a property to isolate the new code, but I think that my solution opens a new path that never worked before.

Logic

When we are with a key-path toMany, we try finding an object in a list. This is exactly what my solution does.

@kierankelleher
Copy link
Contributor Author

@kierankelleher kierankelleher commented on ea23f97 Aug 2, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.