Skip to content

Use code customizations to map internal Storage error in generated code #41624

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.

Sign up for GitHub

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

Merged
alzimmermsft merged 4 commits into from
Aug 27, 2024
Merged

Use code customizations to map internal Storage error in generated code #41624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6898f85
Regenerate with customization to handle internal exception mapping
alzimmermsft Aug 23, 2024
0af208a
Remove manual calls to mapping the exception
alzimmermsft Aug 23, 2024
3e3329f
Merge branch 'main' into AzStorage_ChangeDesignOnInternalExceptionFix
alzimmermsft Aug 27, 2024
4c723d3
Simplify exception mapping
alzimmermsft Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 0 additions & 55 deletions eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,6 @@
<!-- Fields defaultTrustManager and keyStore might be set more than once. -->
<suppress checks="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck" files="com.azure.security.keyvault.jca.KeyVaultTrustManager"/>

<!-- Remove this after fixing: https://github.com/Azure/azure-sdk-for-java/issues/5021 -->
<suppress checks="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck" files="com.azure.storage.blob.specialized.BlobOutputStream.java"/>

<suppress checks="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck" files="com.azure.security.keyvault.secrets.models.KeyVaultSecret"/>
<suppress checks="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck" files="com.azure.security.keyvault.secrets.models.SecretProperties"/>

Expand Down Expand Up @@ -77,15 +74,6 @@
<!-- Suppress LineLength for i18n message properties -->
<suppress checks="LineLength" files="messages.properties"/>

<!-- Suppress the check on any implementation code and codegen classes -->
<suppress checks="LineLength" files=".*[/\\]storage[/\\].*implementation[/\\].*"/>
<suppress checks="LineLength" files="com.azure.storage.blob.models.BlobErrorCode.java"/>
<suppress checks="LineLength" files="com.azure.storage.file.share.models.ShareErrorCode.java"/>
<suppress checks="LineLength" files="com.azure.storage.queue.models.QueueErrorCode.java"/>

<!-- Suppress datalake for now as it is new development -->
<suppress checks="LineLength" files=".*[/\\]storage[/\\]file[/\\]datalake[/\\]models[/\\].*"/>

<!-- Identity and opentelemetry tracing are plugin packages and shouldn't be referenced -->
<suppress checks="IllegalImport" files=".*[/\\]com[/\\]azure[/\\]monitor[/\\]opentelemetry[/\\]exporter[/\\]*"/>
<suppress checks="IllegalImport" files=".*[/\\]com[/\\]azure[/\\]monitor[/\\]applicationinsights[/\\]spring[/\\]*"/>
Expand Down Expand Up @@ -146,29 +134,6 @@ the main ServiceBusClientBuilder. -->
<!-- Suppress warning for ServiceClients to have Client in its name as Azure Container Registry has Clientlets which do not follow the same convention. -->
<suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck" files="(ContainerRepository|RegistryArtifact)(Async)?.java"/>

<!-- Suppress public/private constructor check since BlobClients need protected constructors to create EncryptedClients -->
<suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck"
Comment thread
alzimmermsft marked this conversation as resolved.
files="com.azure.storage.blob.(BlobClient|BlobAsyncClient).java"/>
<!-- Suppress client mutability check since LeaseClients need to update a changing lease ID -->
<suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck"
files="com.azure.storage.blob.specialized.BlobLeaseAsyncClient.java"/>
<suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck"
files="com.azure.storage.file.share.specialized.ShareLeaseAsyncClient.java"/>

<!-- Suppress IO exception for now, which need code owner's attention -->
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck"
files="com.azure.storage.blob.specialized.cryptography.EncryptedBlobClient.java"/>
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck"
files="com.azure.storage.blob.specialized.(BlobInputStream|BlobOutputStream).java"/>
<!-- Suppress IO exception in the NIO package because they are required by the interface -->
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck"
files="com.azure.storage.blob.nio/*"/>
<suppress checks="com.azure.tools.checkstyle.checks.UseCaughtExceptionCauseCheck"
files="com.azure.storage.blob.nio.AzureFileSystemProvider.java"/>

