Better Logging and Miscellaneous Cleanup (Azure#32420)

Better Logging and Miscellaneous Cleanup

Author: alzimmermsft

sdk/core/azure-core/src/main/java/com/azure/core/implementation/RetriableDownloadFlux.java

@@ -3,6 +3,8 @@
 
 package com.azure.core.implementation;
 
+import com.azure.core.util.logging.ClientLogger;
+import com.azure.core.util.logging.LogLevel;
 import reactor.core.CoreSubscriber;
 import reactor.core.publisher.Flux;
 
@@ -15,6 +17,8 @@
  * operation if an error occurs during the download.
  */
 public final class RetriableDownloadFlux extends Flux<ByteBuffer> {
+    private static final ClientLogger LOGGER = new ClientLogger(RetriableDownloadFlux.class);
+
     private final Supplier<Flux<ByteBuffer>> downloadSupplier;
     private final BiFunction<Throwable, Long, Flux<ByteBuffer>> onDownloadErrorResume;
     private final int maxRetries;
@@ -58,9 +62,14 @@ public void subscribe(CoreSubscriber<? super ByteBuffer> actual) {
                 int updatedRetryCount = retryCount + 1;
 
                 if (updatedRetryCount > maxRetries) {
+                    LOGGER.log(LogLevel.ERROR, () -> "Exhausted all retry attempts while downloading, "
+                        + updatedRetryCount + " of " + maxRetries + ".", exception);
                     return Flux.error(exception);
                 }
 
+                LOGGER.log(LogLevel.INFORMATIONAL,
+                    () -> "Using retry attempt " + updatedRetryCount + " of " + maxRetries + " while downloading.",
+                    exception);
                 return new RetriableDownloadFlux(() -> onDownloadErrorResume.apply(exception, currentPosition[0]),
                     onDownloadErrorResume, maxRetries, currentPosition[0], updatedRetryCount);
             })

sdk/core/azure-core/src/main/java/com/azure/core/implementation/SemanticVersion.java

@@ -48,14 +48,14 @@ public static SemanticVersion getPackageVersionForClass(String className) {
      */
     public static SemanticVersion parse(String version) {
         Objects.requireNonNull(version, "'version' cannot be null.");
-        String[] parts = version.split("\\.");
-        if (parts.length < 3) {
+
+        int majorDotIdx = version.indexOf('.');
+        if (majorDotIdx < 0) {
             return createInvalid(version);
         }
 
-        int majorDotIdx = version.indexOf('.');
         int minorDotIdx = version.indexOf('.', majorDotIdx + 1);
-        if (majorDotIdx < 0 || minorDotIdx < 0) {
+        if (minorDotIdx < 0) {
             return createInvalid(version);
         }
 

sdk/core/azure-core/src/main/java/com/azure/core/implementation/StringBuilderWriter.java

@@ -9,11 +9,14 @@
 
 /**
  * Implementation of {@link Writer} that write content to a {@link StringBuilder}.
+ * <p>
+ * Given the backing store of this {@link Writer} is a {@link StringBuilder} this is not thread-safe.
  */
 public final class StringBuilderWriter extends Writer {
     private final StringBuilder builder;
 
-    private volatile boolean closed = false;
+    // This can be non-volatile as StringBuilder itself isn't thread-safe.
+    private boolean closed = false;
 
     /**
      * Creates an instance of {@link StringBuilderWriter}.
@@ -27,45 +30,35 @@ public StringBuilderWriter(StringBuilder builder) {
 
     @Override
     public void write(int c) throws IOException {
-        if (closed) {
-            throw new IOException("Writer has been closed.");
-        }
+        ensureOpen();
 
         builder.append(c);
     }
 
     @Override
     public void write(char[] cbuf) throws IOException {
-        if (closed) {
-            throw new IOException("Writer has been closed.");
-        }
+        ensureOpen();
 
         builder.append(cbuf);
     }
 
     @Override
     public void write(String str) throws IOException {
-        if (closed) {
-            throw new IOException("Writer has been closed.");
-        }
+        ensureOpen();
 
         builder.append(str);
     }
 
     @Override
     public void write(String str, int off, int len) throws IOException {
-        if (closed) {
-            throw new IOException("Writer has been closed.");
-        }
+        ensureOpen();
 
         builder.append(str, off, len);
     }
 
     @Override
     public Writer append(CharSequence csq) throws IOException {
-        if (closed) {
-            throw new IOException("Writer has been closed.");
-        }
+        ensureOpen();
 
         builder.append(csq);
 
@@ -74,9 +67,7 @@ public Writer append(CharSequence csq) throws IOException {
 
     @Override
     public Writer append(CharSequence csq, int start, int end) throws IOException {
-        if (closed) {
-            throw new IOException("Writer has been closed.");
-        }
+        ensureOpen();
 
         builder.append(csq, start, end);
 
@@ -85,9 +76,7 @@ public Writer append(CharSequence csq, int start, int end) throws IOException {
 
     @Override
     public Writer append(char c) throws IOException {
-        if (closed) {
-            throw new IOException("Writer has been closed.");
-        }
+        ensureOpen();
 
         builder.append(c);
 
@@ -96,22 +85,24 @@ public Writer append(char c) throws IOException {
 
     @Override
     public void write(char[] cbuf, int off, int len) throws IOException {
-        if (closed) {
-            throw new IOException("Writer has been closed.");
-        }
+        ensureOpen();
 
         builder.append(cbuf, off, len);
     }
 
     @Override
     public void flush() throws IOException {
-        if (closed) {
-            throw new IOException("Writer has been closed.");
-        }
+        ensureOpen();
     }
 
     @Override
     public void close() {
         closed = true;
     }
+
+    private void ensureOpen() throws IOException {
+        if (closed) {
+            throw new IOException("Writer has been closed.");
+        }
+    }
 }

sdk/core/azure-core/src/main/java/com/azure/core/implementation/TypeUtil.java

@@ -65,7 +65,6 @@ public static Type getTypeArgument(Type type) {
      * @param type the input type
      * @return the raw class
      */
-    @SuppressWarnings("unchecked")
     public static Class<?> getRawClass(Type type) {
         if (type instanceof ParameterizedType) {
             return (Class<?>) ((ParameterizedType) type).getRawType();
@@ -81,6 +80,11 @@ public static Class<?> getRawClass(Type type) {
      * @return the direct super type
      */
     public static Type getSuperType(final Type type) {
+        Type superType = SUPER_TYPE_MAP.get(type);
+        if (superType != null) {
+            return superType;
+        }
+
         return SUPER_TYPE_MAP.computeIfAbsent(type, _type -> {
             if (type instanceof ParameterizedType) {
                 final ParameterizedType parameterizedType = (ParameterizedType) type;
@@ -186,17 +190,6 @@ public Type getOwnerType() {
         };
     }
 
-    /**
-     * Returns whether the rest response expects to have any body (by checking if the body parameter type is set to
-     * Void, in which case no body is expected).
-     *
-     * @param restResponseReturnType The RestResponse subtype containing the type arguments we are inspecting.
-     * @return True if a body is expected, false if a Void body is expected.
-     */
-    public static boolean restResponseTypeExpectsBody(ParameterizedType restResponseReturnType) {
-        return getRestResponseBodyType(restResponseReturnType) != Void.class;
-    }
-
     /**
      * Returns the body type expected in the rest response.
      *

sdk/core/azure-core/src/main/java/com/azure/core/util/Base64Url.java

@@ -52,7 +52,7 @@ private static byte[] unquote(byte[] bytes) {
     }
 
     private static String unquote(String string) {
-        if (string != null && !string.isEmpty()) {
+        if (!CoreUtils.isNullOrEmpty(string)) {
             final char firstCharacter = string.charAt(0);
             if (firstCharacter == '\"' || firstCharacter == '\'') {
                 final int base64UrlStringLength = string.length();
@@ -113,15 +113,14 @@ public int hashCode() {
 
     @Override
     public boolean equals(Object obj) {
-        if (obj == null) {
-            return false;
+        if (obj == this) {
+            return true;
         }
 
         if (!(obj instanceof Base64Url)) {
             return false;
         }
 
-        Base64Url rhs = (Base64Url) obj;
-        return Arrays.equals(this.bytes, rhs.encodedBytes());
+        return Arrays.equals(this.bytes, ((Base64Url) obj).encodedBytes());
     }
 }

sdk/core/azure-core/src/main/java/com/azure/core/util/Configuration.java

@@ -206,8 +206,8 @@ public class Configuration implements Cloneable {
      * {@code NettyAsyncHttpClientProvider}, to disambiguate multiple providers with the same name but from different
      * packages.
      * <p>
-     * If the value isn't set or is an empty string the first {@link HttpClientProvider} found on the class path will
-     * be used to create an instance of {@link HttpClient}. If the value is set and doesn't match any
+     * If the value isn't set or is an empty string the first {@link HttpClientProvider} found on the class path will be
+     * used to create an instance of {@link HttpClient}. If the value is set and doesn't match any
      * {@link HttpClientProvider} found on the class path an {@link IllegalStateException} will be thrown when
      * attempting to create an instance of {@link HttpClient}.
      */
@@ -235,38 +235,43 @@ public class Configuration implements Cloneable {
     /**
      * Constructs a configuration containing the known Azure properties constants.
      *
-     * @deprecated Use {@link ConfigurationBuilder} and {@link ConfigurationSource} that allow to
-     * provide all properties before creating configuration and keep it immutable.
+     * @deprecated Use {@link ConfigurationBuilder} and {@link ConfigurationSource} that allow to provide all properties
+     * before creating configuration and keep it immutable.
      */
     @Deprecated
     public Configuration() {
         this(Collections.emptyMap(), EnvironmentConfiguration.getGlobalConfiguration(), null, null);
     }
 
     /**
-     * Constructs a configuration containing the known Azure properties constants. Use {@link ConfigurationBuilder} to create instance of {@link Configuration}.
+     * Constructs a configuration containing the known Azure properties constants. Use {@link ConfigurationBuilder} to
+     * create instance of {@link Configuration}.
      *
      * @param configurationSource Configuration property source.
      * @param environmentConfiguration instance of {@link EnvironmentConfiguration} to mock environment for testing.
      * @param path Absolute path of current configuration section for logging and diagnostics purposes.
      * @param sharedConfiguration Instance of shared {@link Configuration} section to retrieve shared properties.
      */
-    Configuration(ConfigurationSource configurationSource, EnvironmentConfiguration environmentConfiguration, String path, Configuration sharedConfiguration) {
+    Configuration(ConfigurationSource configurationSource, EnvironmentConfiguration environmentConfiguration,
+        String path, Configuration sharedConfiguration) {
         this(readConfigurations(configurationSource, path), environmentConfiguration, path, sharedConfiguration);
     }
 
     /**
-     * Constructs a configuration containing the known Azure properties constants. Use {@link ConfigurationBuilder} to create instance of {@link Configuration}.
+     * Constructs a configuration containing the known Azure properties constants. Use {@link ConfigurationBuilder} to
+     * create instance of {@link Configuration}.
      *
      * @param configurations map of all properties.
      * @param environmentConfiguration instance of {@link EnvironmentConfiguration} to mock environment for testing.
      * @param path Absolute path of current configuration section for logging and diagnostics purposes.
      * @param sharedConfiguration Instance of shared {@link Configuration} section to retrieve shared properties.
      */
-    private Configuration(Map<String, String> configurations, EnvironmentConfiguration environmentConfiguration, String path, Configuration sharedConfiguration) {
+    private Configuration(Map<String, String> configurations, EnvironmentConfiguration environmentConfiguration,
+        String path, Configuration sharedConfiguration) {
         this.configurations = configurations;
         this.isEmpty = configurations.isEmpty();
-        this.environmentConfiguration = Objects.requireNonNull(environmentConfiguration, "'environmentConfiguration' cannot be null");
+        this.environmentConfiguration = Objects.requireNonNull(environmentConfiguration,
+            "'environmentConfiguration' cannot be null");
         this.path = path;
         this.sharedConfiguration = sharedConfiguration;
     }
@@ -299,8 +304,8 @@ public String get(String name) {
      * Gets the value of system property or environment variable converted to given primitive {@code T} using
      * corresponding {@code parse} method on this type.
      *
-     * Use {@link Configuration#get(ConfigurationProperty)} overload to get explicit configuration or
-     * environment configuration from specific source.
+     * Use {@link Configuration#get(ConfigurationProperty)} overload to get explicit configuration or environment
+     * configuration from specific source.
      *
      * <p>
      * This method first checks the values previously loaded from the environment, if the configuration is found there
@@ -354,9 +359,8 @@ public <T> T get(String name, Function<String, T> converter) {
      * @param name Name of the configuration.
      * @param value Value of the configuration.
      * @return The updated Configuration object.
-     *
-     * @deprecated Use {@link ConfigurationBuilder} and {@link ConfigurationSource} to
-     * provide all properties before creating configuration.
+     * @deprecated Use {@link ConfigurationBuilder} and {@link ConfigurationSource} to provide all properties before
+     * creating configuration.
      */
     @Deprecated
     public Configuration put(String name, String value) {
@@ -371,9 +375,8 @@ public Configuration put(String name, String value) {
      *
      * @param name Name of the configuration.
      * @return The configuration if it previously existed, otherwise null.
-     *
-     * @deprecated Use {@link ConfigurationBuilder} and {@link ConfigurationSource} to
-     * provide all properties before creating configuration.
+     * @deprecated Use {@link ConfigurationBuilder} and {@link ConfigurationSource} to provide all properties before
+     * creating configuration.
      */
     @Deprecated
     public String remove(String name) {
@@ -382,9 +385,9 @@ public String remove(String name) {
 
     /**
      * Determines if the system property or environment variable is defined.
-     *
-     * Use {@link Configuration#contains(ConfigurationProperty)} overload to get explicit configuration or
-     * environment configuration from specific source.
+     * <p>
+     * Use {@link Configuration#contains(ConfigurationProperty)} overload to get explicit configuration or environment
+     * configuration from specific source.
      *
      * <p>
      * This only checks against values previously loaded into the Configuration object, this won't inspect the
@@ -406,12 +409,14 @@ public boolean contains(String name) {
     @SuppressWarnings("CloneDoesntCallSuperClone")
     @Deprecated
     public Configuration clone() {
-        return new Configuration(configurations, new EnvironmentConfiguration(environmentConfiguration), path, sharedConfiguration);
+        return new Configuration(configurations, new EnvironmentConfiguration(environmentConfiguration), path,
+            sharedConfiguration);
     }
 
     /**
-     * Checks if configuration contains the property. If property can be shared between clients, checks this {@code Configuration} and
-     * falls back to shared section. If property has aliases, system property or environment variable defined, checks them as well.
+     * Checks if configuration contains the property. If property can be shared between clients, checks this
+     * {@code Configuration} and falls back to shared section. If property has aliases, system property or environment
+     * variable defined, checks them as well.
      * <p>
      * Value is not validated.
      *
@@ -454,7 +459,7 @@ public boolean contains(ConfigurationProperty<?> property) {
      *
      * @param property instance.
      * @param <T> Type that the configuration is converted to if found.
-     * @return true if property is available, false otherwise.
+     * @return The value of the property if it exists, otherwise the default value of the property.
      * @throws NullPointerException when property instance is null.
      * @throws IllegalArgumentException when required property is missing.
      * @throws RuntimeException when property value conversion (and validation) throws.
@@ -595,8 +600,8 @@ private static Map<String, String> readConfigurations(ConfigurationSource source
     }
 
     /**
-     * Attempts to convert the configuration value to given primitive {@code T} using
-     * corresponding {@code parse} method on this type.
+     * Attempts to convert the configuration value to given primitive {@code T} using corresponding {@code parse} method
+     * on this type.
      *
      * <p><b>Following types are supported:</b></p>
      * <ul>