Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL error when processing messages for Direct addresses that include apostrophes #260

Open
phillipodam opened this issue May 13, 2016 · 1 comment

Comments

@phillipodam
Copy link

phillipodam commented May 13, 2016

The method org.nhindirect.config.store.dao.impl.AddressDaoImpl#listAddresses(java.util.List, org.nhindirect.config.store.EntityStatus) will create an a malformed SQL statement if one of the Direct addresses provided in the list includes an apostrophe.

Here's the existing method

/* 
 * (non-Javadoc)
 * 
 * @see org.nhindirect.config.store.dao.AddressDao#listAddresses(java.util.List, org.nhindirect.config.store.EntityStatus)
 */
@SuppressWarnings("unchecked")
@Transactional(readOnly = true)
public List<Address> listAddresses(List<String> names, EntityStatus status) {
    if (log.isDebugEnabled())
        log.debug("Enter");

    List<Address> result = null;
    Query select = null;
    if (names != null) {
        StringBuffer nameList = new StringBuffer("(");
        for (String aName : names) {
            if (nameList.length() > 1) {
                nameList.append(", ");
            }
            nameList.append("'").append(aName.toUpperCase(Locale.getDefault())).append("'");
        }
        nameList.append(")");
        String query = "SELECT a from Address a WHERE UPPER(a.emailAddress) IN " + nameList.toString();

        if (status != null) {
            select = entityManager.createQuery(query + " AND a.status = ?1");
            select.setParameter(1, status);
        } else {
            select = entityManager.createQuery(query);
        }
    } else {
        if (status != null) {
            select = entityManager.createQuery("SELECT a from Address a WHERE a.status = ?1");
            select.setParameter(1, status);
        } else {
            select = entityManager.createQuery("SELECT a from Address a");
        }

    }

    @SuppressWarnings("rawtypes")
    List rs = select.getResultList();
    if ((rs.size() != 0) && (rs.get(0) instanceof Address)) {
        result = (List<Address>) rs;
    } else {
        result = new ArrayList<Address>();
    }

    if (log.isDebugEnabled())
        log.debug("Exit");
    return result;
}

Here's a possible replacement method

/* 
 * (non-Javadoc)
 * 
 * @see org.nhindirect.config.store.dao.AddressDao#listAddresses(java.util.List, org.nhindirect.config.store.EntityStatus)
 */
@SuppressWarnings("unchecked")
@Transactional(readOnly = true)
public List<Address> listAddresses(List<String> names, EntityStatus status) {
    if (log.isDebugEnabled())
        log.debug("Enter");

    List<Address> result = null;
    Query select = null;
    if (names != null) {
        String query = "SELECT a from Address a WHERE a.emailAddress IN (?1)";

        if (status != null) {
            select = entityManager.createQuery(query + " AND a.status = ?2");
            select.setParameter(2, status);
        } else {
            select = entityManager.createQuery(query);
        }
    select.setParameter(1, names);
    } else {
        if (status != null) {
            select = entityManager.createQuery("SELECT a from Address a WHERE a.status = ?1");
            select.setParameter(1, status);
        } else {
            select = entityManager.createQuery("SELECT a from Address a");
        }
    }

    @SuppressWarnings("rawtypes")
    List rs = select.getResultList();
    if ((rs.size() != 0) && (rs.get(0) instanceof Address)) {
        result = (List<Address>) rs;
    } else {
        result = new ArrayList<Address>();
    }

    if (log.isDebugEnabled())
        log.debug("Exit");
    return result;
}

The IN clause (in MySQL at least) appears to be case insensitive so there may be no need to worry about converting everything to the same case, of course this may not be the case with MSSQL, Oracle, PostgreSQL etc. So iterating through the names list and and ensuring a common case is an option... for example

/* 
 * (non-Javadoc)
 * 
 * @see org.nhindirect.config.store.dao.AddressDao#listAddresses(java.util.List, org.nhindirect.config.store.EntityStatus)
 */
@SuppressWarnings("unchecked")
@Transactional(readOnly = true)
public List<Address> listAddresses(List<String> names, EntityStatus status) {
    if (log.isDebugEnabled())
        log.debug("Enter");

    List<Address> result = null;
    Query select = null;
    if (names != null) {
        List<String> upperNames = new ArrayList(names.size());
        for (String aName : names) {
            upperNames.add(aName.toUpperCase(Locale.getDefault()));
        }
        String query = "SELECT a from Address a WHERE UPPER(a.emailAddress) IN (?1)";

        if (status != null) {
            select = entityManager.createQuery(query + " AND a.status = ?2");
            select.setParameter(2, status);
        } else {
            select = entityManager.createQuery(query);
        }
        select.setParameter(1, upperNames);
    } else {
        if (status != null) {
            select = entityManager.createQuery("SELECT a from Address a WHERE a.status = ?1");
            select.setParameter(1, status);
        } else {
            select = entityManager.createQuery("SELECT a from Address a");
        }

    }

    @SuppressWarnings("rawtypes")
    List rs = select.getResultList();
    if ((rs.size() != 0) && (rs.get(0) instanceof Address)) {
        result = (List<Address>) rs;
    } else {
        result = new ArrayList<Address>();
    }

    if (log.isDebugEnabled())
        log.debug("Exit");
    return result;
}
@phillipodam
Copy link
Author

The following methods similarly build up SQL that has the potential to result in an invalid statement. Some cases definitely won't have a problem since the values being concatenated are numeric. However there's no reason I can see to remain with this style of query building anywhere in the codebase since JPA appears to be quite readily accept a list as a parameter value.

org.nhindirect.config.store.dao.impl.AnchorDaoImpl#list(java.util.List)
org.nhindirect.config.store.dao.impl.AnchorDaoImpl#listByIds(java.util.List)
org.nhindirect.config.store.dao.impl.AnchorDaoImpl#delete(java.util.List)
org.nhindirect.config.store.dao.impl.CertificateDaoImpl#delete(java.util.List)
org.nhindirect.config.store.dao.impl.DNSDaoImpl#get(long[])
org.nhindirect.config.store.dao.impl.DNSDaoImpl#remove(long[])
org.nhindirect.config.store.dao.impl.DomainDaoImpl#getDomains(java.util.List, org.nhindirect.config.store.EntityStatus)
org.nhindirect.config.store.dao.impl.SettingDaoImpl#delete(java.util.Collection)
org.nhindirect.config.store.dao.impl.SettingDaoImpl#getByNames(java.util.Collection)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant