[GitHub] [qpid-broker-j] alex-rufous commented on a change in pull request #103: QPID-8550 - [Broker-J] HP Fortify: Unreleased streams

[GitBox <gi...@apache.org>]

alex-rufous commented on a change in pull request #103:
URL: https://github.com/apache/qpid-broker-j/pull/103#discussion_r683282742



##########
File path: broker-core/src/main/java/org/apache/qpid/server/store/JsonFileConfigStore.java
##########
@@ -156,49 +156,52 @@ protected boolean load(final ConfiguredObjectRecord... initialRecords)
 
             boolean updated = false;
             Collection<ConfiguredObjectRecord> records = Collections.emptyList();
-            ConfiguredObjectRecordConverter configuredObjectRecordConverter =
+            final ConfiguredObjectRecordConverter configuredObjectRecordConverter =
                     new ConfiguredObjectRecordConverter(_parent.getModel());
 
-            records = configuredObjectRecordConverter.readFromJson(_rootClass, _parent, new FileReader(configFile));
-
-            if(_rootClass == null)
+            try (FileReader configFileReader =  new FileReader(configFile))

Review comment:
       This change is unnecessary.  The `configFileReader` is closed in `ObjectMapper`. If you really need to make sure that `configFileReader` is closed in the code where it is created, you can simply add `try-with-resource` block around  the code reading from json
   ```
   try (FileReader configFileReader =  new FileReader(configFile))
   {
       records = configuredObjectRecordConverter.readFromJson(_rootClass, _parent, configFileReader);
   }
   ```
   

##########
File path: broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
##########
@@ -437,7 +437,10 @@ public String getRequestURL()
                 {
                     if ("data".equals(part.getName()) && "application/json".equals(part.getContentType()))
                     {
-                        items = mapper.readValue(part.getInputStream(), LinkedHashMap.class);
+                        try (InputStream inputStream = part.getInputStream())

Review comment:
       The change is unnecessary as methods `ObjectMapper#readValue ` close the input stream/reader..

##########
File path: perftests/src/main/java/org/apache/qpid/disttest/controller/config/ConfigReader.java
##########
@@ -37,10 +37,10 @@
 
     public Config getConfigFromFile(String fileName) throws IOException
     {
-        Reader reader = getConfigReader(fileName);
-
-        Config config = readConfig(reader);
-        return config;
+        try (Reader reader = getConfigReader(fileName))

Review comment:
       The reader is closed in  `ObjectMapper#read...`. Thus, the change is unnecessary...

##########
File path: broker-core/src/main/java/org/apache/qpid/server/util/FileUtils.java
##########
@@ -205,8 +205,10 @@ public static void copy(File src, File dst)
      */
     public static void copyCheckedEx(File src, File dst) throws IOException
     {
-        InputStream in = new FileInputStream(src);
-        copy(in, dst);
+        try (InputStream in = new FileInputStream(src))

Review comment:
       The `InputStream` is closed in `copy`. Arguably the method `copy` should indicate that it closes the input stream. It make more sense to rename it into `copyInputStreamIntoFileAndClose`. Though, I suppose for a better code relaibility the 'try-with-resource` would be good here.

##########
File path: broker-core/src/main/java/org/apache/qpid/server/virtualhostnode/AbstractVirtualHostNode.java
##########
@@ -430,40 +430,42 @@ public PreferenceStore createPreferenceStore()
 
     protected final ConfiguredObjectRecord[] getInitialRecords() throws IOException
     {
-        ConfiguredObjectRecordConverter converter = new ConfiguredObjectRecordConverter(getModel());
+        final ConfiguredObjectRecordConverter converter = new ConfiguredObjectRecordConverter(getModel());
 
-        Collection<ConfiguredObjectRecord> records =
-                new ArrayList<>(converter.readFromJson(VirtualHost.class,this,getInitialConfigReader()));
-
-        if(!records.isEmpty())
+        try (final Reader initialConfigReader = getInitialConfigReader())

Review comment:
       Try-with-resource block is unnecessary here as reading code is closing the reader. If you really have to change to `try-with-resource`, it would be preferable to keep the `try` block small and only use it for reading the records. IMHO, that makes code more readable 
   ```
   final Collection<ConfiguredObjectRecord> records;
   try (final Reader initialConfigReader = getInitialConfigReader())
   {
       records = new ArrayList<>(converter.readFromJson(VirtualHost.class, this, initialConfigReader));
   }
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org