Dangerous setAllowMultiUpdate(false) behavior with primaryKey field criteria other than equals

[Blama]

Hi Isomorphic,

using current 6.1p (v11.1p_2017-11-16) it seems that setAllowMultiUpdate(false) is not behaving as documented.
It seems that almost always it is enough to just have primaryKey-field criteria, regardless of the operator.
The docs state:

Parameters:
newValue - true to allow multi-updates; false to restrict updates so that they [B]can affect just one single record[/B], identified by primaryKey values in the record provided

This means, only requests with top-level equals-criteria should be allowed.

Please see this testcase (no actual delete is done, safe to run), which only disallows GreaterThanField, but allows GreaterOrEqual, EndsWith, NotEqual, NotNull.

DeleteTest.java (register in web.xml):

package com.smartgwt.sample.server.listener;

import java.io.IOException;
import java.io.PrintWriter;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import com.isomorphic.criteria.DefaultOperators;
import com.isomorphic.datasource.DSRequest;
import com.isomorphic.datasource.DataSource;

public class DeleteTest extends HttpServlet {
    private static final long serialVersionUID = -536025913450167992L;

    @Override
    public void doGet(HttpServletRequest servletRequest, HttpServletResponse servletResponse) throws ServletException, IOException {
        PrintWriter pw = servletResponse.getWriter();
        try {
            DSRequest deleteReq = new DSRequest("supplyItem", DataSource.OP_REMOVE);
            deleteReq.setClientRequest(true);
            deleteReq.setAllowMultiUpdate(false);
            deleteReq.addToCriteria("itemID", DefaultOperators.GreaterOrEqual, 10000000);
            deleteReq.addToCriteria("SKU", DefaultOperators.Equals, "blockDelete");
            deleteReq.execute();
        } catch (Exception e) {
            pw.append("Failed GreaterOrEqual\r\n");
        }
        try {
            DSRequest deleteReq = new DSRequest("supplyItem", DataSource.OP_REMOVE);
            deleteReq.setClientRequest(true);
            deleteReq.setAllowMultiUpdate(false);
            deleteReq.addToCriteria("itemID", DefaultOperators.GreaterThanField, "itemID");
            deleteReq.addToCriteria("SKU", DefaultOperators.Equals, "blockDelete");
            deleteReq.execute();
        } catch (Exception e) {
            pw.append("Failed GreaterThanField\r\n");
        }
        try {
            DSRequest deleteReq = new DSRequest("supplyItem", DataSource.OP_REMOVE);
            deleteReq.setClientRequest(true);
            deleteReq.setAllowMultiUpdate(false);
            deleteReq.addToCriteria("itemID", DefaultOperators.EndsWith, "0");
            deleteReq.addToCriteria("SKU", DefaultOperators.Equals, "blockDelete");
            deleteReq.execute();
        } catch (Exception e) {
            pw.append("Failed EndsWith\r\n");
        }
        try {
            DSRequest deleteReq = new DSRequest("supplyItem", DataSource.OP_REMOVE);
            deleteReq.setClientRequest(true);
            deleteReq.setAllowMultiUpdate(false);
            deleteReq.addToCriteria("itemID", DefaultOperators.NotEqual, "123456");
            deleteReq.addToCriteria("SKU", DefaultOperators.Equals, "blockDelete");
            deleteReq.execute();
        } catch (Exception e) {
            pw.append("Failed NotEqual\r\n");
        }
        try {
            DSRequest deleteReq = new DSRequest("supplyItem", DataSource.OP_REMOVE);
            deleteReq.setClientRequest(true);
            deleteReq.setAllowMultiUpdate(false);
            deleteReq.addToCriteria("itemID", DefaultOperators.NotNull);
            deleteReq.addToCriteria("SKU", DefaultOperators.Equals, "blockDelete");
            deleteReq.execute();
        } catch (Exception e) {
            pw.append("Failed NotNull\r\n");
        }
        try {
            DSRequest deleteReq = new DSRequest("supplyItem", DataSource.OP_REMOVE);
            deleteReq.setClientRequest(true);
            deleteReq.setAllowMultiUpdate(false);
            deleteReq.addToCriteria("itemID", DefaultOperators.Equals, 10000000);
            deleteReq.addToCriteria("SKU", DefaultOperators.Equals, "blockDelete");
            deleteReq.execute();
        } catch (Exception e) {
            pw.append("Failed Equals - not expected\r\n");
        }
    }
}