<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck"
files="com.azure.storage.blob.batch.BlobBatchOperationResponse.java"
lines="110"/>
<!-- InvalidKeyException is not a runtime exception, issue link: https://github.com/Azure/azure-sdk-for-java/issues/5178 -->
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLogger"
files="com.azure.security.keyvault.keys.cryptography.(AesCbc|AesCbcPad).java"/>
Expand All @@ -184,13 +149,6 @@ the main ServiceBusClientBuilder. -->
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck"
files="com.azure.containers.containerregistry.ContainerRegistryContentClient.java"/>


<!-- Nested blocks are still readable in the switch statements in Avro. -->
<suppress checks="AvoidNestedBlocks" files="com.azure.storage.internal.avro.implementation.schema.(AvroSchema|AvroType).java"/>

<!-- The constant name is public static. -->
<suppress checks="ConstantNames" files=".*[/\\]azure[/\\]storage[/\\].*LoggingOptions.java"/>

<suppress checks="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck"
files="com.azure.monitor.opentelemetry.exporter.AzureMonitorExporterBuilder"/>

Expand Down Expand Up @@ -231,8 +189,6 @@ the main ServiceBusClientBuilder. -->
<suppress checks="com.azure.tools.checkstyle.checks.HttpPipelinePolicy" files="com.azure.containers.containerregistry.implementation.authentication.ContainerRegistryCredentialsPolicy.java"/>
<suppress checks="com.azure.tools.checkstyle.checks.HttpPipelinePolicy" files="com.azure.messaging.servicebus.implementation.ServiceBusTokenCredentialHttpPolicy.java"/>
<suppress checks="com.azure.tools.checkstyle.checks.HttpPipelinePolicy" files="com.azure.messaging.eventgrid.implementation.CloudEventTracingPipelinePolicy.java"/>
<suppress checks="com.azure.tools.checkstyle.checks.HttpPipelinePolicy" files="com.azure.storage.common.implementation.policy.SasTokenCredentialPolicy.java"/>
<suppress checks="com.azure.tools.checkstyle.checks.HttpPipelinePolicy" files="com.azure.storage.blob.implementation.util.BlobUserAgentModificationPolicy.java"/>

<!-- Fields tenantId, clientId and clientSecret are not set in all constructors. -->
<suppress checks="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck" files="com.azure.security.keyvault.jca.implementation.KeyVaultClient"/>
Expand All @@ -246,9 +202,6 @@ the main ServiceBusClientBuilder. -->
<!-- This class overrides a method that throws, so it cannot be avoided. -->
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.security.keyvault.jca.KeyVaultTrustManager"/>

<!-- Empty while loop waiting for Reactor stream completion -->
<suppress checks="EmptyBlock" files="com.azure.storage.blob.batch.BlobBatch.java"/>

<suppress checks="."
files=".*[/\\]webpubsub[/\\]implementation[/\\]"/>
<suppress checks="."
Expand All @@ -262,7 +215,6 @@ the main ServiceBusClientBuilder. -->

<!-- Only checks Public Api classes -->
<suppress checks="com.azure.tools.checkstyle.checks.DenyListedWordsCheck" files=".*[/\\](implementation|test)[/\\].*"/>
<suppress checks="com.azure.tools.checkstyle.checks.DenyListedWordsCheck" files=".*[/\\]storage[/\\].*samples[/\\].*"/>

<!-- FIXME temp -->
<suppress checks="ServiceClient" files="com.azure.messaging.webpubsub.*"/>
Expand Down Expand Up @@ -440,13 +392,6 @@ the main ServiceBusClientBuilder. -->
<suppress checks="IllegalImport" files="com.azure.*.stress"/>
<suppress checks="ThrowFromClientLogger" files="com.azure.*.stress"/>

<!-- AzureFileSystemProvider requires a deferred instantiation of the static logger to prevent a potential runtime exception. -->
<suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="com.azure.storage.blob.nio.AzureFileSystemProvider"/>

<!-- EncryptedBlobAsyncClient and EncryptedBlobClient overriding methods for download behavior; cannot change signature -->
<suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck"
files="com.azure.storage.blob.specialized.cryptography.(EncryptedBlobAsyncClient|EncryptedBlobClient)"/>

<!-- Star import is ok in impl class - it makes it easier to import all parent APIs, etc -->
<suppress checks="AvoidStarImport" files="com.azure.jedis.AzureJedisClient.java"/>

