Bug in AdvancedCriteria converstion

[chimpeenuts]

Using Smart GWT 3.0 nightlies.

I believe there is a bug in the asAdvancedCriteria method logic on the Criteria class.

Steps to reproduce:
1) Create a standard Criteria object.
2) Add to it a single Criteria of "id" with an array of Doubles
3) Convert the Criteria object to an AdvancedCriteria object using the asAdvancedCriteria() method
4) Note how the new advancedCriteria contains a list of criterions joined by OR, but that each individual criterion is using 'iContains'
5) Try to fetch data with desired criteria and notice how the array originally contained [1, 2], BUT records with 'id' 1, 2, 11, 12, 21, 22, 31, 32, etc. are returned b/c of the iContains operator. Believe this operator should instead by EQUALS or a list should be created in the AdvancedCriteria object and IN_SET operator should be used. Note how the conversion method knows to keep the value as a number, not a string which is good, but the iContains throws everything off.

Example follows:

Criteria crit = new Criteria();
crit.addCriteria("id", new Double[] { new Double(1), new Double(2) });
System.out.println(JSON.encode(crit.getJsObj()));
AdvancedCriteria advCrit = crit.asAdvancedCriteria();
System.out.println(JSON.encode(advCrit.getJsObj()));

Output:

{
    "__gwt_ObjectId":4244, 
    "id":[
        1, 
        2
    ]
}
{
    "_constructor":"AdvancedCriteria", 
    "operator":"and", 
    "criteria":[
        {
            "fieldName":"__gwt_ObjectId", 
            "operator":"equals", 
            "value":4244
        }, 
        {
            "_constructor":"AdvancedCriteria", 
            "operator":"or", 
            "criteria":[
                {
                    "fieldName":"id", 
                    "operator":"iContains", 
                    "value":1
                }, 
                {
                    "fieldName":"id", 
                    "operator":"iContains", 
                    "value":2
                }
            ]
        }
    ], 
    "__gwt_ObjectId":4246
}

Here's the request and response of my actual data fetch. Note that I am passing 2 and 3 to the criteria object for field 'device_id'. I have some additional criteria too but that's working.
Request (see iContains operator for two List objects in the criteria section)

<transaction xmlns:xsi="http://www.w3.org/2000/10/XMLSchema-instance"
  xsi:type="xsd:Object">
  <transactionNum xsi:type="xsd:long">13</transactionNum>
  <operations xsi:type="xsd:List">
    <elem xsi:type="xsd:Object">
      <criteria xsi:type="xsd:Object">
        <_constructor>AdvancedCriteria</_constructor>
        <operator>and</operator>
        <criteria xsi:type="xsd:List">
          <elem xsi:type="xsd:Object">
            <fieldName>__gwt_ObjectId</fieldName>
            <operator>equals</operator>
            <value xsi:type="xsd:long">6544</value>
          </elem>
          <elem xsi:type="xsd:Object">
            <_constructor>AdvancedCriteria</_constructor>
            <operator>or</operator>
            <criteria xsi:type="xsd:List">
              <elem xsi:type="xsd:Object">
                <fieldName>device_id</fieldName>
                <operator>iContains</operator>
                <value xsi:type="xsd:long">2</value>
              </elem>
              <elem xsi:type="xsd:Object">
                <fieldName>device_id</fieldName>
                <operator>iContains</operator>
                <value xsi:type="xsd:long">3</value>
              </elem>
            </criteria>
          </elem>
          <elem xsi:type="xsd:Object"></elem>
          <elem xsi:type="xsd:Object">
            <fieldName>ospf_id</fieldName>
            <operator>notNull</operator>
            <value xsi:type="xsd:boolean">true</value>
          </elem>
        </criteria>
        <__gwt_ObjectId xsi:type="xsd:long">6546</__gwt_ObjectId>
      </criteria>
      <operationConfig xsi:type="xsd:Object">
        <dataSource>routing_network</dataSource>
        <operationType>fetch</operationType>
        <textMatchStyle>exact</textMatchStyle>
      </operationConfig>
      <startRow xsi:type="xsd:long">0</startRow>
      <endRow xsi:type="xsd:long">50</endRow>
      <componentId>isc_ListGrid_16</componentId>
      <appID>builtinApplication</appID>
      <operation>routing_network_fetch</operation>
      <oldValues xsi:type="xsd:Object">
        <_constructor>AdvancedCriteria</_constructor>
        <operator>and</operator>
        <criteria xsi:type="xsd:List">
          <elem xsi:type="xsd:Object">
            <fieldName>__gwt_ObjectId</fieldName>
            <operator>equals</operator>
            <value xsi:type="xsd:long">6544</value>
          </elem>
          <elem xsi:type="xsd:Object">
            <_constructor>AdvancedCriteria</_constructor>
            <operator>or</operator>
            <criteria xsi:type="xsd:List">
              <elem xsi:type="xsd:Object">
                <fieldName>device_id</fieldName>
                <operator>iContains</operator>
                <value xsi:type="xsd:long">2</value>
              </elem>
              <elem xsi:type="xsd:Object">
                <fieldName>device_id</fieldName>
                <operator>iContains</operator>
                <value xsi:type="xsd:long">3</value>
              </elem>
            </criteria>
          </elem>
          <elem xsi:type="xsd:Object"></elem>
          <elem xsi:type="xsd:Object">
            <fieldName>ospf_id</fieldName>
            <operator>notNull</operator>
            <value xsi:type="xsd:boolean">true</value>
          </elem>
        </criteria>
        <__gwt_ObjectId xsi:type="xsd:long">6546</__gwt_ObjectId>
      </oldValues>
    </elem>
  </operations>
