Skip to content
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

Jump to bottom

Recursive acl #14669

Merged
merged 19 commits into from
Sep 9, 2020
Merged

Recursive acl #14669

merged 19 commits into from
Sep 9, 2020

Conversation

rickle-msft
Copy link
Contributor

No description provided.

@ghost ghost added the Storage Storage Service (Queues, Blobs, Files) label Aug 31, 2020
@rickle-msft
Copy link
Contributor Author

I still need to add javadoc code snippets

@rickle-msft
Copy link
Contributor Author

We need to decide if recursiveAcl should go on path or directory and update some names accordingly.

@rickle-msft
Copy link
Contributor Author

Looks like it actually does belong on path, and there are some file tests I need to add. And also consequently some names to change

@kasobol-msft
Copy link
Contributor

Looks like it actually does belong on path, and there are some file tests I need to add. And also consequently some names to change

Calling recursive acl on file client is legit operation. after all it's 1 node tree.
The use case for that would be customer enumerating mixed directory/file result from previous (listing?) operation and wanted to apply new acls regardless it's a file or directory.

gapra-msft
gapra-msft reviewed Sep 1, 2020
View reviewed changes
...-storage-file-datalake/src/main/java/com/azure/storage/file/datalake/DataLakePathClient.java
PathSetAccessControlRecursiveMode.SET, options.getBatchSize(), options.getMaxBatches(),
options.isContinuingOnFailure(), options.getContinuationToken(), context);

return StorageImplUtils.blockWithOptionalTimeout(response, timeout);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do the try catch error thing here so we can log it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea idk why we don't do it on all the datalake methods. I'll add it to all the other ones, too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Looks like we don't do that for any of the methods in BlobClientBase either, so I think it'd be better to do that as a separate PR

gapra-msft
gapra-msft reviewed Sep 1, 2020
View reviewed changes
...va/com/azure/storage/file/datalake/options/DirectoryUpdateAccessControlRecursiveOptions.java Outdated
* @return The POSIX access control list for the file or directory.
*/
public List<PathAccessControlEntry> getAccessControlList() {
return accessControlList;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return Collections.unmodifiable list here as well.

gapra-msft
gapra-msft reviewed Sep 1, 2020
View reviewed changes
...va/com/azure/storage/file/datalake/options/DirectoryUpdateAccessControlRecursiveOptions.java Outdated
* @param accessControlList The POSIX access control list for the file or directory.
*/
public DirectoryUpdateAccessControlRecursiveOptions(List<PathAccessControlEntry> accessControlList) {
this.accessControlList = Collections.unmodifiableList(accessControlList);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a null check here? or protect the call to Collections.unmodifiable list

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea. And I think I need to add null checks to the internal implementation method, too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

gapra-msft
gapra-msft reviewed Sep 1, 2020
View reviewed changes
...atalake/src/main/java/com/azure/storage/file/datalake/models/AccessControlChangeFailure.java
* @param directory Whether entry is a directory.
* @return The updated object.
*/
public AccessControlChangeFailure setDirectory(boolean directory) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds kinda weird. Do we have other examples of this? It feels more normal to call this setIsDirectory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. But can we do setIsDirectory and isDirectory. I'm not sure if the guidelines ok with there being an extra "is" in the setter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://stackoverflow.com/questions/5322648/for-a-boolean-field-what-is-the-naming-convention-for-its-getter-setter
and
https://dzone.com/articles/java-getter-and-setter-basics-common-mistakes-and

gapra-msft
gapra-msft reviewed Sep 1, 2020
View reviewed changes
...alake/src/main/java/com/azure/storage/file/datalake/models/PathRemoveAccessControlEntry.java
* @param defaultScope Whether this is the default entry for the ACL.
* @return The updated object.
*/
public PathRemoveAccessControlEntry setDefaultScope(boolean defaultScope) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as with the isDirectory, this probably sounds better as setIsDefaultScope?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I'd rather keep because it follows the same pattern as PathAccessControlEntry

gapra-msft
gapra-msft reviewed Sep 1, 2020
View reviewed changes
...lake/src/samples/java/com/azure/storage/file/datalake/PathAsyncClientJavaDocCodeSamples.java Outdated
/**
* Code snippets for {@link DataLakePathAsyncClient#removeAccessControlRecursive(List)}
*/
public void removetAccessControlRecursiveCodeSnippets() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo - remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

gapra-msft
gapra-msft reviewed Sep 1, 2020
View reviewed changes
...-storage-file-datalake/src/test/java/com/azure/storage/file/datalake/DirectoryAPITest.groovy
@@ -434,6 +444,786 @@ class DirectoryAPITest extends APISpec {
thrown(DataLakeStorageException)
}

def "Set ACL recursive min"() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have some tests on File? Maybe just a min test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh just read your other comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea they should be there now

@kasobol-msft kasobol-msft requested a review from amnguye September 1, 2020 22:22
@kasobol-msft
Copy link
Contributor

@amnguye could take a look on this PR ?

@gapra-msft gapra-msft merged commit 988e69b into Azure:feature/storage/stg74 Sep 9, 2020
gapra-msft added a commit that referenced this pull request Oct 1, 2020
@gapra-msft @rickle-msft
Implemented features for STG 74 (#15428)
a27047b 
* Updated all service versions to STG74 (#14079)

* Added code for get file range diff (#14140)

* Added code for smb multi channel (#14180)

* Added code to allow scheduling file expiry (#14319)

* Added support for arrow output serialization (#14431)

* Added support to read last access time  (#14342)

* Added support to lease shares (#14287)

* Updated file ranges to getFileRangesDiff (#14839)

* Added support for directory and delegation SAS (#14531)

* Recursive acl (#14669)

* Added missing error code (#14986)

* Added tests to ensure support for 4TB file (#15179)

Co-authored-by: Gauri Prasad <[email protected]>

* Storage/file share error code (#15007)

* Fixed simple renames and doc issues from 74 (#15297)

Co-authored-by: Gauri Prasad <[email protected]>

* Added back support for container undelete. (#15344)

* Regenerated code to address APIView comments (#15341)

* Minor changelog formatting issues

* Formatting - new lines and unused imports

* Fixed public API for file get range diff (#15562)

* Modified recursive acl tests to be able to play in live mode (#15815)

* Added support for live tests in the STG 74 branch (#15724)

* Added code to return batch failures in results for recursive ACL (#15842)

* Wrapped continuation token with Exception when recursive acl call is … (#15839)

Co-authored-by: Gauri Prasad <[email protected]>
Co-authored-by: Rick Ley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gapra-msft gapra-msft gapra-msft approved these changes

@amnguye amnguye amnguye approved these changes

@alzimmermsft alzimmermsft Awaiting requested review from alzimmermsft

@amishra-dev amishra-dev Awaiting requested review from amishra-dev

@jaschrep-msft jaschrep-msft Awaiting requested review from jaschrep-msft

@kasobol-msft kasobol-msft Awaiting requested review from kasobol-msft

Assignees
No one assigned
Labels
Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rickle-msft @kasobol-msft @amnguye @gapra-msft