I'd expect all tests but the last one to fail.

Also, IMHO the docs for the default behavior are contradictory here, see the two bold parts.
the docs for defaultMultiUpdatePolicy:

Default value of null means this DataSource will use the system-wide default, which is set via datasources.defaultMultiUpdatePolicy in server.properties, and defaults to allowing multi updates for requests associated with an RPCManager, see MultiUpdatePolicy for details.

(This is from framework.properties:)

# if allowMultiUpdate is not set, having a PK is required for requests associated with an RPCManager
datasources.defaultMultiUpdatePolicy: rpcManager

These are the docs for enum MultiUpdatePolicy.RPCMANAGER:

having a PK is required for any request that is associated with an RPCManager, which includes clientRequests and server-created DSRequests where an RPCManager was explicitly provided

Best regards
Blama

[Isomorphic]

This is fixed and will be available in nightly builds since Dec 8 (today).

Note that "NotNull" test case of yours is not quite accurate, it uses addToCriteria(fieldName, value) signature providing NotNull operator as value, which results in unexpected behaviour. Please see example below using addToCriteria(Criterion) API instead:

deleteReq.addToCriteria(new NotNullCriterion("itemID"));

[Blama]

Hi Isomorphic,

I can see that the tests now succeed after changing the one line as you suggested and using v11.1p_2017-12-27 (5 deletes blocked, 1 SQL issued sucessfully).

Please note though that the one SQL issued is

DELETE FROM supplyItem WHERE ((supplyItem.itemID = 10000000 AND supplyItem.itemID IS NOT NULL))

and does not contain any criteria for SKU. The code is:

            DSRequest deleteReq = new DSRequest("supplyItem", DataSource.OP_REMOVE);
            deleteReq.setClientRequest(true);
            deleteReq.setAllowMultiUpdate(false);
            deleteReq.addToCriteria("itemID", DefaultOperators.Equals, 10000000);
            deleteReq.addToCriteria("SKU", DefaultOperators.Equals, "blockDelete");
            deleteReq.execute();

Changing the order of the addToCriteria() does not change anything here. While this is only a testcase right now, this is again an important one, as an unconstrained (an potentially unnoticed for some time, if the change comes with a nightly) SQL-DELETE is among the worst things that can happen for me.

Also, the two bold parts in the docs are still contradictory IMHO.

Best regards
Blama

[Blama]

Hi Isomorphic,

it gets worse using OR Criteria, where I can even get an unbounded delete (WHERE '1'='1').
Please see this revised and easy to extend testcase. At the end there are two tests that should fail (still only one delete should succeed).
This might be related to this old thread, where also an unbounded delete was happening.

web.xml addition:

    <servlet>
        <servlet-name>DeleteTest</servlet-name>
        <servlet-class>com.smartgwt.sample.server.listener.DeleteTest</servlet-class>
    </servlet>
    <servlet-mapping>
        <servlet-name>DeleteTest</servlet-name>
        <url-pattern>/DeleteTest</url-pattern>
    </servlet-mapping>

DeleteTest.java:

package com.smartgwt.sample.server.listener;

import java.io.IOException;
import java.io.PrintWriter;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import com.isomorphic.criteria.Criterion;
import com.isomorphic.criteria.DefaultOperators;
import com.isomorphic.criteria.criterion.AndCriterion;
import com.isomorphic.criteria.criterion.NotNullCriterion;
import com.isomorphic.criteria.criterion.OrCriterion;
import com.isomorphic.criteria.criterion.SimpleCriterion;
import com.isomorphic.datasource.DSRequest;
import com.isomorphic.datasource.DSResponse;
import com.isomorphic.datasource.DataSource;

public class DeleteTest extends HttpServlet {
    private static final long serialVersionUID = -536025913450167992L;

    @Override
    public void doGet(HttpServletRequest servletRequest, HttpServletResponse servletResponse) throws ServletException, IOException {
        PrintWriter pw = servletResponse.getWriter();
        doDelete(pw, "GreaterOrEqual", true, new SimpleCriterion("itemID", DefaultOperators.GreaterOrEqual, 10000000),
                new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"));

        doDelete(pw, "GreaterThanField", true, new SimpleCriterion("itemID", DefaultOperators.GreaterThanField, "itemID"),
                new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"));

        doDelete(pw, "EndsWith", true, new SimpleCriterion("itemID", DefaultOperators.EndsWith, "0"),
                new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"));

        doDelete(pw, "NotEqual", true, new SimpleCriterion("itemID", DefaultOperators.NotEqual, "123456"),
                new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"));

        doDelete(pw, "NotNull", true, new NotNullCriterion("itemID"), new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"));

        doDelete(pw, "Equals", false, new SimpleCriterion("itemID", DefaultOperators.Equals, "10000000"),
                new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"));

        doDelete(pw, "AND with PK", false, new AndCriterion(new Criterion[] { new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"),
                new SimpleCriterion("itemID", DefaultOperators.Equals, 10000000) }));

        doDelete(pw, "AND without PK", true, new AndCriterion(new Criterion[] { new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"),
                new SimpleCriterion("SKU", DefaultOperators.Equals, 10000000) }));

        doDelete(pw, "OR with PK", true, new OrCriterion(new Criterion[] { new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"),
                new SimpleCriterion("itemID", DefaultOperators.Equals, 10000000) }));

        doDelete(pw, "OR without PK", true, new OrCriterion(new Criterion[] { new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"),
                new SimpleCriterion("SKU", DefaultOperators.Equals, 10000000) }));
    }

    private void doDelete(PrintWriter pw, String testName, boolean expectedToFail, Criterion... advancedCriteria) {
        try {
            doDelete(advancedCriteria);
        } catch (Exception e) {
            pw.append(testName + " failed - " + (expectedToFail ? "expected" : "UNEXPECTED") + "\r\n");
            return;
        }
        pw.append(testName + " succeeded - " + (expectedToFail ? "UNEXPECTED" : "expected") + "\r\n");
    }

    private DSResponse doDelete(Criterion... advancedCriteria) throws Exception {
        DSRequest deleteReq = new DSRequest("supplyItem", DataSource.OP_REMOVE);
        deleteReq.setClientRequest(true);
        deleteReq.setAllowMultiUpdate(false);
        for (Criterion ac : advancedCriteria)
            deleteReq.addToCriteria(ac);
        return deleteReq.execute();
    }
}

Best regards
Blama

[Blama]

Hi Isomorphic,

I have a suggestion to remedy the consequences of this issue in case of bugs/regressions:

Normally you have ANY_CHANGE as TransactionPolicy and basically all RDBMS support transactions.
Also, I assume that you can get back affectedRows for all statements sent to the DB via JDBC.

With this together you could build a check (possibly only optional, as someone might have a reason to disable this).

If all of the following are true:

• Request with setAllowMultiUpdate(false) (either implicit or explicit)
• TransactionPolicy not NONE (will almost always be the case)
• affectedRows available
• affectedRows after execution >=2 (because something did not work)

then return some kind of error and rollback the Transaction.
This way, if there is ever again a bug in this area, the result is an application failure and not a data loss.

What do you think?

Best regards
Blama

[Isomorphic]

Sorry for the bug with unbound OR, it was fixed immediately in a quick way.

Please consider downloading Jan 19 nightly build for 11.1 to get:
1. final changes to prevent the unbound delete bug
2. fixed bug with SKU criteria missing in actual SQL
3. fixed multiUpdate related docs

Check you are suggesting seems reasonable, but we won't implement it anytime soon.

[Blama]

Hi Isomorphic,

I can that all the testcases work as expected in v11.1p_2018-01-23. My application is almost only insert and update-related with mostly single-row-requests (or many such requests queued), but for the few deletes it's good to be sure that the framework it is working as designed.

I'd be happy to see the suggestion live at some point, but this is not very important for me, if the rest is working as expected. It's only a double safety net.

Thank you & Best regards
Blama

[Blama]

Hi Isomorphic,

please note that most likely the change from #6 (most likely "fixed bug with SKU criteria missing in actual SQL") did break something else, see here.

Best regards
Blama

[Blama]

Hi Isomorphic,

actually, I think your change did change the default behavior of many applications.

Before, are client side triggered delete looked like this:

DELETE FROM table WHERE primaryKeyField = primaryKeyValue

Now it includes all the current state from the client, making the DELETE target 0 rows if some other user changed the row before. Effectively this is now Optimistic locking, while it was no locking before.

I'm not sure this is a change that should be done without announcement. If there is only data in the request, the delete should be PK-only. If there is also criteria, those should be used for the delete.

Updates still work as before IMHO, because there there is always criteria in the request when editing in the GUI.

Best regards
Blama

[Isomorphic]

We agree this is a serious problem and you're are right - its caused by changes addressing this thread. Related changes are reverted and will be available for download in nightly builds since Feb 2 (tomorrow). Sorry for the inconvenience.

[Blama]

Hi Isomorphic,

I forgot to retest this, as it was working in my application again. Using v11.1p_2018-04-07 this is behaving as before again, but these two testcases, which should include criteria for SKU, don't show those criteria in the generated SQL.

        doDelete(pw, "Equals", false, new SimpleCriterion("itemID", DefaultOperators.Equals, "10000000"),
                new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"));

        doDelete(pw, "AND with PK", false, new AndCriterion(new Criterion[] { new SimpleCriterion("SKU", DefaultOperators.Equals, "blockDel"),
                new SimpleCriterion("itemID", DefaultOperators.Equals, 10000000) }));

This means the fix you did in #6.2, which then caused the issue in #8 is only reverted and not repaired. This issue is still open!

Best regards
Blama

[Isomorphic]

As you pointed out previously, allowing criteria other than the PK would be a behavior change, so we restored the old behavior of trimming to just the PK.

If you want to have criteria in addition to the PK you can get allowMultiUpdate:true to do that.

[Blama]

Hi Isomorphic,

yes and no. The testcase is server code - here IMHO it should happen what the developer programs - or at least a warning should be displayed in the server log.
For client requests my suggestion was:

If there is only data in the request, the delete should be PK-only. If there is also criteria, those should be used for the delete.

Updates still work as before IMHO, because there there is always criteria in the request when editing in the GUI.

It will be perceived as an error if the dev adds criteria and those are silently discarded - at least you need to warn, as this is very confusing if you hit it and also the docs do not mention it.
And actually I'm not sure the docs are the correct place for this, as allowMultiUpdate:false is the default and one does not think of looking there, if he or she hits the behavior of discarded criteria.

I did not test updates now, but here it should be the same.

Best regards
Blama

[Isomorphic]

We understood the suggestion, but again this would be a behavior change, and even server-initiated requests are often created from client-submitted data. We do plan to document this behavior under allowMultiUpdate:true, which we agree is not necessarily that easy to find, however, this is a very rare use case, and with the SQL being logged, it will be clear enough that criteria are being dropped, and the docs will be phrased so as to make the relevant section easy to find via search.