Expand Down
117 changes: 1 addition & 116 deletions eng/code-quality-reports/src/main/resources/spotbugs/spotbugs-exclude.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,7 @@
<!-- Suppress non-null warning in the case that we change the code and it is possible for
KeyVaultCredentials.getAuthenticationCredentials to return null. -->
<Match>
<Or>
<Class name="com.microsoft.azure.keyvault.authentication.KeyVaultCredentials"/>
<Class name="com.azure.storage.blob.FileTransferExample"/>
</Or>
<Class name="com.microsoft.azure.keyvault.authentication.KeyVaultCredentials"/>
<Bug pattern="RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE"/>
</Match>

Expand Down Expand Up @@ -411,19 +408,6 @@
<Package name="~com\.microsoft\.azure\.storage\.blob\.model(.+)"/>
</Match>

<!-- Exclude the confusing name bugs as it is designed to name the same as autorest. -->
<Match>
<Or>
<Class name="com.azure.storage.blob.HttpGetterInfo"/>
<Class name="com.azure.storage.blob.BlobProperties"/>
<Class name="com.azure.storage.blob.BlobContainerProperties"/>
<Class name="com.azure.storage.file.share.implementation.models.DirectoryCreateHeaders"/>
<Class name="com.azure.storage.blob.implementation.models.BlobDownloadHeaders"/>
<Class name="com.azure.storage.blob.models.BlobQueryHeaders"/>
</Or>
<Bug pattern="NM_CONFUSING"/>
</Match>

<!-- The public field already exists in the public API surface area -->
<Match>
<Class name="~com\.microsoft\.azure\.servicebus\.(TransactionContext|management\.ManagementClientConstants)"/>
Expand Down Expand Up @@ -482,24 +466,6 @@
<Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/>
</Match>

<!-- Incorrect flagging, if the response is null a StorageException would have been thrown -->
<Match>
<Or>
<Class name="com.azure.storage.blob.specialized.BlobInputStream"/>
<Class name="com.azure.storage.file.share.StorageFileInputStream"/>
<Class name="com.azure.storage.blob.specialized.BlobOutputStream$AppendBlobOutputStream"/>
<Class name="com.azure.storage.queue.QueueServiceClient"/>
</Or>
<Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/>
</Match>

<!-- Incorrect flagging, all the optional values can be null -->
<Match>
<Class name="com.azure.storage.blob.BlobClient"/>
<Method name="downloadToFile"/>
<Bug pattern="NP_NULL_PARAM_DEREF_ALL_TARGETS_DANGEROUS"/>
</Match>

<!-- Jackson property setter method is private -->
<Match>
<Or>
Expand Down Expand Up @@ -541,8 +507,6 @@
<Or>
<Class name="~.*JavaDoc(CodeSnippets|CodeSamples|Samples)"/>
<Class name="~.*ReadmeSamples"/>
<Class name="com.azure.storage.blob.batch.ReadmeCodeSamples"/>
<Class name="com.azure.storage.file.datalake.GetSetAccessControlExample"/>
</Or>
<Bug pattern="DLS_DEAD_LOCAL_STORE,
URF_UNREAD_FIELD,
Expand Down Expand Up @@ -575,12 +539,6 @@
<Bug pattern="NM_CONFUSING"/>
</Match>

<!-- This is a false positive, it is possible for usingUserDelegation to be false. -->
<Match>
<Class name="com.azure.storage.blob.specialized.BlobServiceSasSignatureValues"/>
<Bug pattern="UC_USELESS_CONDITION"/>
</Match>

<!-- Retry thus does not throw.
SharedTokenCacheCredential needs to catch any catch exception that gets thrown.-->
<Match>
Expand All @@ -596,48 +554,6 @@
<Bug pattern="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE"/>
</Match>

<!-- The field is read by parent class StorageOutputStream methods. -->
<Match>
<Class name="com.azure.storage.file.share.StorageFileOutputStream"/>
<Bug pattern="URF_UNREAD_FIELD"/>
</Match>

<!-- Initial implementation of Storage Blob Batch is using an inline class -->
<Match>
<Class name="com.azure.storage.blob.batch.BlobBatchAsyncClient"/>
<Bug pattern="SIC_INNER_SHOULD_BE_STATIC_ANON"/>
</Match>

<!-- Implementation name confusing with public API name and non-public API -->
<Match>
<Or>
<Class name="com.azure.storage.blob.implementation.models.AppendBlobAppendBlockFromUrlHeaders"/>
<Class name="com.azure.storage.file.share.implementation.models.FileGetPropertiesHeaders"/>
<Class name="com.azure.storage.file.datalake.implementation.models.PathAppendDataHeaders"/>
</Or>
<Bug pattern="NM_CONFUSING"/>
</Match>

