Implemented withCloseable/try-with-resources where needed in the code… (

#625)

* Implemented withCloseable/try-with-resources where needed in the code to ensure proper closure of the entity list iterator resource

* Fixed withCloseable syntax

---------

Co-authored-by: David E. Jones <[email protected]>

framework/service/org/moqui/impl/EntitySyncServices.xml

@@ -236,13 +236,10 @@ along with this software (see the LICENSE.md file). If not, see
  // ec.logger.warn("=========== get#EntitySyncData entityName=${entryMap.entityName} count=${currentCount} find=${find}")
 
  if (currentCount > 0) {
- EntityListIterator resultEli = find.iterator()
- try {
+ find.iterator().withCloseable ({resultEli ->
  int levels = entryMap.dependents ? 2 : 0
  resultEli.writeXmlText((Writer) entityWriter, null, levels)
- } finally {
- resultEli.close()
- }
+ })
  }
  }
 

framework/src/main/groovy/org/moqui/impl/context/NotificationMessageImpl.groovy

@@ -20,10 +20,8 @@ import org.moqui.BaseArtifactException
 import org.moqui.Moqui
 import org.moqui.context.ExecutionContext
 import org.moqui.context.NotificationMessage
-import org.moqui.context.NotificationMessage.NotificationType
 import org.moqui.entity.EntityFacade
 import org.moqui.entity.EntityList
-import org.moqui.entity.EntityListIterator
 import org.moqui.entity.EntityValue
 import org.slf4j.Logger
 import org.slf4j.LoggerFactory
