-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Added Exists(), CreateIfNotExists(), and DeleteIfExists() to ShareCli… #10074
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.
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
Added Exists(), CreateIfNotExists(), and DeleteIfExists() to ShareCli… #10074
Conversation
…ent, FileClient, and DirectoryClient
|
@tg-msft, I'm thinking maybe we could potentially start grouping our optional parameters in this PR. What do you think? |
| } | ||
| #endregion Create | ||
|
|
||
| #region Create If Not Exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT - remove spaces. Also I believe for blobs, I put the CreateIfNotExists methods in the Create region
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed CreateIfNotExists per Ted's comment.
| } | ||
| #endregion Exists | ||
|
|
||
| #region Delete If Exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT - Same thing I said about Create
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed the region DeleteIfExists.
| Response<bool> existsResponse = await ExistsInternal( | ||
| async, | ||
| cancellationToken, | ||
| operationName).ConfigureAwait(false); | ||
|
|
||
| if (existsResponse.Value) | ||
| { | ||
| return null; | ||
| } | ||
| else | ||
| { | ||
| return await CreateInternal( | ||
| maxSize, | ||
| httpHeaders, | ||
| metadata, | ||
| smbProperties, | ||
| filePermission, | ||
| conditions, | ||
| async, | ||
| cancellationToken, | ||
| operationName) | ||
| .ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause race conditions. It would be better to try the Create call and ignore errors if it already exists like you did for the directory client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.microsoft.com/en-us/rest/api/storageservices/create-file
I can't call Create an ignore errors, Create File overwrites existing files, and there are no conditional request headers. I think this might be the only way to do CreateIfNotExists for Files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... yeah, that's kind of scary. We don't want to give people APIs they can't be successful with so I think we should only add Exists and DeleteIfExists for now for File Shares.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed CreateIfNotExists from File Share.
…ent, FileClient, and DirectoryClient