<!-- Returning null is correct in these cases as it represents the not-applicable case -->
<Match>
<Or>
<And>
<Class name="com.azure.storage.blob.nio.AzureBlobFileAttributes"/>
<Method name="isServerEncrypted"/>
</And>
<And>
<Class name="com.azure.storage.blob.nio.AzureBlobFileAttributes"/>
<Method name="isAccessTierInferred"/>
</And>
</Or>
<Bug pattern="NP_BOOLEAN_RETURN_NULL"/>
</Match>

<Match>
<Class name="com.azure.storage.blob.implementation.models.BlobPropertiesInternalDownload"/>
<Bug pattern="NP_LOAD_OF_KNOWN_NULL_VALUE,SIC_INNER_SHOULD_BE_STATIC_ANON,UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS,NP_BOOLEAN_RETURN_NULL"/>
</Match>

<!-- Disable some spotbugs rules on perf-tests -->
<Match>
<Or>
Expand Down Expand Up @@ -1891,37 +1807,6 @@
<Bug pattern="DM_EXIT"/>
</Match>

<!-- Internal Avro Parser does read the blockCount field. Seems to be a false positive. -->
<Match>
<Or>
<Class name="com.azure.storage.internal.avro.implementation.schema.complex.AvroArraySchema"/>
<Class name="com.azure.storage.internal.avro.implementation.schema.complex.AvroMapSchema"/>
<Class name="com.azure.storage.internal.avro.implementation.schema.file.AvroBlockSchema"/>
</Or>
<Bug pattern="DLS_DEAD_LOCAL_STORE"/>
</Match>

<!-- Internal Avro Parser does check the types before casting. Seems to be a false positive. -->
<Match>
<Class name="com.azure.storage.internal.avro.implementation.schema.AvroSchema"/>
<Method name="getSchema"/>
<Bug pattern="BC_UNCONFIRMED_CAST"/>
</Match>

<!-- FluxInputStream definitely throws an IOException since this.lastError is an IOException. -->
<Match>
<Class name="com.azure.storage.common.implementation.FluxInputStream"/>
<Method name="read"/>
<Bug pattern="BC_UNCONFIRMED_CAST_OF_RETURN_VALUE"/>
</Match>

<!-- This dead store is due to a deficiency in the Customization class. We need to reference the original return type but we have no use for it so we just comment it out -->
<Match>
<Class name="com.azure.storage.blob.models.Block"/>
<Method name="getSize"/>
<Bug pattern="DLS_DEAD_LOCAL_STORE"/>
</Match>

<!-- Exclude REC_CATCH_EXCEPTION and false positive error for management samples. All samples in management catch Exception instead of throw it. -->
<Match>
<Package name="~com\.azure\.resourcemanager(\.[^.]+)*\.samples(\.[^.]+)*"/>
Expand Down
22 changes: 6 additions & 16 deletions ...age/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobContainerAsyncClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,8 +427,7 @@ Mono<Response<Void>> createWithResponse(Map<String, String> metadata, PublicAcce
Context context) {
context = context == null ? Context.NONE : context;
return this.azureBlobStorage.getContainers().createNoCustomHeadersWithResponseAsync(containerName, null,
metadata, accessType, null, blobContainerEncryptionScope, context)
.onErrorMap(ModelHelper::mapToBlobStorageException);
metadata, accessType, null, blobContainerEncryptionScope, context);
}

/**
Expand Down Expand Up @@ -576,8 +575,7 @@ Mono<Response<Void>> deleteWithResponse(BlobRequestConditions requestConditions,

return this.azureBlobStorage.getContainers().deleteNoCustomHeadersWithResponseAsync(containerName, null,
requestConditions.getLeaseId(), requestConditions.getIfModifiedSince(),
requestConditions.getIfUnmodifiedSince(), null, context)
.onErrorMap(ModelHelper::mapToBlobStorageException);
requestConditions.getIfUnmodifiedSince(), null, context);
}

/**
Expand Down Expand Up @@ -718,7 +716,6 @@ Mono<Response<BlobContainerProperties>> getPropertiesWithResponse(String leaseId

return this.azureBlobStorage.getContainers()
.getPropertiesWithResponseAsync(containerName, null, leaseId, null, context)
.onErrorMap(ModelHelper::mapToBlobStorageException)
.map(rb -> {
ContainersGetPropertiesHeaders hd = rb.getDeserializedHeaders();
BlobContainerProperties properties = new BlobContainerProperties(hd.getXMsMeta(), hd.getETag(),
Expand Down Expand Up @@ -802,8 +799,7 @@ Mono<Response<Void>> setMetadataWithResponse(Map<String, String> metadata,
}

return this.azureBlobStorage.getContainers().setMetadataNoCustomHeadersWithResponseAsync(containerName, null,
requestConditions.getLeaseId(), metadata, requestConditions.getIfModifiedSince(), null, context)
.onErrorMap(ModelHelper::mapToBlobStorageException);
requestConditions.getLeaseId(), metadata, requestConditions.getIfModifiedSince(), null, context);
}

/**
Expand Down Expand Up @@ -871,7 +867,6 @@ Mono<Response<BlobContainerAccessPolicies>> getAccessPolicyWithResponse(String l
context = context == null ? Context.NONE : context;
return this.azureBlobStorage.getContainers().getAccessPolicyWithResponseAsync(
containerName, null, leaseId, null, context)
.onErrorMap(ModelHelper::mapToBlobStorageException)
.map(response -> new SimpleResponse<>(response,
new BlobContainerAccessPolicies(response.getDeserializedHeaders().getXMsBlobPublicAccess(),
response.getValue().items())));
Expand Down Expand Up @@ -995,8 +990,7 @@ OffsetDateTime.now will only give back milliseconds (more precise fields are zer

return this.azureBlobStorage.getContainers().setAccessPolicyNoCustomHeadersWithResponseAsync(containerName,
null, requestConditions.getLeaseId(), accessType, requestConditions.getIfModifiedSince(),
requestConditions.getIfUnmodifiedSince(), null, identifiers, context)
.onErrorMap(ModelHelper::mapToBlobStorageException);
requestConditions.getIfUnmodifiedSince(), null, identifiers, context);
}

/**
Expand Down Expand Up @@ -1203,8 +1197,7 @@ PagedFlux<BlobItem> listBlobsFlatWithOptionalTimeout(ListBlobsOptions options, S
return StorageImplUtils.applyOptionalTimeout(
this.azureBlobStorage.getContainers().listBlobFlatSegmentWithResponseAsync(containerName,
options.getPrefix(), marker, options.getMaxResultsPerPage(), include, null, null, Context.NONE),
timeout)
.onErrorMap(ModelHelper::mapToBlobStorageException);
timeout);
}

/**
Expand Down Expand Up @@ -1378,8 +1371,7 @@ PagedFlux<BlobItem> listBlobsHierarchyWithOptionalTimeout(String delimiter, List
this.azureBlobStorage.getContainers().listBlobHierarchySegmentWithResponseAsync(containerName, delimiter,
options.getPrefix(), marker, options.getMaxResultsPerPage(), include, null, null,
Context.NONE),
timeout)
.onErrorMap(ModelHelper::mapToBlobStorageException);
timeout);
}

/**
Expand Down Expand Up @@ -1462,7 +1454,6 @@ private Mono<PagedResponse<TaggedBlobItem>> findBlobsByTags(
return StorageImplUtils.applyOptionalTimeout(
this.azureBlobStorage.getContainers().filterBlobsWithResponseAsync(containerName, null, null,
options.getQuery(), marker, options.getMaxResultsPerPage(), null, context), timeout)
.onErrorMap(ModelHelper::mapToBlobStorageException)
.map(response -> {
List<TaggedBlobItem> value = response.getValue().getBlobs().stream()
.map(ModelHelper::populateTaggedBlobItem)
Expand Down Expand Up @@ -1529,7 +1520,6 @@ public Mono<Response<StorageAccountInfo>> getAccountInfoWithResponse() {
Mono<Response<StorageAccountInfo>> getAccountInfoWithResponse(Context context) {
context = context == null ? Context.NONE : context;
return this.azureBlobStorage.getContainers().getAccountInfoWithResponseAsync(containerName, context)
.onErrorMap(ModelHelper::mapToBlobStorageException)
.map(rb -> {
ContainersGetAccountInfoHeaders hd = rb.getDeserializedHeaders();
return new SimpleResponse<>(rb, new StorageAccountInfo(hd.getXMsSkuName(), hd.getXMsAccountKind()));
Expand Down
Loading