</transaction>

Response (should only be returning objects with device_id =2 and device_id = 3 but notice how any object that has a 2 or 3 in its device_id is returned):

[ {
	data : [ {
		id : 6,
		area : 1,
		wildcard : "0.0.0.3",
		lineStart : 234,
		ospf_id : 4,
		hostName : "bills_PE_12406_01",
		device_id : 2
	}, {
		id : 7,
		area : 1,
		wildcard : "0.0.0.3",
		lineStart : 393,
		ospf_id : 7,
		hostName : "bills_PE_12406_02",
		device_id : 3
	}, {
		id : 29,
		area : 1,
		wildcard : "0.0.0.3",
		lineStart : 400,
		ospf_id : 30,
		hostName : "jets_PE_12406_01",
		device_id : 12
	}, {
		id : 42,
		area : 1,
		wildcard : "0.0.0.3",
		lineStart : 356,
		ospf_id : 37,
		hostName : "jets_PE_12406_02",
		device_id : 13
	}, {
		id : 64,
		area : 1,
		wildcard : "0.0.0.3",
		lineStart : 374,
		ospf_id : 54,
		hostName : "packers_PE_12406_02",
		device_id : 20
	}, {
		.
		.
		.

[Isomorphic]

In the conversion to AdvancedCriteria, the operator to use is based on the textMatchStyle.

[chimpeenuts]

In the conversion to AdvancedCriteria, the operator to use is based on the textMatchStyle.

textMatchStyle of what? Your answer doesn't make any sense to me.

If I'm converting a basic Criteria object to an AdvancedCriteria object there is no textMatchStyle to set on the Criteria object or the AdvancedCriteria object.

Are you saying if i Set the textMatchStyle on the DSRequest of the fetch operation that uses the advancedCriteria, Smart GWT will 'change' the operator from 'icontains' to 'equals'? Because the conversion has already happened...I would think the icontains or equals would occur during the conversion.

Also, i could potentially understand a default iContains if I was passing an array of strings to the original Criteria object. But I'm passing numbers (in this case doubles). Seems equals would be a better default than icontains when dealing with numbers.

[Isomorphic]

To clarify, yes the server-side APIs for criteria conversion use textMatchStyle (as doc'd). The client-side API doesn't really have sufficient context to do the conversion - we might add textMatchStyle as a parameter to this API. In it's absence, while using the "equals" operator might be a better heuristic for int types, it's better to just not use this convenience API if you need specific operators.

[chimpeenuts]

To clarify, yes the server-side APIs for criteria conversion use textMatchStyle (as doc'd). The client-side API doesn't really have sufficient context to do the conversion - we might add textMatchStyle as a parameter to this API.

I'm not sure I understand this statement either. You say the client-side API doesn't have sufficient context to do the conversion, but it does. If a regular client-side Criteria object behaves like X and I convert that to an AdvancedCriteria object using asAdvancedCriteria() on the client and do NOTHING to that advanced object, it should still behave like X. That's not the case here. The simple conversion from Criteria --> AdvancedCriteria changes the criteria logic which to me seems like an issue. If this is the case, then the asAdvancedCriteria method shouldn't even exist since it's changing the intent of the criteria.

Also, I don't see what context the conversion code on the client needs. I have an array of Doubles that my ID must match. How does the context matter? If I did this with AdvancedCriteria I'd do something like ("id", OperatorId.IN_SET, Double[]) or I would join individual Criterions ("id", OperatorId.Equals, Double) with OperatorId.OR.

In it's absence, while using the "equals" operator might be a better heuristic for int types, it's better to just not use this convenience API if you need specific operators.

I have no issue with convenience API's. In fact, who doesn't love a good convenience API? But if it changes expected logic, then to me there is an issue.

So this whole issue arose because I need to combine a regular Criteria object with an AdvancedCriteria object and pass that to a datasource's fetchData method. The above method doesn't work b/c of the iContains issue.

This strategy throws a null pointer exception on the fetch call (Note c is a standard Criteria. additionalCrit is an AdvancedCriteria object).

protected Criteria getUpdateCriteria() {
	Criteria c = super.getUpdateCriteria();
	AdvancedCriteria finalCrit = new AdvancedCriteria();
	finalCrit.addCriteria(c);
	finalCrit.addCriteria(additionalCrit);
		
	return finalCrit;
}

This doesn't work either:

@Override
protected Criteria getUpdateCriteria() {
	Criteria c = super.getUpdateCriteria();
	Criteria c2 = new Criteria();
	c2.addCriteria(c);
	c2.addCriteria(additionalCrit);


	return c2;
}

This doesn't work either (throws JavaScriptException _4 is undefined):

@Override
protected Criteria getUpdateCriteria() {
	Criteria c = super.getUpdateCriteria();
		
        Criteria c2 = DataSource.combineCriteria(c, additionalCrit);

	return c2;
}

Is there a way to combine Criteria and AdvancedCriteria into a single AdvancedCriteria object or do I need to iterate through the Criteria object and somehow create AdvancedCriteria objects?

I also noticed that I can get all AdvancedCriteria criterions doing AdvancedCriteria.getCriterions(). I could then iterate through each criterion and do Criterion.setOperator(Operator.EQUALS) to <b>fix</b> the faulty conversion in asAdvancedCriteria and change the operator from iCONTAINS to EQUALS but this seems kludgy and fragile to causing issues down the road.

I've tried searching the forums and the only information I found on combing a Criteria and AdvancedCriteria object was:

You can combine simple Criteria and AdvancedCriteria with DataSource.combineCriteria(). But the result will always be AdvancedCriteria of course.

In my JSON print out, it looks like it's correctly formatted:

{
    "operator":"and", 
    "_constructor":"AdvancedCriteria", 
    "criteria":[
        {
            "_constructor":"AdvancedCriteria", 
            "operator":"and", 
            "criteria":[
                {
                    "fieldName":"ospf_id", 
                    "operator":"notNull", 
                    "value":true
                }
            ]
        }, 
        {
            "_constructor":"AdvancedCriteria", 
            "device_id":[
                55
            ]
        }
    ]
}

but I tried that above and I get the error below. Here is my code calling fetch:

public void updateGrid() {
	if (isVisible() && isDrawn()) {
		Criteria c = getUpdateCriteria();
		table.fetchData(c);
	}
}

Error message I get:

Caused by: com.google.gwt.core.client.JavaScriptException: (TypeError): _4 is undefined
	at com.google.gwt.dev.shell.BrowserChannelServer.invokeJavascript(BrowserChannelServer.java:248)
	at com.google.gwt.dev.shell.ModuleSpaceOOPHM.doInvoke(ModuleSpaceOOPHM.java:136)
	at com.google.gwt.dev.shell.ModuleSpace.invokeNative(ModuleSpace.java:561)
	at com.google.gwt.dev.shell.ModuleSpace.invokeNativeVoid(ModuleSpace.java:289)
	at com.google.gwt.dev.shell.JavaScriptHost.invokeNativeVoid(JavaScriptHost.java:107)
	at com.smartgwt.client.widgets.grid.ListGrid.fetchData(ListGrid.java)
	at com.bpt.snap.ui.client.view.GridLayoutBase.updateGrid(GridLayoutBase.java:80)
	at com.bpt.snap.ui.client.view.SelectedHostsDependentGridLayout$2.hostSelectionChanged(SelectedHostsDependentGridLayout.java:65)
	at com.bpt.snap.ui.client.events.EventBroker.fireHostSelectionChangedEvent(EventBroker.java:159)
	at com.bpt.snap.ui.client.events.EventBroker.setSelectedHosts(EventBroker.java:66)
	at com.bpt.snap.ui.client.view.main.HostsView.updatedSelectedRecords(HostsView.java:266)
	at com.bpt.snap.ui.client.view.main.HostsView.access$7(HostsView.java:252)
	at com.bpt.snap.ui.client.view.main.HostsView$6.onRecordClick(HostsView.java:194)
	at com.smartgwt.client.widgets.grid.events.RecordClickEvent.dispatch(RecordClickEvent.java:97)
	at com.smartgwt.client.widgets.grid.events.RecordClickEvent.dispatch(RecordClickEvent.java:1)
	at com.google.gwt.event.shared.GwtEvent.dispatch(GwtEvent.java:1)
	at com.google.web.bindery.event.shared.EventBus.dispatchEvent(EventBus.java:40)
	at com.google.web.bindery.event.shared.SimpleEventBus.doFire(SimpleEventBus.java:193)
	at com.google.web.bindery.event.shared.SimpleEventBus.fireEvent(SimpleEventBus.java:88)
	at com.google.gwt.event.shared.HandlerManager.fireEvent(HandlerManager.java:127)
	... 26 more

[chimpeenuts]

Follow up -

So I wanted to check to see if the method asAdvancedCriteria always defaults to using iContains...and it doesn't.

If you create a Criteria object with a single value (not an array) and call asAdvancedCriteria, that convenience API method actually knows to use EQUALS:

Criteria c = new Criteria();
c.addCriteria("id", 1);
AdvancedCriteria c2 = c.asAdvancedCriteria();
System.out.println(JSON.encode(c2.getJsObj()));

Here is the output:

{
    "_constructor":"AdvancedCriteria", 
    "operator":"and", 
    "criteria":[
        {
            "fieldName":"device_id", 
            "operator":"equals", 
            "value":1
        }
    ]
}

So why would the code inside asAdvancedCriteria change from using EQUALS to ICONTAINS when an array is being used as input to a Criteria object? It correctly converts a single numeric value to using EQUALS...but it decides for an array of numeric values I'm going to use ICONTAINS? How does that make sense?

[Isomorphic]

Criteria are always evaluated in the context of a TextMatchStyle, so converting from Criteria to AdvancedCriteria without a TextMatchStyle is ambiguous - this is very simple, not sure why you don't follow.

Anyway you did point out an issue where array-valued criteria weren't converted correctly, this has been fixed and the fix will be in the next 3.0p and 3.1d nightlies.

We didn't follow the context for your getUpdateCriteria() method or how you're getting an NPE. If you still think there's an issue here, please post as a minimal test case.