Added Exists, CreateIfNotExists, and DeleteIfExists to Data Lake#9996
Conversation
seanmcc-msft
requested review from
JoshLove-msft,
amnguye,
gapra-msft and
tg-msft
| try | ||
| { | ||
| scope.Start(); | ||
| Response<BlobContainerInfo> containerResponse = await _containerClient.CreateIfNotExistsAsync( |
There was a problem hiding this comment.
Nit - this is getting close to enough logic that it might be nice to share a sync/async implementation with a CreateIfNotExistsInternal(..., bool async).
There was a problem hiding this comment.
DataLakeFileSystemClient.CreateIfNotExists() and DataLakeFileSystemClient.CreateIfNotExistsAsync() call BlobContainerClient.CreateIfNotExists() and BlobContainerClient.CreateIfNotExistsAsync(), respectively. BlobContainerClient.CreateIfNotExistsInternal() is private. I think creating a shared DataLakeFileSystemClient.CreateIfNotExistsInternal() would require duplicated most of the logic in BlobContainerClient.CreateIfNotExistsInternal(), so it would be a wash in terms of duplicate code.
There was a problem hiding this comment.
I think it could just be the following in a shared method without exposing the internal blob method:
Response<BlobContainerInfo> containerResponse = async ?
await _containerClient.CreateIfNotExistsAsync(...).ConfigureAwait(false) :
_containerClient.CreateIfNotExists(...);Again though, it doesn't matter as much for this one but it would be nice if/when you're adding other helpers with more complicated logic.
| cancellationToken).ConfigureAwait(false); | ||
| } | ||
| catch (RequestFailedException storageRequestFailedException) | ||
| when (storageRequestFailedException.ErrorCode == "PathAlreadyExists") |
There was a problem hiding this comment.
We should have a DataLakeErrorCode.PathAlreadyExists to use instead of a hard-coded string, but I just noticed that none exists. We should file a bug and get that included before GA.
| try | ||
| { | ||
| scope.Start(); | ||
| DataLakeRequestConditions conditions = new DataLakeRequestConditions { IfNoneMatch = new ETag("*") }; |
There was a problem hiding this comment.
Nit - I think we've got something like Constants.Wildcard for this string.
| scope.Start(); | ||
|
|
||
| return _blockBlobClient.Exists( | ||
| cancellationToken); |
There was a problem hiding this comment.
Style nit - not sure the extra line buys much for a single parameter, but we should at least indent
| scope.Start(); | ||
|
|
||
| return await _blockBlobClient.ExistsAsync( | ||
| cancellationToken).ConfigureAwait(false); |
| /// a failure occurs. | ||
| /// </remarks> | ||
| public virtual Response<bool> DeleteIfExists( | ||
| bool? recursive = default, |
There was a problem hiding this comment.
Is it worth making this a regular bool with a default of false or true? I don't have any good intuition what null would mean here as a customer.
There was a problem hiding this comment.
This is by design. recursive isn't a valid parameter when deleting a File, so when it is set to default, it is not included.
| // Act | ||
| await TestHelper.AssertExpectedExceptionAsync<RequestFailedException>( | ||
| unauthorizedDirecotry.CreateIfNotExistsAsync(), | ||
| e => Assert.AreEqual("AuthenticationFailed", e.ErrorCode.Split('\n')[0])); |
There was a problem hiding this comment.
Is the Split doing anything meaningful? Does the ErrorCode property return multiple lines of text?
There was a problem hiding this comment.
It does not. I've removed it from all our Storage unit tests.
2 participants
Exists(),CreateIfNotExists(), andDeleteIfExists()toDataLakeFileSystemClient,DataLakePathClient,DataLakeDirectoryClient, andDataLakeFileClient.