@@ -93,20 +91,17 @@ class NotificationMessageImpl implements NotificationMessage, Externalizable {
 
  // notify by group, skipping users already notified
  if (userGroupId) {
- EntityListIterator eli = ef.find("moqui.security.UserGroupMember")
+ ef.find("moqui.security.UserGroupMember")
  .conditionDate("fromDate", "thruDate", new Timestamp(System.currentTimeMillis()))
- .condition("userGroupId", userGroupId).disableAuthz().iterator()
- try {
+ .condition("userGroupId", userGroupId).disableAuthz().iterator().withCloseable ({eli ->
  EntityValue nextValue
  while ((nextValue = (EntityValue) eli.next()) != null) {
  String userId = (String) nextValue.userId
  if (checkedUserIds.contains(userId)) continue
  checkedUserIds.add(userId)
  if (checkUserNotify(userId, ef)) notifyUserIds.add(userId)
  }
- } finally {
- eli.close();
- }
+ })
  }
 
  // add all users subscribed to all messages on the topic

framework/src/main/groovy/org/moqui/impl/entity/EntityDataDocument.groovy

@@ -301,8 +301,7 @@ class EntityDataDocument {
  // do the one big query
  String lastDocId = null
  int docCount = 0
- EntityListIterator mainEli = mainFind.iterator()
- try {
+ try (EntityListIterator mainEli = mainFind.iterator()) {
  logger.info("Feed dataDocumentId ${dataDocumentId} query complete (cursor opened) in ${System.currentTimeMillis() - startTimeMillis}ms")
  EntityValue ev
  while ((ev = (EntityValue) mainEli.next()) != null) {
@@ -345,7 +344,6 @@ class EntityDataDocument {
  efi.ecfi.serviceFacade.sync().name(feedReceiveServiceName).parameter("documentList", documentMapList).call()
  }
  } finally {
- mainEli.close()
  logger.info("Feed dataDocumentId ${dataDocumentId} feed complete and cursor closed in ${System.currentTimeMillis() - startTimeMillis}ms")
  }
 
@@ -366,16 +364,14 @@ class EntityDataDocument {
  ArrayList<Map> documentMapList = ddi.hasAllPrimaryPks ? null : new ArrayList<Map>()
 
  // do the one big query
- EntityListIterator mainEli = mainFind.iterator()
- try {
+
+ mainFind.iterator().withCloseable ({mainEli->
  EntityValue ev
  while ((ev = (EntityValue) mainEli.next()) != null) {
  // logger.warn("=========== DataDocument query result for ${dataDocumentId}: ${ev}")
  mergeValueToDocMap(ev, ddi, documentMapMap, documentMapList, docTsString)
  }
- } finally {
- mainEli.close()
- }
+ })
 
  // make the actual list and return it
  if (ddi.hasAllPrimaryPks) {

framework/src/main/groovy/org/moqui/impl/entity/EntityDataWriterImpl.groovy

@@ -170,9 +170,9 @@ class EntityDataWriterImpl implements EntityDataWriter {
  EntityDefinition ed = efi.getEntityDefinition(en)
  boolean useMaster = masterName != null && masterName.length() > 0 && ed.getMasterDefinition(masterName) != null
  EntityFind ef = makeEntityFind(en)
- EntityListIterator eli = ef.iterator()
 
- try {
+
+ try (EntityListIterator eli = ef.iterator()) {
  if (!eli.hasNext()) continue
 
  String filename = path + '/' + en + '.' + fileType.name().toLowerCase()
@@ -200,8 +200,6 @@ class EntityDataWriterImpl implements EntityDataWriter {
  } finally {
  pw.close()
  }
- } finally {
- eli.close()
  }
  }
  } catch (Throwable t) {
@@ -248,8 +246,7 @@ class EntityDataWriterImpl implements EntityDataWriter {
  EntityDefinition ed = efi.getEntityDefinition(en)
  boolean useMaster = masterName != null && masterName.length() > 0 && ed.getMasterDefinition(masterName) != null
  EntityFind ef = makeEntityFind(en)
- EntityListIterator eli = ef.iterator()
- try {
+ try (EntityListIterator eli = ef.iterator()) {
  if (!eli.hasNext()) continue
 
  String filenameBase = tableColumnNames ? ed.getTableName() : en
@@ -274,8 +271,6 @@ class EntityDataWriterImpl implements EntityDataWriter {
  } finally {
  out.closeEntry()
  }
- } finally {
- eli.close()
  }
  }
  } finally {
@@ -306,15 +301,12 @@ class EntityDataWriterImpl implements EntityDataWriter {
  EntityDefinition ed = efi.getEntityDefinition(en)
  boolean useMaster = masterName != null && masterName.length() > 0 && ed.getMasterDefinition(masterName) != null
  EntityFind ef = makeEntityFind(en)
- EntityListIterator eli = ef.iterator()
- try {
+ ef.iterator().withCloseable ({
  EntityValue ev
  while ((ev = eli.next()) != null) {
  valuesWritten+= writeValue(ev, writer, useMaster)
  }
- } finally {
- eli.close()
- }
+ });
  }
 
  endFile(writer)

framework/src/main/groovy/org/moqui/impl/entity/EntityFindBase.groovy

@@ -1149,19 +1149,18 @@ abstract class EntityFindBase implements EntityFind {
  }
 
  // call the abstract method
- EntityListIterator eli
- try { eli = iteratorExtended(queryWhereCondition, havingCondition, orderByExpanded, fieldInfoArray, fieldOptionsArray) }
+ try (EntityListIterator eli = iteratorExtended(queryWhereCondition, havingCondition, orderByExpanded, fieldInfoArray, fieldOptionsArray)) {
+ MNode databaseNode = this.efi.getDatabaseNode(ed.getEntityGroupName())
+ if (limit != null && databaseNode != null && "cursor".equals(databaseNode.attribute("offset-style"))) {
+ el = (EntityListImpl) eli.getPartialList(offset != null ? offset : 0, limit, false)
+ } else {
+ el = (EntityListImpl) eli.getCompleteList(false);
+ }
+ }
  catch (SQLException e) { throw new EntitySqlException(makeErrorMsg("Error finding list of", LIST_ERROR, queryWhereCondition, ed, ec), e) }
  catch (ArtifactAuthorizationException e) { throw e }
  catch (Exception e) { throw new EntityException(makeErrorMsg("Error finding list of", LIST_ERROR, queryWhereCondition, ed, ec), e) }
 
- MNode databaseNode = this.efi.getDatabaseNode(ed.getEntityGroupName())
- if (limit != null && databaseNode != null && "cursor".equals(databaseNode.attribute("offset-style"))) {
- el = (EntityListImpl) eli.getPartialList(offset != null ? offset : 0, limit, true)
- } else {
- el = (EntityListImpl) eli.getCompleteList(true)
- }
-
  // register lock after because we can't before, don't know which records will be returned
  if (forUpdate && !isViewEntity && efi.ecfi.transactionFacade.getUseLockTrack()) {
  int elSize = el.size()
@@ -1446,9 +1445,7 @@ abstract class EntityFindBase implements EntityFind {
 
  this.useCache(false)
  long totalUpdated = 0
- EntityListIterator eli = (EntityListIterator) null
- try {
- eli = iterator()
+ iterator().withCloseable ({eli ->
  EntityValue value
  while ((value = eli.next()) != null) {
  value.putAll(fieldsToSet)
@@ -1458,9 +1455,7 @@ abstract class EntityFindBase implements EntityFind {
  totalUpdated++
  }
  }
- } finally {
- if (eli != null) eli.close()
- }
+ })
  return totalUpdated
  }
 
@@ -1495,33 +1490,27 @@ abstract class EntityFindBase implements EntityFind {
  }
  } else {
  this.resultSetConcurrency(ResultSet.CONCUR_UPDATABLE)
- EntityListIterator eli = (EntityListIterator) null
- try {
- eli = iterator()
+ iterator().withCloseable ({eli->
+
  while (eli.next() != null) {
  // no longer need to clear cache, eli.remove() does that
  eli.remove()
  totalDeleted++
  }
- } finally {
- if (eli != null) eli.close()
- }
+ })
  }
  return totalDeleted
  }
 
  @Override
  void extract(SimpleEtl etl) {
- EntityListIterator eli = iterator()
- try {
+ try (EntityListIterator eli = iterator()) {
  EntityValue ev
  while ((ev = eli.next()) != null) {
  etl.processEntry(ev)
  }
  } catch (StopException e) {
  logger.warn("EntityFind extract stopped on: " + (e.getCause()?.toString() ?: e.toString()))
- } finally {
- eli.close()
  }
  }
 

framework/src/main/groovy/org/moqui/impl/entity/EntityListIteratorImpl.java

@@ -285,6 +285,7 @@ public EntityValueBase currentEntityValueBase() {
  } catch (SQLException e) {
  throw new EntityException("Error getting all results", e);
  } finally {
+ //TODO: Remove closeAfter with respect to try-with-resource implementation
  if (closeAfter) close();
  }
  }

framework/src/test/groovy/EntityNoSqlCrud.groovy

@@ -121,23 +121,19 @@ class EntityNoSqlCrud extends Specification {
 
  def "ELI find TestNoSqlEntity"() {
  when:
- EntityListIterator eli
  EntityList partialEl = null
  EntityValue first = null
- try {
- eli = ec.entity.find("moqui.test.TestNoSqlEntity")
- .orderBy("-testNumberInteger").iterator()
+ try (EntityListIterator eli = ec.entity.find("moqui.test.TestNoSqlEntity")
+  .orderBy("-testNumberInteger").iterator()) {
+
 
  partialEl = eli.getPartialList(0, 100, false)
 
  eli.beforeFirst()
  first = eli.next()
  } catch (Exception e) {
  logger.error("partialEl error", e)
- } finally {
- if (eli != null) eli.close()
  }
-
  // logger.warn("partialEl.size() ${partialEl.size()} first value ${first}")
 
  then: