CODEWITHSUNDEEP
javasqljooq
DanO
DanO

Reputation: 193

How Do You Check For DELETE/UPDATE Without A WHERE Clause

I currently have a listener that we use to do a few different monitoring-type activities (like log a warning if a query takes more than 5 seconds), but it also watches for and kills "silly bugs" -- especially UPDATE and DELETE queries that are missing a WHERE clause.

In the past we did the following (note that we are using com.foundationdb.sql):

/**
 * Hook into the query execution lifecycle before rendering queries. We are checking for silly mistakes,
 * pure SQL, etc.
 */
@Override
public void renderStart(final @NotNull ExecuteContext ctx) {
    if (ctx.type() != ExecuteType.WRITE)
        return;

    String queryString = ctx.sql();
    try (final Query query = ctx.query()) {

        // Is our Query object empty? If not, let's run through it
        if (!ValidationUtils.isEmpty(query)) {
            queryString = query.getSQL(ParamType.INLINED);

            final SQLParser parser = new SQLParser();
            try {
                final StatementNode tokens = parser.parseStatement(query.getSQL());
                final Method method = tokens.getClass().getDeclaredMethod("getStatementType");
                method.setAccessible(true);
                switch (((Integer) method.invoke(tokens)).intValue()) {                 
                    case StatementType.UPDATE:
                        SelectNode snode = ConversionUtils.as(SelectNode.class,
                                ((DMLStatementNode) tokens).getResultSetNode());

                        // check if we are a mass delete/update (which we don't allow)
                        if ((Objects.isNull(snode)) || (Objects.isNull(snode.getWhereClause())))
                            throw new RuntimeException("A mass update has been detected (and prevented): "
                                    + DatabaseManager.getBuilder().renderInlined(ctx.query()));
                        break;
                    case StatementType.DELETE:
                        snode = ConversionUtils.as(SelectNode.class,
                                ((DMLStatementNode) tokens).getResultSetNode());

                        // check if we are a mass delete/update (which we don't allow)
                        if ((Objects.isNull(snode)) || (Objects.isNull(snode.getWhereClause())))
                            throw new RuntimeException("A mass delete has been detected (and prevented): "
                                    + DatabaseManager.getBuilder().renderInlined(ctx.query()));
                        break;
                    default:
                        if (__logger.isDebugEnabled()) {
                            __logger
                                    .debug("Skipping query because we don't need to do anything with it :-): {}", queryString);
                        }
                }
            } catch (@NotNull StandardException | IllegalAccessException
                    | IllegalArgumentException | InvocationTargetException | NoSuchMethodException
                    | SecurityException e) {
                // logger.error(e.getMessage(), e);
            }
        }
        // If the query object is empty AND the SQL string is empty, there's something wrong
        else if (ValidationUtils.isEmpty(queryString)) {
            __logger.error(
                    "The ctx.sql and ctx.query.getSQL were empty");
        } else
            throw new RuntimeException(
                    "Someone is trying to send pure SQL queries... we don't allow that anymore (use jOOQ): "
                            + queryString);
    }
}

I really don't want to use yet another tool -- especially since most SQL parsers can't handle UPSERTs or the wide variety of queries that jOOQ can, so a lot just get cut out -- and would love to use jOOQ's constructs, but I'm having trouble. Ideally I could just check the query class and if it's an Update or Delete (or a subclass), I would just scream if it isn't an instance of UpdateConditionStep or DeleteConditionStep, but that doesn't work because the queries are coming back as UpdateQueryImpl... and without crazy reflection, I can't see if there is a condition in use.

So... right now I'm doing:

/**
 * Hook into the query execution lifecycle before rendering queries. We are checking for silly mistakes, pure SQL,
 * etc.
 */
@Override
public void renderStart(final @NotNull ExecuteContext ctx) {
    if (ctx.type() != ExecuteType.WRITE)
        return;

    try (final Query query = ctx.query()) {
        // Is our Query object empty? If not, let's run through it
        if (!ValidationUtils.isEmpty(query)) {
            // Get rid of nulls
            query.getParams().entrySet().stream().filter(entry -> Objects.nonNull(entry.getValue()))
                    .filter(entry -> CharSequence.class.isAssignableFrom(entry.getValue().getDataType().getType()))
                    .filter(entry -> NULL_CHARACTER.matcher((CharSequence) entry.getValue().getValue()).find())
                    .forEach(entry -> query.bind(entry.getKey(),
                            NULL_CHARACTER.matcher((CharSequence) entry.getValue().getValue()).replaceAll("")));

            if (Update.class.isInstance(query)) {
                if (!UpdateConditionStep.class.isInstance(query)) {
                    if (!WHERE_CLAUSE.matcher(query.getSQL(ParamType.INDEXED)).find()) {
                        final String queryString = query.getSQL(ParamType.INLINED);
                        throw new RuntimeException(
                                "Someone is trying to run an UPDATE query without a WHERE clause: " + queryString);
                    }
                }
            } else if (Delete.class.isInstance(query)) {
                if (!DeleteConditionStep.class.isInstance(query)) {
                    if (!WHERE_CLAUSE.matcher(query.getSQL(ParamType.INDEXED)).find()) {
                        final String queryString = query.getSQL(ParamType.INLINED);
                        throw new RuntimeException(
                                "Someone is trying to run a DELETE query without a WHERE clause: " + queryString);
                    }
                }
            }
        } else
            throw new RuntimeException(
                    "Someone is trying to send pure SQL queries... we don't allow that anymore (use jOOQ): "
                            + ctx.sql());
    }
}

This let's me get rid of the third party SQL parser, but now I'm using a regular expression on the non-inlined query looking for \\s[wW][hH][eE][rR][eE]\\s, which isn't ideal, either.

  1. Is there a way to use jOOQ to tell me if an UPDATE, DELETE, has a WHERE clause?
  2. Similarly, is there a way that let's me see what table the query is acting against (so that I can limit the tables someone can perform mutable actions against -- obviously that one wouldn't check if it's UPDATE or DELETE, instead using the ExecuteType)?

Upvotes: 1

Views: 440

Answers (1)

Lukas Eder
Lukas Eder

Reputation: 220762

That's an interesting idea and approach. One problem I can see with it is performance. Rendering the SQL string a second time and then parsing it again sounds like a bit of overhead. Perhaps, this ExecuteListener should be active in development and integration test environments only, not in production.

Regarding your questions

  1. Is there a way to use jOOQ to tell me if an UPDATE, DELETE, has a WHERE clause?

Since you seem to be open to use reflection to access a third party library's internals, well of course, you could check if the ctx.query() is of type org.jooq.impl.UpdateQueryImpl or org.jooq.impl.DeleteQueryImpl. In version 3.10.1, both of them have a private condition member, which you could check.

This will obviously break any time the internals are changed, but it might be a pragmatic solution for now.

  1. Similarly, is there a way that let's me see what table the query is acting against

A more general and more robust approach would be to implement a VisitListener, which is jOOQ's callback that is called during expression tree traversal. You can hook into the generation of the SQL string and the collection of bind variables, and throw your errors as soon as you encounter:

  • An UPDATE or DELETE statement
  • ... without a WHERE clause
  • ... updating a table from a specific set of tables

You "just" have to implement a stack machine that remembers all of the above things prior to throwing the exception. An example of how VisitListener can be implemented is given here: https://blog.jooq.org/2015/06/17/implementing-client-side-row-level-security-with-jooq

New feature in the future

This kind of feature has been discussed a couple of times on the mailing list as well. It's a low hanging fruit to support by jOOQ natively. I've created a feature request for jOOQ 3.11, for this: https://github.com/jOOQ/jOOQ/issues/6771

Upvotes: 1

Related Questions