You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2022/07/20 07:28:22 UTC

[GitHub] [sling-org-apache-sling-jcr-resource] anchela opened a new pull request, #31: SLING-11468 : Simplify JcrPropertyMapCacheEntry.convertToType

anchela opened a new pull request, #31:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/31

   @cziegeler , @joerghoh , i would appreciate if you could take a look at the refactoring. in particular i tried to get rid of the duplicate conversion for strings and the fall-through behavior for conversion from inputstream that i found super hard to read.


-- 
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@sling.apache.org

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


[GitHub] [sling-org-apache-sling-jcr-resource] anchela commented on a diff in pull request #31: SLING-11468 : Simplify JcrPropertyMapCacheEntry.convertToType

Posted by GitBox <gi...@apache.org>.
anchela commented on code in PR #31:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/31#discussion_r925415518


##########
src/main/java/org/apache/sling/jcr/resource/internal/helper/JcrPropertyMapCacheEntry.java:
##########
@@ -284,79 +284,84 @@ public boolean isArray() {
         if (type.isInstance(initialValue)) {
             return (T) initialValue;
         }
+        
+        if (initialValue instanceof InputStream) {
+            return convertInputStream(index, (InputStream) initialValue, type, node, dynamicClassLoader);
+        } else {
+            return convert(initialValue, type, node);
+        }
+    }
+    
+    private @Nullable <T> T convertInputStream(int index,
+                                               final @NotNull InputStream value,
+                                               final @NotNull Class<T> type,
+                                               final @NotNull Node node,
+                                               final @Nullable ClassLoader dynamicClassLoader) throws RepositoryException {
+        // object input stream
+        if (ObjectInputStream.class.isAssignableFrom(type)) {
+            try {
+                return (T) new PropertyObjectInputStream(value, dynamicClassLoader);
+            } catch (final IOException ioe) {
+                // ignore and use fallback
+            }
 
-        Object value = initialValue;
-
-        // special case input stream first
-        if (value instanceof InputStream) {
-            // object input stream
-            if (ObjectInputStream.class.isAssignableFrom(type)) {
-                try {
-                    return (T) new PropertyObjectInputStream((InputStream) value, dynamicClassLoader);
-                } catch (final IOException ioe) {
-                    // ignore and use fallback
-                }
-
-                // any number: length of binary
-            } else if (Number.class.isAssignableFrom(type)) {
-                // avoid NPE if this instance has not been created from a property (see SLING-11465)
-                if (property == null) {
-                    return null;
-                } 
-                
-                if (index == -1) {
-                    value = Long.valueOf(this.property.getLength());
-                } else {
-                    value = Long.valueOf(this.property.getLengths()[index]);
-                }
-
-                // string: read binary
-            } else if (String.class == type) {
-                final InputStream in = (InputStream) value;
-                try {
-                    final ByteArrayOutputStream baos = new ByteArrayOutputStream();
-                    final byte[] buffer = new byte[2048];
-                    int l;
-                    while ((l = in.read(buffer)) >= 0) {
-                        if (l > 0) {
-                            baos.write(buffer, 0, l);
-                        }
-                    }
-                    value = new String(baos.toByteArray(), StandardCharsets.UTF_8);
-                } catch (final IOException e) {
-                    throw new IllegalArgumentException(e);
-                } finally {
-                    try {
-                        in.close();
-                    } catch (final IOException ignore) {
-                        // ignore
-                    }
+        // any number: length of binary
+        } else if (Number.class.isAssignableFrom(type)) {
+            // avoid NPE if this instance has not been created from a property (see SLING-11465)
+            if (property == null) {
+                return null;
+            }
+            return convert(propertyToLength(property, index), type, node);
+            
+        // string: read binary
+        } else if (String.class == type) {
+            return (T) inputStreamToString(value);
+            
+        // any serializable
+        } else if (Serializable.class.isAssignableFrom(type)) {
+            try (ObjectInputStream ois = new PropertyObjectInputStream(value, dynamicClassLoader)) {
+                final Object obj = ois.readObject();
+                if (type.isInstance(obj)) {
+                    return (T) obj;
                 }
-
-                // any serializable
-            } else if (Serializable.class.isAssignableFrom(type)) {
-                ObjectInputStream ois = null;
-                try {
-                    ois = new PropertyObjectInputStream((InputStream) value, dynamicClassLoader);
-                    final Object obj = ois.readObject();
-                    if (type.isInstance(obj)) {
-                        return (T) obj;
-                    }
-                    value = obj;
-                } catch (final ClassNotFoundException | IOException cnfe) {
-                    // ignore and use fallback
-                } finally {
-                    if (ois != null) {
-                        try {
-                            ois.close();
-                        } catch (final IOException ignore) {
-                            // ignore
-                        }
-                    }
+                return convert(obj, type, node);
+            } catch (final ClassNotFoundException | IOException cnfe) {
+                // ignore and use fallback
+            }
+            // ignore
+        } 
+        
+        // fallback
+        return convert(value, type, node);
+    }
+    
+    private static @NotNull Long propertyToLength(@NotNull Property property, int index) throws RepositoryException {
+        if (index == -1) {
+            return Long.valueOf(property.getLength());
+        } else {
+            return Long.valueOf(property.getLengths()[index]);
+        }
+    }
+    
+    private static @NotNull String inputStreamToString(@NotNull InputStream value) {
+        try (InputStream in = value) {
+            final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+            final byte[] buffer = new byte[2048];
+            int l;
+            while ((l = in.read(buffer)) >= 0) {
+                if (l > 0) {
+                    baos.write(buffer, 0, l);
                 }
             }
+            return new String(baos.toByteArray(), StandardCharsets.UTF_8);

Review Comment:
   i don't want to make major changes to the logic in a simple refactoring PR. can we cover this with a separate ticket?
   



-- 
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@sling.apache.org

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


[GitHub] [sling-org-apache-sling-jcr-resource] anchela commented on a diff in pull request #31: SLING-11468 : Simplify JcrPropertyMapCacheEntry.convertToType

Posted by GitBox <gi...@apache.org>.
anchela commented on code in PR #31:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/31#discussion_r925421638


##########
src/test/java/org/apache/sling/jcr/resource/internal/helper/JcrPropertyMapCacheEntryTest.java:
##########
@@ -119,6 +127,39 @@ public void testCharArray() throws Exception {
         verifyZeroInteractions(node);
     }
     
+    @Test
+    public void testCannotStore() throws Exception {
+        Object value = new TestClass();
+        try {
+            new JcrPropertyMapCacheEntry(value, node);
+            fail("IllegalArgumentException expected");
+        } catch (IllegalArgumentException e) {

Review Comment:
   ma certo....
   



-- 
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@sling.apache.org

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


[GitHub] [sling-org-apache-sling-jcr-resource] sonarcloud[bot] commented on pull request #31: SLING-11468 : Simplify JcrPropertyMapCacheEntry.convertToType

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #31:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/31#issuecomment-1190090075

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=CODE_SMELL)
   
   [![92.6%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90-16px.png '92.6%')](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&metric=new_coverage&view=list) [92.6% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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@sling.apache.org

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


[GitHub] [sling-org-apache-sling-jcr-resource] anchela commented on pull request #31: SLING-11468 : Simplify JcrPropertyMapCacheEntry.convertToType

Posted by GitBox <gi...@apache.org>.
anchela commented on PR #31:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/31#issuecomment-1191145185

   @joerghoh , can you take a look at my changes again an (hopefully) approve?


-- 
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@sling.apache.org

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


[GitHub] [sling-org-apache-sling-jcr-resource] anchela merged pull request #31: SLING-11468 : Simplify JcrPropertyMapCacheEntry.convertToType

Posted by GitBox <gi...@apache.org>.
anchela merged PR #31:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/31


-- 
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@sling.apache.org

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


[GitHub] [sling-org-apache-sling-jcr-resource] joerghoh commented on a diff in pull request #31: SLING-11468 : Simplify JcrPropertyMapCacheEntry.convertToType

Posted by GitBox <gi...@apache.org>.
joerghoh commented on code in PR #31:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/31#discussion_r925430811


##########
src/main/java/org/apache/sling/jcr/resource/internal/helper/JcrPropertyMapCacheEntry.java:
##########
@@ -284,79 +284,84 @@ public boolean isArray() {
         if (type.isInstance(initialValue)) {
             return (T) initialValue;
         }
+        
+        if (initialValue instanceof InputStream) {
+            return convertInputStream(index, (InputStream) initialValue, type, node, dynamicClassLoader);
+        } else {
+            return convert(initialValue, type, node);
+        }
+    }
+    
+    private @Nullable <T> T convertInputStream(int index,
+                                               final @NotNull InputStream value,
+                                               final @NotNull Class<T> type,
+                                               final @NotNull Node node,
+                                               final @Nullable ClassLoader dynamicClassLoader) throws RepositoryException {
+        // object input stream
+        if (ObjectInputStream.class.isAssignableFrom(type)) {
+            try {
+                return (T) new PropertyObjectInputStream(value, dynamicClassLoader);
+            } catch (final IOException ioe) {
+                // ignore and use fallback
+            }
 
-        Object value = initialValue;
-
-        // special case input stream first
-        if (value instanceof InputStream) {
-            // object input stream
-            if (ObjectInputStream.class.isAssignableFrom(type)) {
-                try {
-                    return (T) new PropertyObjectInputStream((InputStream) value, dynamicClassLoader);
-                } catch (final IOException ioe) {
-                    // ignore and use fallback
-                }
-
-                // any number: length of binary
-            } else if (Number.class.isAssignableFrom(type)) {
-                // avoid NPE if this instance has not been created from a property (see SLING-11465)
-                if (property == null) {
-                    return null;
-                } 
-                
-                if (index == -1) {
-                    value = Long.valueOf(this.property.getLength());
-                } else {
-                    value = Long.valueOf(this.property.getLengths()[index]);
-                }
-
-                // string: read binary
-            } else if (String.class == type) {
-                final InputStream in = (InputStream) value;
-                try {
-                    final ByteArrayOutputStream baos = new ByteArrayOutputStream();
-                    final byte[] buffer = new byte[2048];
-                    int l;
-                    while ((l = in.read(buffer)) >= 0) {
-                        if (l > 0) {
-                            baos.write(buffer, 0, l);
-                        }
-                    }
-                    value = new String(baos.toByteArray(), StandardCharsets.UTF_8);
-                } catch (final IOException e) {
-                    throw new IllegalArgumentException(e);
-                } finally {
-                    try {
-                        in.close();
-                    } catch (final IOException ignore) {
-                        // ignore
-                    }
+        // any number: length of binary
+        } else if (Number.class.isAssignableFrom(type)) {
+            // avoid NPE if this instance has not been created from a property (see SLING-11465)
+            if (property == null) {
+                return null;
+            }
+            return convert(propertyToLength(property, index), type, node);
+            
+        // string: read binary
+        } else if (String.class == type) {
+            return (T) inputStreamToString(value);
+            
+        // any serializable
+        } else if (Serializable.class.isAssignableFrom(type)) {
+            try (ObjectInputStream ois = new PropertyObjectInputStream(value, dynamicClassLoader)) {
+                final Object obj = ois.readObject();
+                if (type.isInstance(obj)) {
+                    return (T) obj;
                 }
-
-                // any serializable
-            } else if (Serializable.class.isAssignableFrom(type)) {
-                ObjectInputStream ois = null;
-                try {
-                    ois = new PropertyObjectInputStream((InputStream) value, dynamicClassLoader);
-                    final Object obj = ois.readObject();
-                    if (type.isInstance(obj)) {
-                        return (T) obj;
-                    }
-                    value = obj;
-                } catch (final ClassNotFoundException | IOException cnfe) {
-                    // ignore and use fallback
-                } finally {
-                    if (ois != null) {
-                        try {
-                            ois.close();
-                        } catch (final IOException ignore) {
-                            // ignore
-                        }
-                    }
+                return convert(obj, type, node);
+            } catch (final ClassNotFoundException | IOException cnfe) {
+                // ignore and use fallback
+            }
+            // ignore
+        } 
+        
+        // fallback
+        return convert(value, type, node);
+    }
+    
+    private static @NotNull Long propertyToLength(@NotNull Property property, int index) throws RepositoryException {
+        if (index == -1) {
+            return Long.valueOf(property.getLength());
+        } else {
+            return Long.valueOf(property.getLengths()[index]);
+        }
+    }
+    
+    private static @NotNull String inputStreamToString(@NotNull InputStream value) {
+        try (InputStream in = value) {
+            final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+            final byte[] buffer = new byte[2048];
+            int l;
+            while ((l = in.read(buffer)) >= 0) {
+                if (l > 0) {
+                    baos.write(buffer, 0, l);
                 }
             }
+            return new String(baos.toByteArray(), StandardCharsets.UTF_8);

Review Comment:
   https://issues.apache.org/jira/browse/SLING-11471



-- 
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@sling.apache.org

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


[GitHub] [sling-org-apache-sling-jcr-resource] joerghoh commented on a diff in pull request #31: SLING-11468 : Simplify JcrPropertyMapCacheEntry.convertToType

Posted by GitBox <gi...@apache.org>.
joerghoh commented on code in PR #31:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/31#discussion_r925287755


##########
src/test/java/org/apache/sling/jcr/resource/internal/helper/JcrPropertyMapCacheEntryTest.java:
##########
@@ -119,6 +127,39 @@ public void testCharArray() throws Exception {
         verifyZeroInteractions(node);
     }
     
+    @Test
+    public void testCannotStore() throws Exception {
+        Object value = new TestClass();
+        try {
+            new JcrPropertyMapCacheEntry(value, node);
+            fail("IllegalArgumentException expected");
+        } catch (IllegalArgumentException e) {

Review Comment:
   you can simplify this test case like this:
   ```
       @Test(expected=IllegalArgumentException.class)
       public void testCannotStore() throws Exception {
           Object value = new TestClass();
           new JcrPropertyMapCacheEntry(value, node);
        }
   ```
   
   You might want to split this testcase into 2 distinct testcases, though :-)



##########
src/main/java/org/apache/sling/jcr/resource/internal/helper/JcrPropertyMapCacheEntry.java:
##########
@@ -284,79 +284,84 @@ public boolean isArray() {
         if (type.isInstance(initialValue)) {
             return (T) initialValue;
         }
+        
+        if (initialValue instanceof InputStream) {
+            return convertInputStream(index, (InputStream) initialValue, type, node, dynamicClassLoader);
+        } else {
+            return convert(initialValue, type, node);
+        }
+    }
+    
+    private @Nullable <T> T convertInputStream(int index,
+                                               final @NotNull InputStream value,
+                                               final @NotNull Class<T> type,
+                                               final @NotNull Node node,
+                                               final @Nullable ClassLoader dynamicClassLoader) throws RepositoryException {
+        // object input stream
+        if (ObjectInputStream.class.isAssignableFrom(type)) {
+            try {
+                return (T) new PropertyObjectInputStream(value, dynamicClassLoader);
+            } catch (final IOException ioe) {
+                // ignore and use fallback
+            }
 
-        Object value = initialValue;
-
-        // special case input stream first
-        if (value instanceof InputStream) {
-            // object input stream
-            if (ObjectInputStream.class.isAssignableFrom(type)) {
-                try {
-                    return (T) new PropertyObjectInputStream((InputStream) value, dynamicClassLoader);
-                } catch (final IOException ioe) {
-                    // ignore and use fallback
-                }
-
-                // any number: length of binary
-            } else if (Number.class.isAssignableFrom(type)) {
-                // avoid NPE if this instance has not been created from a property (see SLING-11465)
-                if (property == null) {
-                    return null;
-                } 
-                
-                if (index == -1) {
-                    value = Long.valueOf(this.property.getLength());
-                } else {
-                    value = Long.valueOf(this.property.getLengths()[index]);
-                }
-
-                // string: read binary
-            } else if (String.class == type) {
-                final InputStream in = (InputStream) value;
-                try {
-                    final ByteArrayOutputStream baos = new ByteArrayOutputStream();
-                    final byte[] buffer = new byte[2048];
-                    int l;
-                    while ((l = in.read(buffer)) >= 0) {
-                        if (l > 0) {
-                            baos.write(buffer, 0, l);
-                        }
-                    }
-                    value = new String(baos.toByteArray(), StandardCharsets.UTF_8);
-                } catch (final IOException e) {
-                    throw new IllegalArgumentException(e);
-                } finally {
-                    try {
-                        in.close();
-                    } catch (final IOException ignore) {
-                        // ignore
-                    }
+        // any number: length of binary
+        } else if (Number.class.isAssignableFrom(type)) {
+            // avoid NPE if this instance has not been created from a property (see SLING-11465)
+            if (property == null) {
+                return null;
+            }
+            return convert(propertyToLength(property, index), type, node);
+            
+        // string: read binary
+        } else if (String.class == type) {
+            return (T) inputStreamToString(value);
+            
+        // any serializable
+        } else if (Serializable.class.isAssignableFrom(type)) {
+            try (ObjectInputStream ois = new PropertyObjectInputStream(value, dynamicClassLoader)) {
+                final Object obj = ois.readObject();
+                if (type.isInstance(obj)) {
+                    return (T) obj;
                 }
-
-                // any serializable
-            } else if (Serializable.class.isAssignableFrom(type)) {
-                ObjectInputStream ois = null;
-                try {
-                    ois = new PropertyObjectInputStream((InputStream) value, dynamicClassLoader);
-                    final Object obj = ois.readObject();
-                    if (type.isInstance(obj)) {
-                        return (T) obj;
-                    }
-                    value = obj;
-                } catch (final ClassNotFoundException | IOException cnfe) {
-                    // ignore and use fallback
-                } finally {
-                    if (ois != null) {
-                        try {
-                            ois.close();
-                        } catch (final IOException ignore) {
-                            // ignore
-                        }
-                    }
+                return convert(obj, type, node);
+            } catch (final ClassNotFoundException | IOException cnfe) {
+                // ignore and use fallback
+            }
+            // ignore
+        } 
+        
+        // fallback
+        return convert(value, type, node);
+    }
+    
+    private static @NotNull Long propertyToLength(@NotNull Property property, int index) throws RepositoryException {
+        if (index == -1) {
+            return Long.valueOf(property.getLength());
+        } else {
+            return Long.valueOf(property.getLengths()[index]);
+        }
+    }
+    
+    private static @NotNull String inputStreamToString(@NotNull InputStream value) {
+        try (InputStream in = value) {
+            final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+            final byte[] buffer = new byte[2048];
+            int l;
+            while ((l = in.read(buffer)) >= 0) {
+                if (l > 0) {
+                    baos.write(buffer, 0, l);
                 }
             }
+            return new String(baos.toByteArray(), StandardCharsets.UTF_8);

Review Comment:
   I don't know if this is reasonable, but this functionality can be a cause of memory issues. Can you add some warning logic, if the number of bytes read into the ```ByteArrayOutputStream``` exceeds 100k/1M/10M/100M bytes? 



##########
src/main/java/org/apache/sling/jcr/resource/internal/helper/JcrPropertyMapCacheEntry.java:
##########
@@ -403,7 +408,7 @@ public boolean isArray() {
 
         // fallback in case of unsupported type
         return null;
-    }
+    } 

Review Comment:
   unnecessary whitespace :-)



-- 
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@sling.apache.org

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


[GitHub] [sling-org-apache-sling-jcr-resource] sonarcloud[bot] commented on pull request #31: SLING-11468 : Simplify JcrPropertyMapCacheEntry.convertToType

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #31:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/31#issuecomment-1189932132

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&resolved=false&types=CODE_SMELL)
   
   [![92.6%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90-16px.png '92.6%')](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&metric=new_coverage&view=list) [92.6% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=31&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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@sling.apache.org

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


[GitHub] [sling-org-apache-sling-jcr-resource] anchela commented on pull request #31: SLING-11468 : Simplify JcrPropertyMapCacheEntry.convertToType

Posted by GitBox <gi...@apache.org>.
anchela commented on PR #31:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/31#issuecomment-1190085687

   @joerghoh the potential memory issue with ByteArrayOutputStream should go into a separate bug report. the other findings i fixed. thanks a lot for taking a close look. let me know if there are other things left.


-- 
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@sling.apache.org

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